1792685 - Ensure that all document_start resources have finished loading before starting document_end scripts

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P1)

Description

[Henri Sivonen (:hsivonen)]

Normally WebExtensions block the parser while injected document_start are loaded. This doesn't work with about:blank whose DOM is generated in one task.

WebExtensions should 1) not attempt to block about:blank DOM creation (in the non-initial case, there is a parser) and 2) should have a mechanism other than blocking the parser to defer the execution of document_end scripts until document_start resources have been loaded and injected.

This is relevant to bug 543435, but at least for now, I'm planning to annotate the relevant tests instead of treating this as a blocker.

Comment 1

[Henri Sivonen (:hsivonen)]

This should probably build on the document-level parser blocking promise mechanism even if it doesn't actually block a parser with about:blank:
https://searchfox.org/mozilla-central/rev/4e695325c5cfc6a37f271b98672f9c03252b1f6f/dom/base/Document.cpp#13523-13529

It would likely be bad for Web compat if the presence of extensions could alter the way load and DOMContentLoaded fire for about:blank (either initial or non-initial), so if there are document_start items, load and DOMContentLoaded should be allowed to fire on about:blank even while document_end is being deferred pending the completion of document_start items.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:rpl, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 3

[Tomislav Jovanovic :zombie]

(In reply to Henri Sivonen (:hsivonen) from comment #0)

WebExtensions should 1) not attempt to block about:blank DOM creation (in the non-initial case, there is a parser) and 2) should have a mechanism other than blocking the parser to defer the execution of document_end scripts until document_start resources have been loaded and injected.

We're not blocking the parser to defer execution of document_end content scripts, but to ensure document_start scripts run before any page scripts, because many extensions work by patching the page's view of the DOM (I believe even our webcompat and SmartBlock depend on that guarantee).

I'm not sure, but I think this (sometimes) matters even when the final url is actually "about:blank", when a page is creating an iframe and expecting to access its DOM from the outside right away, we probably want to run document_start content scripts before that.

Comment 4

[Henri Sivonen (:hsivonen)]

(In reply to Tomislav Jovanovic :zombie from comment #3)

I'm not sure, but I think this (sometimes) matters even when the final url is actually "about:blank", when a page is creating an iframe and expecting to access its DOM from the outside right away, we probably want to run document_start content scripts before that.

AFAICT, right now document_start scripts and CSS are handled asynchronously. It would be bad the Web-observable synchronicity of about:blank to depend on what WebExtensions are installed.

Does doing what you say (for about:blank that appears to materialize synchronously from the point of view of Web content) imply spinning a nested event loop, then? Those are bad, too, and have hard-to-reason-about edge cases. It would be bad if those edge cases depended on the extensions that are installed. At least with things like sync XHR and alert, the page can avoid those edge cases by staying away from those frowned-upon features. If we spun a nested event loop for WebExtension purposes, the presence of nested event loops wouldn't be a page-avoidable thing but would depend on the installed WebExtensions.

How bad would it be if we didn't provide the guarantee for WebExtension-provided scripts injected into about:blank? What does Chrome do?

Comment 5

[Tomislav Jovanovic :zombie]

I apologize for the late reply, this fell through the cracks. :/

(In reply to Henri Sivonen (:hsivonen) from comment #4)

AFAICT, right now document_start scripts and CSS are handled asynchronously. It would be bad the Web-observable synchronicity of about:blank to depend on what WebExtensions are installed.

I'm not sure, do you know what/how it is supposed to be observable by web content? In a plain web page, I can't get sync access to an iframe's DOM right after creating it in Firefox, but perhaps I'm doing it wrong?

Does doing what you say (for about:blank that appears to materialize synchronously from the point of view of Web content) imply spinning a nested event loop, then?

All we do from the extensions framework is block parsing via the code you linked above, not sure if that triggers a nested event loop somewhere.

How bad would it be if we didn't provide the guarantee for WebExtension-provided scripts injected into about:blank? What does Chrome do?

It might be bad for privacy-preserving extensions (and also our webcompat efforts). I'll need to double check, but my testing shows this is not as strong guarantee in Chromium, which is unsurprising -- we know of other years-old bugs about content scripts being delayed for perf reasons (during Chrome startup for example).

Comment 6

[Henri Sivonen (:hsivonen)]

I'm not sure, do you know what/how it is supposed to be observable by web content? In a plain web page, I can't get sync access to an iframe's DOM right after creating it in Firefox, but perhaps I'm doing it wrong?

The about:blank DOM is available in an iframe synchronously after inserting the iframe into the parent document. Currently, it seems that extensions don't interact with that synchronous about:blank and only interact with a subsequent asynchronous about:blank. Bug 543435 removes the subsequent about:blank and tries to make the actual initial one visible to extensions (without any blocking).

It might be bad for privacy-preserving extensions (and also our webcompat efforts).

Do you mean Web compat efforts in the sense of us providing extension-injected scripts that fix sites?

I'll need to double check, but my testing shows this is not as strong guarantee in Chromium, which is unsurprising

OK. Good to know that Chromium doesn't guarantee immediate script/style injection into about:blank. In that case, I don't think we should, either, post- bug 543435. (I don't want to introduce a new case of nested event loop.)

Comment 7

[Henri Sivonen (:hsivonen)]

I see that on the patch for bug 543435, :mixedpuppy thinks test_ext_contentscript_about_blank_start.js should not be skipped by the patch.

Is there any chance the Web Extensions team could look into adding a mechanism that ensures that all document_start resources have finished loading before starting document_end scripts? (As noted, I think we shouldn't allow extensions to affect the synchronicity of about:blank, so a difference from all other pages is necessary.)

Comment 8

[Rob Wu [:robwu]]

The entry point for scheduling script injections is injectInto, called via loadContentScript from C++ (primarily by ExtensionPolicyService::CheckContentScripts).

In the document_start case, injectInto immediately calls inject (whereas in the other cases, we await their relevant events first, e.g. DOMContentLoaded for document_end).

When a script is about to execute for the first time in the process, script execution is slightly delayed until the script has been compiled async. Note that extensions are not expected to encounter this delay in practice, because scripts are preloaded as soon as a document request is observed.

In common cases, inject "immediately" injects the script - so document_start scripts are immediately executed when called from the document-element-inserted notification in C++. "immediately" here means "at the next microtask checkpoint" because there is an unconditional await cssPromise, commonly no-op except when css is present. The await cssPromise call only covers CSS that have been registered together with the JS file:

• Example of js and css declared together: test_ext_contentscript_about_blank.html
• Example of js and css declared separately: test_ext_contentscript_about_blank_start.js - note this is the test case that you're considering to skip in bug 543435.

Currently, we rely on blockParsing to also block DOMContentLoaded (thus document_end). If the parser cannot be blocked at that point, then the next obvious step is to seek alternatives, at least for the cases where the parser completes in one task.

If we want to ensure that content scripts are blocked on any previously declared CSS, then we would have to maintain a per-document collection of promises and await that set of Promises when needed (instead of just cssHash). I suppose that we can do that, potentially for about:blank only. While there is no test coverage for the scenario, there is also the consideration of relative ordering between stylesheets: in theory, if a document_end stylesheet has already been parsed where document_start had not, then the document_end stylesheet could apply before the document_start stylesheet.

Introducing a broader about:blank-blocked-on-CSS-load is that extensions would then not be able to execute scripts ASAP, which is against the wishes of privacy-sensitive extensions who want to run their scripts before the page has had a chance to access the JS context in the frame (WECG#513). On the other hand, since web-accessible about:blank can only be created from other documents, we could presume such "all URLs including about:blank" stylesheets to have been loaded & compiled before.

All together, I think that it would be feasible to get the test_ext_contentscript_about_blank_start.js test case passing (and the use cases we care about), by doing the following work:

• Refactor the CSS loading implementation, to only block if any CSS stylesheet (registered by the extension) has not been loaded yet. When there are no pending CSS loads, just add the stylesheet without blockParser, at https://searchfox.org/mozilla-central/rev/ae3df68e9ba2faeaf76445a7650e4e742eb7b4e7/toolkit/components/extensions/ExtensionContent.sys.mjs#523-558
• Add before the CSS load (+add test case): when another CSS stylesheet is loading, wait with adding the stylesheet until the previous stylesheets have loaded.
• At the current await cssPromise checkpoint: await any pending CSS loads of the document by the current extension, if needed.

Comment 9

[Rob Wu [:robwu]]

Attachment: Bug 1792685 - Ensure ordered content script execution

Before this patch, there was only a guaranteed ordering of css/js
injection within individual entries in registered content scripts.

With this patch, this guarantee is extended to multiple content scripts
within the same extension. This is achieved by maintaining an
extension-specific script execution queue for each
ContentScriptContextChild (which wraps a unique window+document).

In the initial case, when scripts or styles have not been compiled
yet, the Script execution adds to the queue, which will block all
further injections until the compilation completes.

In the common case, when there are no pending async compilations,
the script/style injection is immediate.

This patch may affect timing behavior, as follows:

• Content scripts/styles may apply later if there is an earlier
  declaration that is still pending compilation. Since compilation
  is async, in practice this only makes a difference if some of
  the "later" scripts/styles were already somehow cached by an
  earlier injection/preload.

• When a style has already been cached before, the injectInto
  method now applies styles immediately instead of at the next
  microtask checkpoint. This also implies faster script injection
  if js + css is specified together.

• document_start scripts and styles are executed more reliably
  now, because injectInto now awaits both compilations in parallel
  instead of sequentially. This concludes the fix to the issue
  described in https://bugzilla.mozilla.org/show_bug.cgi?id=1892775

Comment 10

[Rob Wu [:robwu]]

Attachment: Bug 1792685 - Add pref to allow reverting to previous timing behavior

Add pref to revert to previous timing behavior. This enables
users/developers to opt out of the new behavior, which may be useful to
confirm or rule out this bug as the cause of the issue.

This pref can be removed in a few cycles, once it has been established
that the changes did not cause undesired regressions.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9481f7aff1d5
Ensure ordered content script execution r=zombie
https://hg.mozilla.org/integration/autoland/rev/a2d827e489b6
Add pref to allow reverting to previous timing behavior r=zombie

Comment 13

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/9481f7aff1d5
https://hg.mozilla.org/mozilla-central/rev/a2d827e489b6

Comment 14

[Rob Wu [:robwu]]

dev-doc-needed: Content scripts and styles are now guaranteed to execute in the order of registration (e.g. the order in manifest.json). Previously, this was only guaranteed for scripts within the same "js" array.

Comment 15

[Rob Wu [:robwu]]

Richard has put up a PR with release notes at https://github.com/mdn/content/pull/39245

The expected order should also be documented at https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/content_scripts

The article already mentions the order of script execution, but does not explicitly state the behavior when the declarations are separate.
There is a question of whether this behavior is only guaranteed in Firefox or also in other browsers. For reference, in bug 1920169 a developer expressed the expectation that Chrome's execution order is in the order of declaration. This behavior is not explicitly documented in Chrome's documentation, however (like MDN, they currently only document the expected order of execution within one declaration).

Comment 16

[Rob Wu [:robwu]]

For documentation purposes, we were wondering what other browsers do.

I asked an Apple engineer, and they claimed that Safari executes scripts in the order as specified in the manifest file.

To get an answer for Chrome, I looked at Chrome's source, and my conclusion is that they also have the order as specified in manifest.json. Sources:

• UserScriptSetManager::GetAllInjections reads a list of scripts to inject from all extensions. All scripts from individual extensions are retrieved by UserScriptSet::GetInjections. These are ordered.
• These scripts are initialized from shared memory by UserScriptSet::UpdateUserScripts, received from UserScriptLoader::SendUpdate in the parent process, via UserScriptLoader::OnScriptsLoaded, which receives the list of scripts_to_load from UserScriptLoader::StartLoad.
• So far, whatever order there was, is maintained. Now comes the trickier part.
• UserScriptLoader::StartLoad creates scripts_to_load from added_scripts_map_, which is a std::map<std::string, ...>. The iteration order of the map is deterministic, and dependent on the ordering relation of its keys, in case of std::string it is a lexicographic comparison.
• UserScriptLoader::AddScripts populates added_scripts_map_, and has multiple callers that provide the id key and script. The first call is at extension startup, by ExtensionUserScriptLoader::AddScriptsForExtensionLoad.
• The static content scripts are initialized before anything else, when the manifest is parsed, by ContentScriptsHandler::Parse. This happens in the order as specified in content_scripts array in manifest.json
• For each parsed content script the id is generated, and the generated id is basically a counter. When ordered lexicographically, the order is maintained.
• Conclusion: with the current implementation of Chrome, the order of content script execution is deterministic, in the order as specified in the manifest file.