[webextension] Error in include.postload.js

[hfiguiere]

Environment

Firefox Nightly
Adblock Plus web-extension master for Firefox

How to reproduce

1. Load the add-on
2. Debug the add-on using the add-on debugging support in Firefox
3. Look at the console in the debugger.

Observed behaviour

ReferenceError: ext is not defined include.postload.js:609:3

Expected behaviour

No error

Note

Doesn't happen in Chrome.

[mjethani]

I'm pretty sure that both the scripts run in the same context, so if it's defined in the preload script it should be defined in the postload script as well.

Are you trying this out on master?

ext can be undefined if there is some other error during initialization. Are you seeing any other errors in the background page's console? What about the console of the web page?

[hfiguiere]

This is master. Revision 3c423eeb5b89.

The first error in the console when reloading the add-on is the one mentioned. It is repeated 14 times in my case.

Indeed MDN says:

There is only one global scope per frame per extension, so variables from one content script can directly be accessed by another content script, regardless of how the content script was loaded.

[mjethani]

I see, this is interesting. I added a couple of log statements to see what's going on. Firefox sometimes runs the postload script before the preload script. This appears to happen only on installation. It also doesn't happen always.

There is already a ​bug about Firefox's behavior of running the content scripts before loading the background page. We should report another one.

The workaround may be to check for a variable declared in the preload script and exit if it is undefined. At the end of include.preload.js we could set window.preloadCompleted = true and check for this in the postload script; if it is not true, skip the postload stuff.

[kzar]

Note this could be a security issue since if the page contains an element with the id of ext then typeof ext will no longer be undefined. We've always assumed the preload scripts will run before the postload ones. ​See this great writeup.

[mjethani]

An example extension

[mjethani]

I've attached a standalone extension where you can see this problem. Let's report this to Mozilla. Hubert, will you do it? I can do it otherwise.

[mjethani]

Replying to kzar:

Note this could be a security issue since if the page contains an element with the id of ext then typeof ext will no longer be undefined. We've always assumed the preload scripts will run before the postload ones. ​See this great writeup.

That's a very good point.

Hmm ... it should be sufficient to check for window.preloadCompleted === true (strict equality).

[hfiguiere]

After the error about ext I get a lot of

Error: Could not establish connection. Receiving end does not exist. (unknown)

Like described at ​https://bugzilla.mozilla.org/show_bug.cgi?id=1369841#c2 (the Mozilla bug mentioned above).

[hfiguiere]

Replying to mjethani:

I've attached a standalone extension where you can see this problem. Let's report this to Mozilla. Hubert, will you do it? I can do it otherwise.

As I am filing a bug for Mozilla, and I can't reproduce the problem with your sample extension. :-/

[mjethani]

Replying to hfiguiere:

Replying to mjethani:

I've attached a standalone extension where you can see this problem. Let's report this to Mozilla. Hubert, will you do it? I can do it otherwise.

As I am filing a bug for Mozilla, and I can't reproduce the problem with your sample extension. :-/

I found that it doesn't happen all the time. You have to try a few times to reproduce it. In the web page console you should see the error message.

Yeah, you have to keep one web page open in the browser so it runs the content scripts on installation.

[trev]

It's best to clean up manifest.json in this example extension before filing the bug, it has lots of unnecessary details - ideally, it should really be a minimal extension.

[hfiguiere]

I filed this Mozilla issue ​https://bugzilla.mozilla.org/show_bug.cgi?id=1395287

Haven't attached the reproducible test case yet.

[trev]

Minimal extension

[trev]

I attached the extension with minimized manifest. However, I am also unable to reproduce this issue, neither in Nightly nor in Firefox 55. Tried reloading the extension ten times, content scripts always load in correct order.

[trev]

Minimal extension

[trev]

It seems that I guessed correctly and the issue is I/O delay here. In example3.xpi I made content.js very large (added padding comments) and now I can reproduce the issue reliably.

[trev]

I attached the extension to ​https://bugzilla.mozilla.org/show_bug.cgi?id=1395287 and added steps to reproduce.

[trev]

As to what we can do about this, I see two options:

1. Make the content scripts self-contained. This probably means turning them and their dependencies (ext in particular) into modules that are compiled together via webpack (there will be two collections of modules executing in the same scope, our current approach doesn't support that). This will mean some code duplication of course, but it is only 3 kB already, less if we reduce the ext part as we plan to do anyway.
2. In composer.postload.js, check whether "ext" is defined (make sure to consider the scenario mentioned in comment:6). If it isn't, delay initialization.

While I think that the former is the proper solution, we probably want to implement the latter as a short-term solution.

[trev]

Replying to kzar:

Note this could be a security issue since if the page contains an element with the id of ext then typeof ext will no longer be undefined.

We should be able to avoid hitting this by adding var ext; (not let ext;) to composer.postload.js. If the variable is defined, the fallback won't be triggered. And if ext is already defined, this statement will have not effect

[sebastian]

Note that it is not only ext but also ​some functions/variables from include.preload.js which include.postload.js relies on. Having the document_end script load and run all of that code again seems like a bad idea to me, as this will add up to the memory overhead per docunment, not to mention side effects if any of the shared logic relies on a global state.

[mjethani]

It seems to me that at_document_end is for content scripts that are independent of content scripts executed at at_document_start. Even if that is not the case, as of now it seems we have a problem assuming that they'll run in sequence. How about running all the stuff in include.postload.js after the document has been loaded, i.e. in the document's load handler? Then we'll have only one set of scripts executed at at_document_start, and they'll set up their event handlers as necessary. It certainly reduces the reliance on browsers doing the right thing.

[sebastian]

Replying to mjethani:

It seems to me that at_document_end is for content scripts that are independent of content scripts executed at at_document_start.

That is not the case. document_start is for content scripts that are required to run early (i.e. before the browser switches to the new document and/or before any of the page's scripts run). document_end is for content scripts that require the DOM to be complete or are just not urgent enough to block the renderer while the document is still loading. Naturally extensions can have different features that have different requirements, and naturally there might be some shared code. If Firefox isn't guaranteeing the order of content scripts, its obviously a bug on their end.

Even if that is not the case, as of now it seems we have a problem assuming that they'll run in sequence. How about running all the stuff in include.postload.js after the document has been loaded, i.e. in the document's load handler? Then we'll have only one set of scripts executed at at_document_start, and they'll set up their event handlers as necessary. It certainly reduces the reliance on browsers doing the right thing.

For the code we have in include.preload.js and inject.preload.js, document_end is too late. Some of the code in there has to run before any of the page's scripts run, in order to avoid circumvention. Furthermore, deferring element hiding until the DOM is complete, would cause ads to be visible while the page is still loading.

However, the other way around, running all scripts at document_start would be possible (when adding some DOMContentReady listeners). However, the downside with that approach would be that it will potentially slow down page load a bit, as there will be more code to load/run early. But if we can limit this workaround to the Firefox builds, this might be acceptable.

[mjethani]

Replying to sebastian:

However, the other way around, running all scripts at document_start would be possible (when adding some DOMContentReady listeners). However, the downside with that approach would be that it will potentially slow down page load a bit, as there will be more code to load/run early. But if we can limit this workaround to the Firefox builds, this might be acceptable.

Yes, this is what I meant, but instead of DOMContentReady we should use load. The latter is dispatched after everything else, including images, stylesheets, and so on have been loaded. This should not slow down the page load then.

[sebastian]

Sorry, in fact, this is essentially what you were suggesting.

But I'm not sure about using the load event. A website can potentially remain in "interactive" state for a rather long time, possibly forever, with abp:subscribe links and the composer not working then.

I suppose some performance degradation could still occur, by loading/parsing more lines/files of code in a hot spot, even if most execution won't happen before DOMContentReady. Also it seems like a hack using document_start+DOMContentReady if there is document_end. However, we could limit that workaround by having our buildtools convert document_end scripts to document_start scripts wrapped into a DOMContentReady listener only for the gecko-webext build target.

[hfiguiere]

I have updated the patch. I have opted to not have a different code for Chrome and Firefox (Gecko) for the sake of simplicity.

It is my understanding that DOMContentReady is about the same time as the document_end content script would be run.

[mjethani]

OK, let's put this in perspective a little bit.

First of all, Firefox needs to get it together. If Chrome is able to avoid this problem, Firefox should too. These are clearly bugs in Firefox, and we can expect them to be fixed eventually, which means we can use a short-term solution/hack until such time.

Secondly, this is not the only error that occurs on installation; since the background page is unreachable from the content script, there are a number of errors.

Third, this only happens on installation of the extension and on browser startup. It's not as severe as it may seem.

We should probably just go for the simple solution of checking for the value of ext and not running that code if it's undefined.

We also have to do a similar hack for ​Mozilla bug #1369841: we're getting "response is undefined" in the elemhide.getSelectors handler, so we'll just have to check for that condition and exit from that function early.

[kzar]

Replying to mjethani:

We should probably just go for the simple solution of checking for the value of ext and not running that code if it's undefined.

Yea, a quick check along with a comment linking the Mozilla bug sounds good to me. I wonder if we should also display an alert message informing the user that they should restart Firefox?

[mjethani]

Replying to kzar:

Replying to mjethani:

We should probably just go for the simple solution of checking for the value of ext and not running that code if it's undefined.

Yea, a quick check along with a comment linking the Mozilla bug sounds good to me. I wonder if we should also display an alert message informing the user that they should restart Firefox?

They don't have to restart Firefox unless there's an error in the background page, which is not the case here at least.

[kzar]

Ah cool, nevermind then.

[hfiguiere]

​https://bugzilla.mozilla.org/show_bug.cgi?id=1395287 has been fixed for Firefox 57.

[sebastian]

Note that we still support Firefox >= 50. So we might still want to implement the workaround suggested in comment:25. On the other hand, if all this bug is causing is an error logged to the web page's console, sometimes, after the extension loaded, perhaps this isn't worth to be addressed if it is already fixed in newer Firefox versions.

[abpbot]

A commit referencing this issue has landed:
​Issue 5599 - Delay composer initialization if ext object is unavailable