Open Bug 1582788 Opened 2 years ago Updated 3 months ago

Replace dummy.xhtml with an about:blank content viewer

Categories

(WebExtensions :: General, task, P1)

task

Tracking

(Not tracked)

People

(Reporter: bgrins, Assigned: emalysz)

Details

Attachments

(1 file)

This file https://searchfox.org/mozilla-central/source/toolkit/components/extensions/dummy.xul is an empty chrome xul file (now loaded as an HTML document) used for extensions. Looking at the callers, I think this could be just replaced with an empty .html file instead (nothing seems to care that there's a <window> root element): https://searchfox.org/mozilla-central/search?q=dummy.xul&path=

The reason that we initially had to use a XUL document is that when we used an HTML function, the lack of a layer manager caused us to never trigger layout, which caused our browsers to never get layout frames, which caused them to never get frameloaders.

That likely doesn't matter anymore, given that XULDocument and HTMLDocument are now the same thing, in which case, we should be able to just create an about:blank content viewer and get rid of the dummy document altogether. But it needs testing first.

Summary: Replace dummy.xul with an html document → Replace dummy.xul with an about:blank content viewer

(In reply to Kris Maglione [:kmag] from comment #1)

The reason that we initially had to use a XUL document is that when we used an HTML function, the lack of a layer manager caused us to never trigger layout, which caused our browsers to never get layout frames, which caused them to never get frameloaders.

Good to know. Even though it's an html document now, it still uses a <xul:window> which could change how layers are working so it will definitely need testing.

That likely doesn't matter anymore, given that XULDocument and HTMLDocument are now the same thing, in which case, we should be able to just create an about:blank content viewer and get rid of the dummy document altogether. But it needs testing first.

But don't we still need a chrome document so that we stuff like creating browsers https://searchfox.org/mozilla-central/rev/e18057a9613ffda06dfd3640209ca234ed7dc37d/toolkit/components/extensions/ExtensionParent.jsm#1397? We support dummy.xul with the browser custom element (https://searchfox.org/mozilla-central/rev/e18057a9613ffda06dfd3640209ca234ed7dc37d/toolkit/content/customElements.js#743-753) and none of that code would run if we don't match the isSystemPrincipal check at https://searchfox.org/mozilla-central/rev/e18057a9613ffda06dfd3640209ca234ed7dc37d/toolkit/components/processsingleton/CustomElementsListener.jsm#16

Flags: needinfo?(kmaglione+bmo)

We do still need a chrome document, but we already call createAboutBlankContentViewer with a chrome principal, for a complicated set of reasons, so we already have one.

Flags: needinfo?(kmaglione+bmo)

That said... the check in question also excludes about:blank, which would obviously need to be fixed.

(In reply to Kris Maglione [:kmag] from comment #4)

That said... the check in question also excludes about:blank, which would obviously need to be fixed.

I guess we can't differentiate between this about:blank and any others? bz made a change to intentionally exclude about:blank in Bug 1528762, so I'd be worried about removing that check.

Not really. I suppose we could manually inject them into that window, though. Maybe by having the custom elements code inject a function we can call to do the things it normally does.

(In reply to Kris Maglione [:kmag] from comment #6)

Not really. I suppose we could manually inject them into that window, though. Maybe by having the custom elements code inject a function we can call to do the things it normally does.

Is there a particular benefit to using the about:blank content viewer instead of a chrome HTML document? In general I'd prefer to not add new entry points or special cases for how our chrome CEs get loaded in various documents in order to keep things more consistent (i.e. we can always assume we aren't going to be running any of this stuff in a document that doesn't match https://searchfox.org/mozilla-central/rev/e18057a9613ffda06dfd3640209ca234ed7dc37d/toolkit/components/processsingleton/CustomElementsListener.jsm#16-23).

Yes. Having to load a document from a channel is considerably more expensive. about:blank content viewers are created synchronously using essentially pure DOM operations.

(In reply to Kris Maglione [:kmag] from comment #8)

Yes. Having to load a document from a channel is considerably more expensive. about:blank content viewers are created synchronously using essentially pure DOM operations.

OK, probably worth trying it with that as well. We can at least test with https://phabricator.services.mozilla.com/D46629 to make sure we aren't tripping any of these layer issues you mentioned.

Should we be loading the URI like this https://hg.mozilla.org/try/rev/8eaa3abe782ef960f331f66a57d940ffea02f881#l2.16?
The current patch that I have up has a few unexpected failures (https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=267760663&revision=5d3c62411eb00c449758cbe77a5fedbb0903e008) when we change to an about:blank content viewer.

Flags: needinfo?(kmaglione+bmo)
Assignee: nobody → emalysz
Attachment #9094254 - Attachment description: Bug 1582788, replace dummy.xul with an empty .html file → Bug 1582788, replace dummy.xul with an about:blank content viewer

setting prio to move off our triage list

Priority: -- → P1
No longer blocks: 1579952
Summary: Replace dummy.xul with an about:blank content viewer → Replace dummy.xhtml with an about:blank content viewer

(In reply to Emma Malysz from comment #11)

Should we be loading the URI like this https://hg.mozilla.org/try/rev/8eaa3abe782ef960f331f66a57d940ffea02f881#l2.16?
The current patch that I have up has a few unexpected failures (https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=267760663&revision=5d3c62411eb00c449758cbe77a5fedbb0903e008) when we change to an about:blank content viewer.

I would expect that to work, but the artifacts for that try push are unfortunately gone.

Flags: needinfo?(kmaglione+bmo)
Severity: normal → N/A
You need to log in before you can comment on or make changes to this bug.