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

Status: NEW

(WebExtensions :: General, defect, P2)

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.