1892593 - Full browser crash loading chrome://browser/content/browser.xhtml inside a content tab

Status: NEW

(Firefox :: Tabbed Browser, defect, P3)

Description

[Raphael]

Attachment: ff_asan_log.4607

Steps to reproduce:

Visit the page chrome://browser/content/browser.xhtml
If the crash did not occur, click on the Account icon

Actual results:

This crash is similar to Bug 1884047. I also observe multiple errors in the console when visiting this address.
The crash occurs at a null address, detailed report attached from Asan.

Comment 1

[Mayank Bansal]

STR:
Copy-paste chrome://browser/content/browser.xhtml in the address-bar
A "tab within a tab" thing will open
Right Click on the main tab-bar -> Customize Toolbar
The customization tab will open. Close it by clicking the red "x" button.

AR: Crash
Crash report :
https://crash-stats.mozilla.org/report/index/5d9f4457-15d3-4d06-9638-ae8c60240421
https://crash-stats.mozilla.org/report/index/f43048b5-de73-458f-ac50-840660240421#tab-bugzilla

Comment 2

[Mayank Bansal]

Bisection:
Bug 1875100 - Propagate top level activeness automatically to top descendants. r=nika,tabbrowser-reviewers,mconley,extension-reviewers,robwu,geckoview-reviewers,owlish,kaya

For that, opt in tabbrowser and the shopping sidebar to manual
activeness management.

Differential Revision: https://phabricator.services.mozilla.com/D198942

Comment 3

[Mayank Bansal]

Attachment: about:support

Comment 4

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

Set release status flags based on info from the regressing bug 1875100

Comment 5

[Emilio Cobos Álvarez (:emilio)]

I mean, is loading browser.xhtml inside a content tab something that we're fine doing / we should support at all?

There's all sorts of weird stuff going on here, like the <browsers> inside failing to load due to this, in a way that leave the BrowsingContext attached, but with no embedder element, which already causes previous assertions in debug builds.

What is failing there is that IsTop() is false, because we have a frame like this for a tab:

XUL* browser@0x703b4c11d400 contextmenu="contentAreaContextMenu" message="true" messagemanagergroup="browsers" tooltip="aHTMLTooltip" type="content" manualactiveness="true" maychangeremoteness="true" autocompletepopup="PopupAutoComplete" state=[100820000] flags=[00200002] selectorflags=[00000000] primaryframe=(nil) refcount=9<>

Which is not top, which is something that usually can't happen if browser.xhtml would be in a chrome docshell.

So I guess, questions:

• Is there any good reason to support loading browser.xhtml inside a content tab nowadays?
• If not, should we try harder to block this?

Comment 6

[:Gijs (he/him)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)

I mean, is loading browser.xhtml inside a content tab something that we're fine doing

It was more trouble than it was worth to stop it before.

/ we should support at all?

No, we don't care about supporting this and there isn't really a good usecase for doing it, as far as I'm aware.

• Is there any good reason to support loading browser.xhtml inside a content tab nowadays?

None that I'm aware of.

• If not, should we try harder to block this?

Maybe. I'm kind of fine with this MOZ_Crashing so we could also just wontfix this, I think? What would you propose?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

I think WONTFIX is fine unless there's a trivial-ishway of blocking this file to be loaded on a content tab. I don't know of a way of doing that off-hand... Nika, do you happen to know?

Comment 8

[Nika Layzell [:nika] (ni? for response)]

I unfortunately don't think we have a good HTTP-header like system for associating extra information with a chrome:// URI (or a resource:// URI). The chrome service technically has some flags associated with packages which we could potentially abuse for that (that's how things like https://searchfox.org/mozilla-central/rev/863aef60f9fa1536884252ad90cad1d8ee9d8a41/chrome/nsIChromeRegistry.idl#49-76 are implemented), but those don't work on a per-URL basis.

If we had some system for that, we could add some kind of flag like CHROME_WINDOW_ONLY and then make document loads of those URIs fail unless they are in a chrome BrowsingContext, but we don't so I don't think there's an easy solution. We'd want the load to fail before DOM parsing & document creation.

I'm inclined to make this WONTFIX. You have to do something like this fairly intentionally, much like how you can load about:crashparent, and it's unsurprising that it doesn't work.

Comment 9

[:Gijs (he/him)]

Sorry for not posting this yesterday, I was clearly having a case of the Mondays. In the fresh light of Tuesday morning: I'm pretty happy to redirect any chrome:// load (where the channel's original URI wasn't an about one) in a content tab to about:neterror (ie LoadErrorPage) in the relevant docshell code. Bonus if we can constrain it to HTML/XHTML/XUL type loads by inspecting the channel mimetype (but I could live with doing it for all URLs). That would also "fix" the "see also" bug. Is there any reason that would be a bad idea / wouldn't work here?

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Attachment: WIP: Bug 1892593 - Block loading chrome:// URIs within content browsing contexts

I expect this change will break a number of tests.

Comment 11

[Nika Layzell [:nika] (ni? for response)]

(In reply to :Gijs (he/him) from comment #9)

Sorry for not posting this yesterday, I was clearly having a case of the Mondays. In the fresh light of Tuesday morning: I'm pretty happy to redirect any chrome:// load (where the channel's original URI wasn't an about one) in a content tab to about:neterror (ie LoadErrorPage) in the relevant docshell code. Bonus if we can constrain it to HTML/XHTML/XUL type loads by inspecting the channel mimetype (but I could live with doing it for all URLs). That would also "fix" the "see also" bug. Is there any reason that would be a bad idea / wouldn't work here?

I have attached a sloppy WIP patch to this bug which could perhaps be used to figure out what might be broken with a change like that. It should block toplevel content document loads of chrome:// URIs (unless I typoed something, which is very possible), without blocking things like about URIs.

It might be worthwhile to check and see what the consequences of something like this would be. Off the top of my head I wouldn't be surprised if it broke sidebars, devtools, and/or a number of other tests, though I haven't built it yet to find out (still digging through my ni? and review backlog).

Comment 12

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

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

For more information, please visit BugBot documentation.

Comment 13

[Marco Bonardo [:mak]]

(In reply to Nika Layzell [:nika] (ni? for response) from comment #11)

It might be worthwhile to check and see what the consequences of something like this would be.

Out of officially supported cases, I think various (don't have numbers, sorry) advanced users load chrome://browser/content/places/places.xhtml in a content tab to avoid having the Library as a separate window. They may have a bookmark pointing to that URL. Long term this problem should be solved by Firefox View, but we're not there yet.

Comment 14

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #13)

(In reply to Nika Layzell [:nika] (ni? for response) from comment #11)

It might be worthwhile to check and see what the consequences of something like this would be.

Out of officially supported cases, I think various (don't have numbers, sorry) advanced users load chrome://browser/content/places/places.xhtml in a content tab to avoid having the Library as a separate window. They may have a bookmark pointing to that URL. Long term this problem should be solved by Firefox View, but we're not there yet.

Would we be OK with users like that having to flip a pref? browser.dangerous_chrome_urls_enabled or something like that?

Comment 15

[Marco Bonardo [:mak]]

My concerns are:

1. it doesn't exactly work correctly, various things are broken already (dnd and probably other features), so we'd prefer it to not work at all... but I suspect users will be vocal about this, without a replacement.
2. long term the Library window must go away, so there should not be something (like a pref) that makes this look like a supported option, it works but we're not fixing any part of it.

For these reasons, I'd probably prefer an hardcoded exception in the code, rather than a pref.