Ensure that all document_start resources have finished loading before starting document_end scripts
Categories
(WebExtensions :: General, defect, P1)
Tracking
(firefox139 fixed)
Tracking | Status | |
---|---|---|
firefox139 | --- | fixed |
People
(Reporter: hsivonen, Assigned: robwu)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, Whiteboard: [addons-jira])
Attachments
(2 files)
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.
Reporter | ||
Comment 1•3 years ago
|
||
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•3 years ago
|
||
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•3 years ago
|
||
(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 ofdocument_end
scripts untildocument_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.
Reporter | ||
Comment 4•3 years ago
|
||
(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•2 years ago
•
|
||
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 ofabout: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).
Reporter | ||
Comment 6•2 years ago
|
||
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.)
Reporter | ||
Comment 7•2 years ago
|
||
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.)
Updated•2 years ago
|
Assignee | ||
Comment 8•2 years ago
|
||
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
andcss
declared together: test_ext_contentscript_about_blank.html - Example of
js
andcss
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 Promise
s 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.
Updated•2 years ago
|
Assignee | ||
Updated•2 years ago
|
Updated•2 years ago
|
Assignee | ||
Updated•5 months ago
|
Assignee | ||
Comment 9•5 months ago
|
||
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
Assignee | ||
Comment 10•5 months ago
|
||
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•3 months ago
|
||
Comment 13•3 months ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9481f7aff1d5
https://hg.mozilla.org/mozilla-central/rev/a2d827e489b6
Assignee | ||
Comment 14•3 months ago
|
||
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.
Assignee | ||
Comment 15•3 months ago
|
||
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).
Assignee | ||
Comment 16•3 months ago
|
||
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 byUserScriptSet::GetInjections
. These are ordered.- These scripts are initialized from shared memory by
UserScriptSet::UpdateUserScripts
, received fromUserScriptLoader::SendUpdate
in the parent process, viaUserScriptLoader::OnScriptsLoaded
, which receives the list ofscripts_to_load
fromUserScriptLoader::StartLoad
. - So far, whatever order there was, is maintained. Now comes the trickier part.
UserScriptLoader::StartLoad
createsscripts_to_load
fromadded_scripts_map_
, which is astd::map<std::string, ...>
. The iteration order of the map is deterministic, and dependent on the ordering relation of its keys, in case ofstd::string
it is a lexicographic comparison.UserScriptLoader::AddScripts
populatesadded_scripts_map_
, and has multiple callers that provide the id key and script. The first call is at extension startup, byExtensionUserScriptLoader::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 incontent_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.
Description
•