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

VERIFIED FIXED in Firefox 57

Status

defect
P1
normal
VERIFIED FIXED
2 years ago
10 months ago

People

(Reporter: hub, Assigned: zombie)

Tracking

(Blocks 2 bugs)

unspecified
mozilla57
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified)

Details

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
This happen in combination to bug 1369841 which is about the background script.

Comment 2

2 years ago
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.

Comment 3

2 years ago
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).

Updated

2 years ago
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
(Assignee)

Comment 4

2 years ago
This is happening because the script reading and "compilation" is done async, so it may end up

Comment 5

2 years ago
For what it's worth, Chrome seems to avoid this problem entirely by not running the content scripts at all upon installation.

Comment 6

2 years ago
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!".

Updated

2 years ago
Priority: -- → P1

Updated

2 years ago
Assignee: nobody → tomica
(Assignee)

Updated

2 years ago
Attachment #8906383 - Flags: review?(kmaglione+bmo)

Comment 8

2 years ago
mozreview-review
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+
(Assignee)

Comment 9

2 years ago
mozreview-review-reply
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 ;)
Comment hidden (mozreview-request)

Comment 11

2 years ago
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
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Comment 13

a year ago
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

Updated

10 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.