Closed Bug 1395287 Opened 3 years ago Closed 3 years ago

document_end content scripts run before document_start content scripts when extension starts up

Categories

(WebExtensions :: Untriaged, defect, P1)

defect

Tracking

(firefox57 verified)

VERIFIED FIXED
mozilla57
Tracking Status
firefox57 --- verified

People

(Reporter: hub, Assigned: zombie)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

This is with Nightly 2017-08-30.

In some circumstances, content_script "document_end" runs after the content_script "document_start". For us this mean that some of the things initialised in the document_start aren't when the document_end is run.

This happen often when installing or reloading the add-on.

I'm trying to come up with a simplified reproducible test case.
This happen in combination to bug 1369841 which is about the background script.
In the description, it should say: "document_end content script runs *before* document_start content script" of course. This is an issue because document_end scripts can usually rely on variables defined by document_start scripts.
I was able to reproduce the issue reliably in Firefox 57.0a1 nightly (2017-08-30). Steps to reproduce:

1. Download attached extension.
2. Make sure exactly one website is loaded, e.g. http://example.com/
3. Open about:debugging in a new tab, click "Load Temporary Add-on" and select the downloaded extension.
4. Press Ctrl-Shift-J (Cmd-Shift-J on Mac) to open Browser Console and inspect the messages.

Expected results:

You first get the messages "Initializing content.js" and "Finished initializing content.js" (produced by the document_start content script), then "Initializing content-post.js" and "Finished initializing content-post.js" (produced by the document_end content script). The document_end script can always rely on variables defined by the document_start script. This is the behavior seen in Chrome.

Actual results:

The messages "Initializing content-post.js" and "Finished initializing content-post.js" got first, so the document_end content script loads before the document_start content script. There is an error message "content.js not yet initialized!" because required variable is missing. The order seems to be determined by I/O delays, for me it only reproduces reliably if I make the document_start script very large (73 kB, mostly padding comments).
Summary: content_script "document_end" runs after the content_script "document_start" → document_end content scripts run before document_start content scripts when extension starts up
This is happening because the script reading and "compilation" is done async, so it may end up
For what it's worth, Chrome seems to avoid this problem entirely by not running the content scripts at all upon installation.
I should add that this is not a problem only on installation. It also happens on browser startup. To see this behavior on startup, set your browser's home page to https://mozilla.org, then restart the browser and look at the messages in the console. You'll see the error "content.js not yet initialized!".
Priority: -- → P1
Assignee: nobody → tomica
Attachment #8906383 - Flags: review?(kmaglione+bmo)
Comment on attachment 8906383 [details]
Bug 1395287 - Order and chain content_scripts injection on startup

https://reviewboard.mozilla.org/r/178096/#review183038

Nice. Thanks!

::: toolkit/components/extensions/extension-process-script.js:204
(Diff revision 1)
> -          contentScripts.get(script).injectInto(window);
> +          runAt[script.runAt].push(script);
>          }
>        }
> +
> +      let inject = matcher => contentScripts.get(matcher).injectInto(window);
> +      let allDone = matchers => Promise.all(matchers.map(inject));

Nit: Maybe call this something like `injectAll`? `allDone` sounds like we're just waiting for scripts that are already injected to finish, which wouldn't make sense here.

::: toolkit/components/extensions/extension-process-script.js:208
(Diff revision 1)
> +      allDone(runAt.document_start)
> +        .then(() => allDone(runAt.document_end))
> +        .then(() => allDone(runAt.document_idle));

I'm fine with this, but you could also do something like:

    (async () => {
      await injectAll(document_start);
      await injectAll(document_end);
      await injectAll(document_idle);
    })();
    
Not that the last `await` is strictly necessary. It's probably also not strictly necessary to wait for `document_end` scripts to finish before running the `document_idle` ones, but it probably doesn't matter much either way.
Attachment #8906383 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8906383 [details]
Bug 1395287 - Order and chain content_scripts injection on startup

https://reviewboard.mozilla.org/r/178096/#review183038

> I'm fine with this, but you could also do something like:
> 
>     (async () => {
>       await injectAll(document_start);
>       await injectAll(document_end);
>       await injectAll(document_idle);
>     })();
>     
> Not that the last `await` is strictly necessary. It's probably also not strictly necessary to wait for `document_end` scripts to finish before running the `document_idle` ones, but it probably doesn't matter much either way.

Fair, though I like the explicitness of `.then` where blocking makes a difference (`await` is too easy to ignore and pretend it's not there ;)
Pushed by tomica@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ce17d1d232f1
Order and chain content_scripts injection on startup r=kmag
https://hg.mozilla.org/mozilla-central/rev/ce17d1d232f1
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Verified as fixed in Build identifier: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0 with steps from Comment #3. Please see postfix screenshot attached.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.