2003255 - Extensions interact with whether the load event fires on the initial about:blank

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Emilio Cobos Álvarez [:emilio]]

https://bug2002481.bmoattachments.org/attachment.cgi?id=9530020 doesn't log on my regular nightly profile, but logs on a clean profile.

On a file:// path, SingleFile was the culprit. But I still don't get the log there.

I suspect some of the mechanisms that add-ons use to inject scripts on the page are interacting poorly with the sync about:blank.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

[Tracking Requested - why for this release]: Potential compatibility issues

Ok, so the following extensions cause the issue:

• SingleFile (https://addons.mozilla.org/en-US/firefox/addon/single-file/)
• KDE Plasma Integration (https://addons.mozilla.org/en-US/firefox/addon/plasma-integration/)

At least the later is shipped by default by some environments. Also it's likely that other extensions suffer from this issue. So I suggest we at least root-cause this sooner rather than later.

Comment 2

[Adrian London]

(Copied from the above-referenced bug as it's probably a separate issue ...)

I also noticed that Bitwarden (and other extensions, but I mainly use that one with shortcut keys) ignore shortcut keys.

So, ⌘⇧L should open the Bitwarden window. I have to restart Firefox four or five times before it works. It's kind of random and I assume something to do with the (random?) order things start up when Firefox is launched. Of course, this may be a totally different issue - I've done no real investigation.

Comment 3

[Vincent Hilla [:vhilla]]

I can reproduce it with SingleFile.

From the DOM side, everything looks fine in the DocShell and Document logs. DOMContentLoaded is dispatched, but the listeners don't run. It doesn't matter what src is as long as it causes an initial about:blank.

Comment 4

[Vincent Hilla [:vhilla]]

In Chrome, the SingleFile extension causes the iframe load event to become async.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Gildas, any guesses as to what in SingleFile is causing this behaviour change?

Comment 6

[Emilio Cobos Álvarez [:emilio]]

The network tab has a data: uri load with singlefile (seems to be to hook FontFace and such?). I'm guessing the extension triggers a load even in about:blank, and that causes the load event to be effectively made async in chrome (and missing for us).

Comment 7

[Henri Sivonen (:hsivonen)]

Since for the purpose of Web-exposed semantics, there is supposed to be no possibility of Web content injecting load-event-blocking child loads into an about:blank, we might get away with the total hack of having a different indicator than mOnloadBlockCount if mInitialAboutBlankLoadCompleting is true.

Comment 8

[Vincent Hilla [:vhilla]]

Yes, logs show that the extension changes the timing of EndPageLoad / the data: child load blocks the load event. And because of the resetLoadCompleting scope exit in CompleteInitialAboutBlankLoad, nsDocumentViewer::LoadComplete determines restoring=true.

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Per spec (thanks Simon) it seems we should still fire the event sync (https://html.spec.whatwg.org/#process-the-iframe-attributes:~:text=If%20url%20matches,element%2E), but it'd be good to file a spec issue perhaps?

Comment 10

[Vincent Hilla [:vhilla]]

(I think it's clear how SingleFile causes this, so I'm clearing the ni?)

Comment 11

[Vincent Hilla [:vhilla]]

We synchronously get till nsDocLoader::DocLoaderIsEmpty and then exit early due to nsDocLoader::IsBusy, as the load group is pending. I don't think circumventing mOnloadBlockCount would do anything.

The chrome-compatible option is likely to call doc->EndInitialAboutBlankLoadCompleting(); in nsDocumentViewer::LoadComplete.

Otherwise, I'm not sure how we could force the load event. Perhaps we can explicitly call nsDocShell::EndPageLoad after Document::EndLoad if we determine that no load event occurred? Or propagate some flag to the docloader?

:hsivonen are we ok with following Chrome for now, and risk breaking pages because extensions cause an async load, or should I look into forcing the load event?

Comment 12

[Henri Sivonen (:hsivonen)]

I think it's very bad to let extensions affect this particular bit of Web-exposed behavior, so I'd prefer not to be as broken as Chrome and I'd prefer to make load group emptying not fire a load event for the initial doc and to manually fire a lead event at the point where the load event would be fired if stuff hadn't been added to the load group.

Comment 13

[Gildas Lormeau]

Hello, author of SingleFile here, it turns out that I accidentally fixed this bug in the update currently under review.

In the past, there was no "world" property that allowed a script to be injected into the MAIN world. As a result, SingleFile attempted to inject this type of script (hooks) via the content script. It seems that since I fixed this, the "load" event is being dispatched correctly.

SingleFile attempted this injection twice, I believe. If you wish, I can try to identify what was causing this bug.

Comment 14

[Gildas Lormeau]

I can reproduce the issue with this extension implementation.

manifest.json

{
    "name": "load bug reproducer",
    "version": "1.0",
    "manifest_version": 2,
    "content_scripts": [
        {
            "matches": ["<all_urls>"],
            "run_at": "document_start",
            "js": ["script.js"],
            "all_frames": true,
			"match_about_blank": true
        }
    ]
}

script.js:

const scriptElement = document.createElement("script");
scriptElement.src = "data:," + "(" + injectedScript.toString() + ")()";
(document.documentElement || document).appendChild(scriptElement);

function injectedScript() {
    console.log("injected script loaded");
}

Comment 15

[BugBot [:suhaib / :marco]]

The bug is marked as tracked for firefox147 (nightly). We have limited time to fix this, the soft freeze is in 2 days. However, the bug still isn't assigned.

:hsinyi, could you please find an assignee for this tracked bug? If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit BugBot documentation.

Comment 16

[Vincent Hilla [:vhilla]]

Here's a comment to document me iterating on ideas for forcing the load.

• In CompleteInitialDocumentLoad, block the load event such that EndLoad() will dispatch DOMContentLoaded but not load. Then explicitly call EndPageLoad() circumvent the docloader. mEODForCurrentDocument ensures we don't get a duplicate load if the docloader becomes empty later. A problem is that state change notifications won't be sent sync. Also, perhaps something could mess with the document and start another load, and mEODForCurrentDocument becoming false breaks the consecutive task.
• Call EndInitialAboutBlankLoadCompleting() from nsDocumentViewer::LoadComplete instead of via a scope exit. Then it can be used to track whether a load occured. After EndLoad(), if that flag is still true, we manually call EndPageLoad(). Similar problems as with the above.
• In nsDocLoader::DocLoaderIsEmpty(), override a few checks if we want to force the load. We want to force it if InitialAboutBlankLoadCompleting() is true, i.e. we are in the scope of CompleteInitialDocumentLoad(). This will clear mIsLoadingDocument as well as mIsLoadingDocument, so no duplicate load events. It seems to me this fits best into the current code.

Comment 17

[Vincent Hilla [:vhilla]]

Attachment: Bug 2003255 - Force sync about:blank despite busy DocLoader. r=#dom-core

Comment 18

[Vincent Hilla [:vhilla]]

The plan for 147 is to back bug 543435 out of beta in bug 2003720.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/e1d22ea27f36
https://hg.mozilla.org/integration/autoland/rev/4e1d18402a1a
Force sync about:blank despite busy DocLoader. r=dom-core,smaug

Comment 20

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/4e1d18402a1a