Issue 29737561: Issue 6539, 6782 - Implement support for snippets (Closed)
|
Created:
March 30, 2018, 8:25 p.m. by Manish Jethani Modified:
July 20, 2018, 5:23 p.m. CC:
Felix Dahlke, mapx Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
DescriptionDepends on adblockpluscore hg:8e466288a0c3
Patch Set 1 #
Total comments: 10
Patch Set 2 : Use JSON.stringify #Patch Set 3 : Load snippet library like a module #
Total comments: 4
Patch Set 4 : Add support for remote loading #
Total comments: 4
Patch Set 5 : Load public key off disk #Patch Set 6 : Add link to Chromium bug #Patch Set 7 : Check filter against current frame, polish up #Patch Set 8 : Use adblockpluscore/lib/snippets.js for script parsing #Patch Set 9 : Move script compilation to Core #Patch Set 10 : Use top-level compileScript #Patch Set 11 : Update snippets.js to include just one log function #
Total comments: 2
Patch Set 12 : Wrap tabs.executeScript #Patch Set 13 : Ignore error when frame is removed #Patch Set 14 : Update lib/cssInjection.js to use CodeInjectionFilter.code #Patch Set 15 : Rebase #
Total comments: 5
Patch Set 16 : Rebase, simplify, execute script on "snippets.executeScripts" message #
Total comments: 15
Patch Set 17 : Rebase, use snippets library from core #Patch Set 18 : Update adblockpluscore dependency #Patch Set 19 : Merge elemhide and snippets #Patch Set 20 : Remove elemhideWhitelisted temporary variable #
Total comments: 4
Patch Set 21 : Rename ContentFiltering to Content #
Total comments: 42
Patch Set 22 : Remove dependency update #Patch Set 23 : Clean up messaging for content.applyFilters #Patch Set 24 : Rename Content to ContentFiltering #Patch Set 25 : Rename lib/scriptInjection.js to lib/contentFiltering.js #Patch Set 26 : Fix formatting for content.applyFilters message #Patch Set 27 : Add explanatory comment in catch block #
MessagesTotal messages: 44
Patch Set 1 https://codereview.adblockplus.org/29737561/diff/29737562/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29737561/diff/29737562/lib/snippets.js#new... lib/snippets.js:32: fetch(browser.extension.getURL("/snippets.js"), {cache: "no-cache"}) Using the fetch API to load the snippets library. We could also use XMLHttpRequest or something similar. https://codereview.adblockplus.org/29737561/diff/29737562/lib/snippets.js#new... lib/snippets.js:36: library = text; The "library" is the entire text of the file. See ./snippets.js for an example. It's a file that defines an anonymous object, each of the properties of which is a function. e.g. { hello() { console.log("Hello, world!"); } } https://codereview.adblockplus.org/29737561/diff/29737562/lib/snippets.js#new... lib/snippets.js:44: let name = snippet.replace("\"", "\\\""); The name could contains quotes so we need to escape those. Actually we should escape any non-whitespace escapable characters. https://codereview.adblockplus.org/29737561/diff/29737562/lib/snippets.js#new... lib/snippets.js:59: if (!/^https?:\/\//.test(details.url)) I'll try to make a standalone test case for this and log it in the Chromium bugbase.
https://codereview.adblockplus.org/29737561/diff/29737562/snippets.js File snippets.js (right): https://codereview.adblockplus.org/29737561/diff/29737562/snippets.js#newcode2 snippets.js:2: hello() So if you want to invoke this snippet on example.com you have to write the following filter: ||example.com^$snippet=hello&domain=example.com
https://codereview.adblockplus.org/29737561/diff/29737562/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29737561/diff/29737562/lib/snippets.js#new... lib/snippets.js:32: fetch(browser.extension.getURL("/snippets.js"), {cache: "no-cache"}) So this is literally just a file that is bundled with the extension. In the future (version 2) we want to be able to update the snippets library dynamically. https://codereview.adblockplus.org/29737561/diff/29737562/lib/snippets.js#new... lib/snippets.js:69: thirdParty, null, true); Note that here we're explicitly asking for domain-specific filters. I'm not sure if generic filters make sense here, but we could keep it domain-specific for the initial implementation anyway.
Patch Set 2: Use JSON.stringify https://codereview.adblockplus.org/29737561/diff/29737562/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29737561/diff/29737562/lib/requestBlocker.... lib/requestBlocker.js:72: yield "ELEMHIDE"; Just rearranged this for consistency. https://codereview.adblockplus.org/29737561/diff/29737562/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29737561/diff/29737562/lib/snippets.js#new... lib/snippets.js:44: let name = snippet.replace("\"", "\\\""); On 2018/03/30 20:33:15, Manish Jethani wrote: > The name could contains quotes so we need to escape those. Actually we should > escape any non-whitespace escapable characters. It turns out the correct way to quote a string is using JSON.stringify. Done. https://codereview.adblockplus.org/29737561/diff/29737562/metadata.chrome File metadata.chrome (right): https://codereview.adblockplus.org/29737561/diff/29737562/metadata.chrome#new... metadata.chrome:120: snippets.js = snippets.js This by the way will eventually come from a separate repo. e.g. snippets.js = adblockplussnippets/snippets.js
Patch Set 3: Load snippet library like a module https://codereview.adblockplus.org/29737561/diff/29738565/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29737561/diff/29738565/lib/snippets.js#new... lib/snippets.js:31: let executableCode = new Map(); This cache is not strictly necessary but it would be stupid to generate the executable code for each call. https://codereview.adblockplus.org/29737561/diff/29738565/lib/snippets.js#new... lib/snippets.js:50: new Function("exports", ${JSON.stringify(library)})(imports); So we're loading the snippet library as a function and calling it with an imports object, which it populates. https://codereview.adblockplus.org/29737561/diff/29738565/lib/snippets.js#new... lib/snippets.js:52: if (Object.prototype.hasOwnProperty.call(imports, key)) Just to be extra safe it's better to check that the key indeed is in the object itself and not in the object's prototype chain. https://codereview.adblockplus.org/29737561/diff/29738565/snippets.js File snippets.js (right): https://codereview.adblockplus.org/29737561/diff/29738565/snippets.js#newcode22 snippets.js:22: function hello() So this is now like a regular JS module. It has an implicit "exports" variable that it can assign to.
Patch Set 4: Add support for remote loading https://codereview.adblockplus.org/29737561/diff/29738574/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29737561/diff/29738574/lib/snippets.js#new... lib/snippets.js:32: // Dummy public key and remote URL The key is only for illustration purposes. If you want the corresponding private key, look in adblockpluscore/test/signatures.js FWIW I think the public key itself should be a separate file, bundled with the extension. https://codereview.adblockplus.org/29737561/diff/29738574/lib/snippets.js#new... lib/snippets.js:53: return fetchText(url + ".sig").then( Even though we load the code over a TLS connection, it's a good idea to have signature verification as a second line of defense (just in case of any vulnerabilities in the browser's TLS implementation). Also note that we never run the code in the background page's context, only in the context of content scripts. https://codereview.adblockplus.org/29737561/diff/29738574/lib/snippets.js#new... lib/snippets.js:103: let imports = Object.assign(Object.create(null), Remote imports override local imports. https://codereview.adblockplus.org/29737561/diff/29738574/lib/snippets.js#new... lib/snippets.js:132: // Only Chrome supports dynamic loading of JS. According to Felix Mozilla doesn't allow loading JS dynamically. It's just a policy, not enforced.
Patch Set 5: Load public key off disk
(Adding Mapx to Cc at his request.)
Patch Set 6: Add link to Chromium bug Patch Set 7: Check filter against current frame, polish up
shouldn't snippets.js and lib/snippets.js be in adblockpluscore instead?
On 2018/04/19 12:27:11, hub wrote: > shouldn't snippets.js and lib/snippets.js be in adblockpluscore instead? I've moved most of the logic to Core now.
Patch Set 8: Use adblockpluscore/lib/snippets.js for script parsing Patch Set 9: Move script compilation to Core
Patch Set 10: Use top-level compileScript Patch Set 11: Update snippets.js to include just one log function You can test this now by applying all the patches and then adding the filter "example.com#$#log 'Hello from Adblock Plus!'". You'll see this printed in the tab's console when you visit example.com.
https://codereview.adblockplus.org/29737561/diff/29762693/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29762693/lib/scriptInjection... lib/scriptInjection.js:106: runAt: "document_start" I am getting multiple occurence of Unchecked runtime.lastError while running tabs.executeScript: The frame was removed. at injectCode (<URL>) at browser.webNavigation.onCommitted.addListener (<URL>)
Patch Set 12: Wrap tabs.executeScript Patch Set 13: Ignore error when frame is removed https://codereview.adblockplus.org/29737561/diff/29762693/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29762693/lib/scriptInjection... lib/scriptInjection.js:106: runAt: "document_start" On 2018/04/27 03:05:20, hub wrote: > I am getting multiple occurence of > > Unchecked runtime.lastError while running tabs.executeScript: The frame was > removed. > at injectCode (<URL>) > at browser.webNavigation.onCommitted.addListener (<URL>) Fixed.
https://codereview.adblockplus.org/29737561/diff/29768643/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29768643/lib/scriptInjection... lib/scriptInjection.js:38: response => response.ok ? response.text() : "" you should return a promise if response is not ok.
https://codereview.adblockplus.org/29737561/diff/29768643/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29768643/lib/scriptInjection... lib/scriptInjection.js:38: response => response.ok ? response.text() : "" On 2018/05/09 12:23:00, hub wrote: > you should return a promise if response is not ok. The function already returns a promise. That promise is resolved with a value. If the value is also a promise (as in the case of response.text()), it is resolved with the value returned by that promise. So there's no need to return a promise here since we already have the value.
https://codereview.adblockplus.org/29737561/diff/29768643/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29768643/lib/scriptInjection... lib/scriptInjection.js:38: response => response.ok ? response.text() : "" On 2018/05/09 13:47:38, Manish Jethani wrote: > On 2018/05/09 12:23:00, hub wrote: > > you should return a promise if response is not ok. > > The function already returns a promise. That promise is resolved with a value. > If the value is also a promise (as in the case of response.text()), it is > resolved with the value returned by that promise. > > So there's no need to return a promise here since we already have the value. ok.
https://codereview.adblockplus.org/29737561/diff/29768643/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29768643/lib/scriptInjection... lib/scriptInjection.js:107: }) shouldn't we log that the filter was applied (ie injected) into the page?
Patch Set 16: Rebase, simplify, execute script on "snippets.executeScripts" message Comments inline. https://codereview.adblockplus.org/29737561/diff/29768643/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29768643/lib/scriptInjection... lib/scriptInjection.js:107: }) On 2018/05/09 17:19:49, hub wrote: > shouldn't we log that the filter was applied (ie injected) into the page? You're right, let me look into that. https://codereview.adblockplus.org/29737561/diff/29787589/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29737561/diff/29787589/include.preload.js#... include.preload.js:595: browser.runtime.sendMessage({type: "snippets.executeScripts"}); Send message to background page to execute any scripts. https://codereview.adblockplus.org/29737561/diff/29787589/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29787589/lib/scriptInjection... lib/scriptInjection.js:175: executableCode.set(script, code); Cache executable. https://codereview.adblockplus.org/29737561/diff/29787589/lib/scriptInjection... lib/scriptInjection.js:207: RegExpFilter.typeMap.DOCUMENT)) Note: I'm not checking for ELEMHIDE here. I don't think ELEMHIDE should apply to snippets. I'm not sure what we should do here, but applying ELEMHIDE seems wrong. https://codereview.adblockplus.org/29737561/diff/29787589/lib/scriptInjection... lib/scriptInjection.js:215: }); Note: No DevTools logging, I'll get it to eventually. https://codereview.adblockplus.org/29737561/diff/29787589/lib/scriptInjection... lib/scriptInjection.js:267: fetch(browser.extension.getURL("/snippets.js"), {cache: "no-cache"}) Note: I deleted the part that loads a remote version of the library, checks the signature, and so on. Let's keep it simple so we have something working.
https://codereview.adblockplus.org/29737561/diff/29787589/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29737561/diff/29787589/include.preload.js#... include.preload.js:595: browser.runtime.sendMessage({type: "snippets.executeScripts"}); On 2018/05/23 05:12:45, Manish Jethani wrote: > Send message to background page to execute any scripts. How about using a combined message for element hiding and snippets, keeping the amount of messaging between content script and background page minimal?
https://codereview.adblockplus.org/29737561/diff/29787589/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29787589/lib/scriptInjection... lib/scriptInjection.js:207: RegExpFilter.typeMap.DOCUMENT)) On 2018/05/23 05:12:46, Manish Jethani wrote: > Note: I'm not checking for ELEMHIDE here. I don't think ELEMHIDE should apply to > snippets. I'm not sure what we should do here, but applying ELEMHIDE seems > wrong. So would a #@# exception filter be honoured? https://codereview.adblockplus.org/29737561/diff/29787589/lib/scriptInjection... lib/scriptInjection.js:215: }); On 2018/05/23 05:12:46, Manish Jethani wrote: > Note: No DevTools logging, I'll get it to eventually. Please could you add this as a known limitation to the issue description? https://codereview.adblockplus.org/29737561/diff/29787589/lib/scriptInjection... lib/scriptInjection.js:267: fetch(browser.extension.getURL("/snippets.js"), {cache: "no-cache"}) No need to fetch this script, and to add the mapping in the metadata, we could do something like let snippetLibraries = require("../snippets");` right?
With the current implementation, snippets would be injected as plain content
scripts, i.e. run in their own context which has benefits in some scenarios, but
snippets that attempt to mock JavaScript APIs or mess with the state of the web
application itself, would have to explicitly inject a <script> element in order
to run code in the context of the page. This boilerplate is non-trivial and
would have to be repeated in multiple snippet. Here is the correct
implementation (currently used to inject for the WebRTC wrapper):
if (document instanceof HTMLDocument)
{
let sandbox = window.frameElement &&
window.frameElement.getAttribute("sandbox");
if (typeof sandbox != "string" ||
/(^|\s)allow-scripts(\s|$)/i.test(sandbox))
{
let script = document.createElement("script");
let code = "(" + injected + ")('" + randomEventName + "');";
script.type = "application/javascript";
script.async = false;
// Firefox 58 only bypasses site CSPs when assigning to 'src',
// while Chrome 67 only bypasses site CSPs when using 'textContent'.
if (browser.runtime.getURL("").startsWith("chrome-extension://"))
{
script.textContent = code;
document.documentElement.appendChild(script);
}
else
{
let url = URL.createObjectURL(new Blob([code]));
script.src = url;
document.documentElement.appendChild(script);
URL.revokeObjectURL(url);
}
document.documentElement.removeChild(script);
}
}
So maybe we should have an option (given in the snippets themselves, not in the
filter text), to control whether to inject it as plain content scripts, or wrap
it in order to run in the page context?
On 2018/07/11 17:21:09, Sebastian Noack wrote:
> With the current implementation, snippets would be injected as plain content
> scripts, i.e. run in their own context which has benefits in some scenarios,
but
> snippets that attempt to mock JavaScript APIs or mess with the state of the
web
> application itself, would have to explicitly inject a <script> element in
order
> to run code in the context of the page. This boilerplate is non-trivial and
> would have to be repeated in multiple snippet. Here is the correct
> implementation (currently used to inject for the WebRTC wrapper):
>
> [...]
>
> So maybe we should have an option (given in the snippets themselves, not in
the
> filter text), to control whether to inject it as plain content scripts, or
wrap
> it in order to run in the page context?
Yes, I already ran into this issue when implementing some snippets for testing.
Here's what I came up with:
function injectCode(code)
{
// inject code into the document using a script tag
}
function stringifyFunctionCall(func, ...params)
{
return `(${func})(${params.map(JSON.stringify).join(",")});`;
}
function makeInjector(injectable)
{
return (...args) => injectCode(stringifyFunctionCall(injectable, ...args));
}
function fooSnippet()
{
// snippets code that should run in document context
}
exports.fooSnippet = makeInjector(fooSnippet);
So I just think that some snippets will be entirely exported via the
makeInjector wrapper. In other cases, there may be some code to run in the
content script context and some more to run in the document context. Let's see
how this plays out.
By the way I'll get to this patch once the core patches have landed.
On 2018/07/12 11:51:48, Manish Jethani wrote: > Yes, I already ran into this issue when implementing some snippets for testing. > Here's what I came up with: > > [...] > > So I just think that some snippets will be entirely exported via the > makeInjector wrapper. In other cases, there may be some code to run in the > content script context and some more to run in the document context. Let's see > how this plays out. > > By the way I'll get to this patch once the core patches have landed. So you plan on providing an API to snippets, that they may use to inject all/some/none of their code into the page context with minimal boilerplate? That will work too, I guess.
On 2018/07/12 13:40:33, Sebastian Noack wrote: > On 2018/07/12 11:51:48, Manish Jethani wrote: > > Yes, I already ran into this issue when implementing some snippets for > testing. > > Here's what I came up with: > > > > [...] > > > > So I just think that some snippets will be entirely exported via the > > makeInjector wrapper. In other cases, there may be some code to run in the > > content script context and some more to run in the document context. Let's see > > how this plays out. > > > > By the way I'll get to this patch once the core patches have landed. > > So you plan on providing an API to snippets, that they may use to inject > all/some/none of their code into the page context with minimal boilerplate? That > will work too, I guess. Yep. https://codereview.adblockplus.org/29829569/ I'll reply to the other comments here once the main core patches have landed.
Patch Set 17: Rebase, use snippets library from core Patch Set 18: Update adblockpluscore dependency Patch Set 19: Merge elemhide and snippets https://codereview.adblockplus.org/29737561/diff/29787589/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29737561/diff/29787589/include.preload.js#... include.preload.js:595: browser.runtime.sendMessage({type: "snippets.executeScripts"}); On 2018/05/23 12:32:18, Sebastian Noack wrote: > On 2018/05/23 05:12:45, Manish Jethani wrote: > > Send message to background page to execute any scripts. > > How about using a combined message for element hiding and snippets, keeping the > amount of messaging between content script and background page minimal? Done. The ElemHide class is now renamed to ContentFiltering, and the elemhide.getSelectors message is now renamed to content.applyFilters. https://codereview.adblockplus.org/29737561/diff/29787589/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29787589/lib/scriptInjection... lib/scriptInjection.js:207: RegExpFilter.typeMap.DOCUMENT)) On 2018/07/10 14:52:00, kzar wrote: > On 2018/05/23 05:12:46, Manish Jethani wrote: > > Note: I'm not checking for ELEMHIDE here. I don't think ELEMHIDE should apply > to > > snippets. I'm not sure what we should do here, but applying ELEMHIDE seems > > wrong. > > So would a #@# exception filter be honoured? Yes, in the initial version #@# will not work. https://codereview.adblockplus.org/29737561/diff/29787589/lib/scriptInjection... lib/scriptInjection.js:215: }); On 2018/07/10 14:52:00, kzar wrote: > On 2018/05/23 05:12:46, Manish Jethani wrote: > > Note: No DevTools logging, I'll get it to eventually. > > Please could you add this as a known limitation to the issue description? I do intend to add DevTools logging in the initial version, but if I don't manage to add it (in a separate patch) then I'll mention this in #6782. https://codereview.adblockplus.org/29737561/diff/29787589/lib/scriptInjection... lib/scriptInjection.js:267: fetch(browser.extension.getURL("/snippets.js"), {cache: "no-cache"}) On 2018/07/10 14:52:00, kzar wrote: > No need to fetch this script, and to add the mapping in the metadata, we could > do something like let snippetLibraries = require("../snippets");` right? We actually need this in text form (not as a JavaScript object), as per the design. The idea is that any snippet library will be loaded in a "sandbox", which is basically a Function object (not much in the way of security) for now.
Patch Set 20: Remove elemhideWhitelisted temporary variable
Patch Set 21: Rename ContentFiltering to Content
https://codereview.adblockplus.org/29737561/diff/29787589/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29787589/lib/scriptInjection... lib/scriptInjection.js:215: }); On 2018/07/19 01:00:53, Manish Jethani wrote: > On 2018/07/10 14:52:00, kzar wrote: > > On 2018/05/23 05:12:46, Manish Jethani wrote: > > > Note: No DevTools logging, I'll get it to eventually. > > > > Please could you add this as a known limitation to the issue description? > > I do intend to add DevTools logging in the initial version, but if I don't > manage to add it (in a separate patch) then I'll mention this in #6782. Could you mention it in #6782, along with the #@# thing? You can remove them again if you manage to work around. Also the issue has barely any details in general, would you mind fleshing it out so I can triage? That would help me review this code as well. For example, I wouldn't have had to clarify about #@# if it was already mentioned in the description. https://codereview.adblockplus.org/29737561/diff/29787589/lib/scriptInjection... lib/scriptInjection.js:267: fetch(browser.extension.getURL("/snippets.js"), {cache: "no-cache"}) On 2018/07/19 01:00:53, Manish Jethani wrote: > On 2018/07/10 14:52:00, kzar wrote: > > No need to fetch this script, and to add the mapping in the metadata, we could > > do something like let snippetLibraries = require("../snippets");` right? > > We actually need this in text form (not as a JavaScript object), as per the > design. The idea is that any snippet library will be loaded in a "sandbox", > which is basically a Function object (not much in the way of security) for now. Acknowledged. https://codereview.adblockplus.org/29737561/diff/29833620/dependencies File dependencies (right): https://codereview.adblockplus.org/29737561/diff/29833620/dependencies#newcode4 dependencies:4: adblockpluscore = adblockpluscore hg:8e466288a0c3 git:f289a76 The issue doesn't talk about this dependency update at all. Please could you update the issue description, if the change belongs here. Or, if the change doesn't belong here, please could you leave an inline comment at least?
I'll get back after updating #6539 and #6782 with all the details.
> I'll get back after updating #6539 and #6782 with all the details. Thanks, in the mean time I've made a start. https://codereview.adblockplus.org/29737561/diff/29833613/composer.postload.js File composer.postload.js (right): https://codereview.adblockplus.org/29737561/diff/29833613/composer.postload.j... composer.postload.js:23: const {checkCollapse, contentFiltering, getURLsFromElement, typeMap} = window; How come you changed `contentFiltering` to `content`? To me the former is clearer, "content" could mean the content of the page or similar. https://codereview.adblockplus.org/29737561/diff/29833620/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29737561/diff/29833620/include.preload.js#... include.preload.js:566: message.filterTypes = filterTypes; Nit: We could assign the `undefined` instead? browser.runtime.sendMessage({type: "content.applyFilters", filterTypes}, response => https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:18: /** @module scriptInjection */ Be careful that Mercurial knows you've renamed lib/cssInjection.js to lib/scriptInjection.js. Otherwise, the history gets lost - yes I've done that before! https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:18: /** @module scriptInjection */ Like I mentioned on one of the other reviews, I don't feel like a CSS selector / rule is a "script". So, maybe we can come up with a name which makes sense for both snippet scripts and CSS rules? https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:199: catch (error) So we re-throw the exception, catch it again, then ignore it? https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:211: let {elemhide = false, snippets = false} = message.filterTypes || Is this syntax supported by the browsers we support? I've not seen it used before. In fact, the `= false` parts seems redundant, it'll end up as `undefined` which is falsey anyway. Wouldn't this work the same? let {elemhide, snippets} = message.filterTypes || {elemhide: true, snippets: true}; https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:226: RegExpFilter.typeMap.ELEMHIDE)) It kind of sucks we have to call checkWhitelisted an extra time, but I don't see an alternative if #@# exceptions don't apply to snippet filters. I wonder if we could tweak the logic, so that the extra check is only made if `snippets` is truthy? https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:228: let specificOnly = checkWhitelisted(sender.page, sender.frame, null, If we don't check for generichide/genericblock for snippet filters, do we at least ensure that snippet filters have a domain when they're created? https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:263: fetch(browser.extension.getURL("/snippets.js"), {cache: "no-cache"}) What if fetch throws? https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:264: .then(response => response.ok ? response.text() : "") Nit: We normally put some indentation before the `.then`. https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:265: .then(text => Why not just do the assignment in the previous function? .then(response => { if (response.ok) snippetsLibrarySource = response.text(); }); (That way I also avoid assigning it on failure, I figure it's better to keep the old version rather than trashing it.
https://codereview.adblockplus.org/29737561/diff/29833613/composer.postload.js File composer.postload.js (right): https://codereview.adblockplus.org/29737561/diff/29833613/composer.postload.j... composer.postload.js:23: const {checkCollapse, contentFiltering, getURLsFromElement, typeMap} = window; On 2018/07/19 12:24:17, kzar wrote: > How come you changed `contentFiltering` to `content`? To me the former is > clearer, "content" could mean the content of the page or similar. I was finding that name ugly but I wasn't too sure. If you think contentFiltering is an appropriate name, I'm happy to go with it. https://codereview.adblockplus.org/29737561/diff/29833620/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29737561/diff/29833620/include.preload.js#... include.preload.js:566: message.filterTypes = filterTypes; On 2018/07/19 12:24:18, kzar wrote: > Nit: We could assign the `undefined` instead? > > browser.runtime.sendMessage({type: "content.applyFilters", filterTypes}, > response => Yes, we could just pass undefined, I was just trying to avoid adding an additional property to the message payload for the most common case. I guess it doesn't matter too much, I'll change it. https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:18: /** @module scriptInjection */ On 2018/07/19 12:24:19, kzar wrote: > Be careful that Mercurial knows you've renamed lib/cssInjection.js to > lib/scriptInjection.js. Otherwise, the history gets lost - yes I've done that > before! Thanks, I'll try to do this correctly. https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:18: /** @module scriptInjection */ On 2018/07/19 12:24:20, kzar wrote: > Like I mentioned on one of the other reviews, I don't feel like a CSS selector / > rule is a "script". So, maybe we can come up with a name which makes sense for > both snippet scripts and CSS rules? Fair enough. Since we're referring to it as "content filtering" in include.preload.js, how about lib/contentFiltering.js? https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:199: catch (error) On 2018/07/19 12:24:21, kzar wrote: > So we re-throw the exception, catch it again, then ignore it? No, the error thrown from the .catch() handler is asynchronous, it doesn't appear here. We rethrow that so it appears in the console so we know what's going on. I added the .catch() handler not to ignore all errors but only to ignore the one error that happens too often and doesn't mean anything to us. If you think that we should instead ignore all errors over there, then that would be a different thing. I personally think it's better to see the errors in the console, it doesn't mean anything to the user and is only for developers. https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:211: let {elemhide = false, snippets = false} = message.filterTypes || On 2018/07/19 12:24:19, kzar wrote: > Is this syntax supported by the browsers we support? I've not seen it used > before. Yeah, that's just object destructuring with default values. It's quite old, I'm pretty sure it's supported by all our platforms. > In fact, the `= false` parts seems redundant, it'll end up as `undefined` which > is falsey anyway. Wouldn't this work the same? > > let {elemhide, snippets} = message.filterTypes || {elemhide: true, snippets: > true}; Yeah, it would be the same. I guess I can remove the default assignments. Let me do that in the next update. https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:226: RegExpFilter.typeMap.ELEMHIDE)) On 2018/07/19 12:24:20, kzar wrote: > It kind of sucks we have to call checkWhitelisted an extra time, but I don't see > an alternative if #@# exceptions don't apply to snippet filters. #@# exception will be applied within Snippets.getScriptsForDomain already so that's not the issue. The issue is that $elemhide doesn't not apply to snippet filters. I'm not even sure how this is going to go, but I don't think we need an equivalent of $elemhide for snippets. > I wonder if we could tweak the logic, so that the extra check is only made if > `snippets` is truthy? The only time snippets will be falsy is when this is called from the composer. I don't think it's worth complicating the code for that case alone. What do you think? https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:228: let specificOnly = checkWhitelisted(sender.page, sender.frame, null, On 2018/07/19 12:24:18, kzar wrote: > If we don't check for generichide/genericblock for snippet filters, do we at > least ensure that snippet filters have a domain when they're created? Currently there are no such checks. I'll mention this in the issue. I don't think we need an equivalent of $generichide for snippets, we already have $document if we want to whitelist entirely. But I suppose requiring domains in a snippet filter is a good idea. https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:263: fetch(browser.extension.getURL("/snippets.js"), {cache: "no-cache"}) On 2018/07/19 12:24:20, kzar wrote: > What if fetch throws? I don't see how it could throw in practice if the file exists, but in the worst case it'll log to the console and there won't be any snippets available (it fails gracefully). https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:264: .then(response => response.ok ? response.text() : "") On 2018/07/19 12:24:21, kzar wrote: > Nit: We normally put some indentation before the `.then`. I thought when we break foo.bar.lambda() then (1) the continuation should start with a dot and (2) it should be aligned with the start of the expression. foo.bar .lambda() Rather than: foo.bar .lambda() This is pretty much how it's done in other projects. I couldn't find any reference to any such rule in our style guide though. https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:265: .then(text => On 2018/07/19 12:24:20, kzar wrote: > Why not just do the assignment in the previous function? > > .then(response => > { > if (response.ok) > snippetsLibrarySource = response.text(); > }); > > (That way I also avoid assigning it on failure, I figure it's better to keep the > old version rather than trashing it. response.text() returns a promise, which then resolves with the actual text, asynchronously. By the way, async/await is going to make these things so much easier to write and maintain.
https://codereview.adblockplus.org/29737561/diff/29833613/composer.postload.js File composer.postload.js (right): https://codereview.adblockplus.org/29737561/diff/29833613/composer.postload.j... composer.postload.js:23: const {checkCollapse, contentFiltering, getURLsFromElement, typeMap} = window; On 2018/07/19 12:55:23, Manish Jethani wrote: > On 2018/07/19 12:24:17, kzar wrote: > > How come you changed `contentFiltering` to `content`? To me the former is > > clearer, "content" could mean the content of the page or similar. > > I was finding that name ugly but I wasn't too sure. If you think > contentFiltering is an appropriate name, I'm happy to go with it. Well, it is kind of ugly I suppose, but then "content" seems kind of misleading. On balance I prefer "contentFiltering", but I guess since both names suck I don't feel too strongly. I don't have any better suggestions anyway. https://codereview.adblockplus.org/29737561/diff/29833620/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29737561/diff/29833620/include.preload.js#... include.preload.js:566: message.filterTypes = filterTypes; On 2018/07/19 12:55:23, Manish Jethani wrote: > On 2018/07/19 12:24:18, kzar wrote: > > Nit: We could assign the `undefined` instead? > > > > browser.runtime.sendMessage({type: "content.applyFilters", filterTypes}, > > response => > > Yes, we could just pass undefined, I was just trying to avoid adding an > additional property to the message payload for the most common case. I guess it > doesn't matter too much, I'll change it. Thanks, but looks like you didn't push that change? https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:18: /** @module scriptInjection */ On 2018/07/19 12:24:20, kzar wrote: > Like I mentioned on one of the other reviews, I don't feel like a CSS selector / > rule is a "script". So, maybe we can come up with a name which makes sense for > both snippet scripts and CSS rules? Sounds good. https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:199: catch (error) On 2018/07/19 12:55:24, Manish Jethani wrote: > On 2018/07/19 12:24:21, kzar wrote: > > So we re-throw the exception, catch it again, then ignore it? > > No, the error thrown from the .catch() handler is asynchronous, it doesn't > appear here. We rethrow that so it appears in the console so we know what's > going on. OK. > I added the .catch() handler not to ignore all errors but only to ignore the one > error that happens too often and doesn't mean anything to us. Which one's that? > I personally think it's better to see the errors in the console, it > doesn't mean anything to the user and is only for developers. Yea me too, I'm generally against hiding exceptions unless there's a good reason to do so. https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:226: RegExpFilter.typeMap.ELEMHIDE)) On 2018/07/19 12:55:24, Manish Jethani wrote: > > I wonder if we could tweak the logic, so that the extra check is only made if > > `snippets` is truthy? > > The only time snippets will be falsy is when this is called from the composer. I > don't think it's worth complicating the code for that case alone. What do you > think? Alright, fair enough I guess. https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:228: let specificOnly = checkWhitelisted(sender.page, sender.frame, null, On 2018/07/19 12:55:25, Manish Jethani wrote: > But I suppose requiring domains in a snippet filter is a good idea. Yea, I definitely think so. Please could you open an issue for that? https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:263: fetch(browser.extension.getURL("/snippets.js"), {cache: "no-cache"}) On 2018/07/19 12:55:24, Manish Jethani wrote: > On 2018/07/19 12:24:20, kzar wrote: > > What if fetch throws? > > I don't see how it could throw in practice if the file exists, but in the worst > case it'll log to the console and there won't be any snippets available (it > fails gracefully). Acknowledged. https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:264: .then(response => response.ok ? response.text() : "") On 2018/07/19 12:55:25, Manish Jethani wrote: > On 2018/07/19 12:24:21, kzar wrote: > > Nit: We normally put some indentation before the `.then`. > > I thought when we... Well, with your example it would be more like this: foo.bar .lambda() But who cares? Not me to be honest. > This is pretty much how it's done in other projects. I couldn't find > any reference to any such rule in our style guide though. There isn't one. https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:265: .then(text => On 2018/07/19 12:55:26, Manish Jethani wrote: > response.text() returns a promise... Ah right, the indirection makes sense then, fair enough.
Patch Set 22: Remove dependency update Patch Set 23: Clean up messaging for content.applyFilters Patch Set 24: Rename Content to ContentFiltering Patch Set 25: Rename lib/scriptInjection.js to lib/contentFiltering.js https://codereview.adblockplus.org/29737561/diff/29833613/composer.postload.js File composer.postload.js (right): https://codereview.adblockplus.org/29737561/diff/29833613/composer.postload.j... composer.postload.js:23: const {checkCollapse, contentFiltering, getURLsFromElement, typeMap} = window; On 2018/07/19 13:28:53, kzar wrote: > On 2018/07/19 12:55:23, Manish Jethani wrote: > > On 2018/07/19 12:24:17, kzar wrote: > > > How come you changed `contentFiltering` to `content`? To me the former is > > > clearer, "content" could mean the content of the page or similar. > > > > I was finding that name ugly but I wasn't too sure. If you think > > contentFiltering is an appropriate name, I'm happy to go with it. > > Well, it is kind of ugly I suppose, but then "content" seems kind of misleading. > On balance I prefer "contentFiltering", but I guess since both names suck I > don't feel too strongly. I don't have any better suggestions anyway. Renamed to contentFiltering now. Done. https://codereview.adblockplus.org/29737561/diff/29833620/dependencies File dependencies (right): https://codereview.adblockplus.org/29737561/diff/29833620/dependencies#newcode4 dependencies:4: adblockpluscore = adblockpluscore hg:8e466288a0c3 git:f289a76 On 2018/07/19 10:37:12, kzar wrote: > The issue doesn't talk about this dependency update at all. Please could you > update the issue description, if the change belongs here. Or, if the change > doesn't belong here, please could you leave an inline comment at least? This is covered by #6784 now, which I have set as a dependency for #6782. Done. https://codereview.adblockplus.org/29737561/diff/29833620/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29737561/diff/29833620/include.preload.js#... include.preload.js:566: message.filterTypes = filterTypes; On 2018/07/19 13:28:54, kzar wrote: > On 2018/07/19 12:55:23, Manish Jethani wrote: > > On 2018/07/19 12:24:18, kzar wrote: > > > Nit: We could assign the `undefined` instead? > > > > > > browser.runtime.sendMessage({type: "content.applyFilters", filterTypes}, > > > response => > > > > Yes, we could just pass undefined, I was just trying to avoid adding an > > additional property to the message payload for the most common case. I guess > it > > doesn't matter too much, I'll change it. > > Thanks, but looks like you didn't push that change? Done. https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:18: /** @module scriptInjection */ On 2018/07/19 13:28:55, kzar wrote: > On 2018/07/19 12:24:20, kzar wrote: > > Like I mentioned on one of the other reviews, I don't feel like a CSS selector > / > > rule is a "script". So, maybe we can come up with a name which makes sense for > > both snippet scripts and CSS rules? > > Sounds good. Done. https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:199: catch (error) On 2018/07/19 13:28:55, kzar wrote: > On 2018/07/19 12:55:24, Manish Jethani wrote: > > On 2018/07/19 12:24:21, kzar wrote: > > > So we re-throw the exception, catch it again, then ignore it? > > > > No, the error thrown from the .catch() handler is asynchronous, it doesn't > > appear here. We rethrow that so it appears in the console so we know what's > > going on. > > OK. > > > I added the .catch() handler not to ignore all errors but only to ignore the > one > > error that happens too often and doesn't mean anything to us. > > Which one's that? It's the "The frame was removed." error. > [...] https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:211: let {elemhide = false, snippets = false} = message.filterTypes || On 2018/07/19 12:55:25, Manish Jethani wrote: > On 2018/07/19 12:24:19, kzar wrote: > > Is this syntax supported by the browsers we support? I've not seen it used > > before. > > Yeah, that's just object destructuring with default values. It's quite old, I'm > pretty sure it's supported by all our platforms. > > > In fact, the `= false` parts seems redundant, it'll end up as `undefined` > which > > is falsey anyway. Wouldn't this work the same? > > > > let {elemhide, snippets} = message.filterTypes || {elemhide: true, snippets: > > true}; > > Yeah, it would be the same. I guess I can remove the default assignments. Let me > do that in the next update. Done. https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:228: let specificOnly = checkWhitelisted(sender.page, sender.frame, null, On 2018/07/19 13:28:54, kzar wrote: > On 2018/07/19 12:55:25, Manish Jethani wrote: > > But I suppose requiring domains in a snippet filter is a good idea. > > Yea, I definitely think so. Please could you open an issue for that? Done. https://issues.adblockplus.org/ticket/6797
Patch Set 26: Fix formatting for content.applyFilters message
https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:199: catch (error) On 2018/07/19 14:04:34, Manish Jethani wrote: > It's the "The frame was removed." error. That's the above catch. I thought we were talking about this one (line 199). I guess I still don't understand what it's for. https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:228: let specificOnly = checkWhitelisted(sender.page, sender.frame, null, On 2018/07/19 14:04:35, Manish Jethani wrote: > On 2018/07/19 13:28:54, kzar wrote: > > On 2018/07/19 12:55:25, Manish Jethani wrote: > > > But I suppose requiring domains in a snippet filter is a good idea. > > > > Yea, I definitely think so. Please could you open an issue for that? > > Done. > > https://issues.adblockplus.org/ticket/6797 Thanks
https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:199: catch (error) On 2018/07/19 14:52:34, kzar wrote: > On 2018/07/19 14:04:34, Manish Jethani wrote: > > It's the "The frame was removed." error. > > That's the above catch. I thought we were talking about this one (line 199). I > guess I still don't understand what it's for. Ah, I see what you mean. Yeah, I probably just added that catch as a precaution, I can remove it and see what happens. But we will have to have a general catch-all-and-ignore somewhere because we still want the rest of the code to proceed even if the call to executeCode fails. What I don't like about it here is that we're just ignoring the error, not even logging it (so we don't know what happened).
https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:199: catch (error) On 2018/07/19 15:33:38, Manish Jethani wrote: > But we will have to have a general catch-all-and-ignore somewhere > because we still want the rest of the code to proceed even if the > call to executeCode fails. What I don't like about it here is that > we're just ignoring the error, not even logging it (so we don't know > what happened). Yea, ideally there would be an entry in the developer tools that a snippet attempted to execute for a website, but that there was an exception. But perhaps, for now, we should note this as a limitation and simply log the exception to the console instead? We could prepend the message with some more information about the snippet and the arguments passed to it.
https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:199: catch (error) On 2018/07/19 15:37:45, kzar wrote: > On 2018/07/19 15:33:38, Manish Jethani wrote: > > But we will have to have a general catch-all-and-ignore somewhere > > because we still want the rest of the code to proceed even if the > > call to executeCode fails. What I don't like about it here is that > > we're just ignoring the error, not even logging it (so we don't know > > what happened). > > Yea, ideally there would be an entry in the developer tools that a snippet > attempted to execute for a website, but that there was an exception. But > perhaps, for now, we should note this as a limitation and simply log the > exception to the console instead? We could prepend the message with some more > information about the snippet and the arguments passed to it. Can we simply log the error? catch (error) { console.error(error); } Let me try a snippet that throws an error and see what happens. I'll get back.
Patch Set 27: Add explanatory comment in catch block https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:199: catch (error) On 2018/07/19 15:44:05, Manish Jethani wrote: > On 2018/07/19 15:37:45, kzar wrote: > > On 2018/07/19 15:33:38, Manish Jethani wrote: > > > But we will have to have a general catch-all-and-ignore somewhere > > > because we still want the rest of the code to proceed even if the > > > call to executeCode fails. What I don't like about it here is that > > > we're just ignoring the error, not even logging it (so we don't know > > > what happened). > > > > Yea, ideally there would be an entry in the developer tools that a snippet > > attempted to execute for a website, but that there was an exception. But > > perhaps, for now, we should note this as a limitation and simply log the > > exception to the console instead? We could prepend the message with some more > > information about the snippet and the arguments passed to it. > > Can we simply log the error? > > catch (error) > { > console.error(error); > } > > Let me try a snippet that throws an error and see what happens. I'll get back. OK, so when a snippet throws an error that error is logged independently, because of course the snippet runs as a its own content script. I have added a comment here explaining why we catch and ignore all errors. We do it for the same reason we do it for tabs.insertCSS
LGTM https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection.js File lib/scriptInjection.js (right): https://codereview.adblockplus.org/29737561/diff/29833620/lib/scriptInjection... lib/scriptInjection.js:199: catch (error) On 2018/07/19 15:54:08, Manish Jethani wrote: > On 2018/07/19 15:44:05, Manish Jethani wrote: > > On 2018/07/19 15:37:45, kzar wrote: > > > On 2018/07/19 15:33:38, Manish Jethani wrote: > > > > But we will have to have a general catch-all-and-ignore somewhere > > > > because we still want the rest of the code to proceed even if the > > > > call to executeCode fails. What I don't like about it here is that > > > > we're just ignoring the error, not even logging it (so we don't know > > > > what happened). > > > > > > Yea, ideally there would be an entry in the developer tools that a snippet > > > attempted to execute for a website, but that there was an exception. But > > > perhaps, for now, we should note this as a limitation and simply log the > > > exception to the console instead? We could prepend the message with some > more > > > information about the snippet and the arguments passed to it. > > > > Can we simply log the error? > > > > catch (error) > > { > > console.error(error); > > } > > > > Let me try a snippet that throws an error and see what happens. I'll get back. > > OK, so when a snippet throws an error that error is logged independently, > because of course the snippet runs as a its own content script. > > I have added a comment here explaining why we catch and ignore all errors. We do > it for the same reason we do it for tabs.insertCSS Alright, fair enough.
On 2018/07/19 16:07:41, kzar wrote: > LGTM Thanks! I'll commit this after patch #29833597. Meanwhile I'll spend all of tomorrow updating all the issues to bring them up to date with all the information about the current implementation in both core and the web extension (#6538, #6539, #6781, #6782, and possibly others). 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
