1563619 - Destroy BrowsingContexts from crashed processes

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect, P2)

Description

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

When a subframe process crashes, we don't currently detach the browsing contexts related to that subframe from the tree, this can mean that we unfortunately have tree entries with no backing process.

When a process crashes, we should detach all BrowsingContexts which are owned by those processes, and cache their children (as they may still be owned by non-dead browsing contexts)

This can cause parent-process crashes when subframes crash due to interactions with the subframe crash actors.

Comment 1

[Andreas Farre [:farre]]

Is it really this simple? If we compare with what would happen today if a subframe's process would crash we would unconditionally tear down the entire bc tree. If we detach only the bc (and the subtree rooted at that bc) would we need to unbind all corresponding frame elements as well. I mean the one's in the subtree that wasn't in the process that crashed.

Comment 2

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

(In reply to Andreas Farre [:farre] from comment #1)

Is it really this simple? If we compare with what would happen today if a subframe's process would crash we would unconditionally tear down the entire bc tree. If we detach only the bc (and the subtree rooted at that bc) would we need to unbind all corresponding frame elements as well. I mean the one's in the subtree that wasn't in the process that crashed.

So all out-of-process subframes will be torn down by the crash killing the BrowserBridgeParent already, so we don't need to worry about subframes which are not owned by the process which crashed. All frames owned by the process which crashed won't be cleaned up though. We probably want to first cache all of their children, and then trigger a discard on the BrowsingContext objects which are owned by the process which just crashed.

The <iframe> elements themselves which directly reference the crashed BrowsingContext should be transitioned to point to an in-process process-crashed page, which is what the SubframeProcessCrash code is currently trying to do. We might need to pull that behaviour into the platform layer though.

Comment 3

[Andreas Farre [:farre]]

Right, so a couple of more questions:

Will we be keeping the tree above the crashed bc alive? I can't seem to find SubframeProcessCrash. Won't that be kind of a security leak. An attacking page that embeds a cross origin site in an iframe that it can somehow get to crash could maybe detect this.

Caching, detaching children and all of this sounds very much alike what docshells do when we navigate them. Would it be an idea to instead implement BrowsingContext::Naviagate, and implement this case by navigating browsing contexts (or really only the browsing contexts highest up in the tree of browsing contexts) in a crashed process to the process-crashed page?

Comment 4

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

(In reply to Andreas Farre [:farre] from comment #3)

Right, so a couple of more questions:

Will we be keeping the tree above the crashed bc alive? I can't seem to find SubframeProcessCrash. Won't that be kind of a security leak. An attacking page that embeds a cross origin site in an iframe that it can somehow get to crash could maybe detect this.

Current plan was to keep the tree above the bc alive, as it seems not-great to crash an entire tab just because an ad inside of it crashed. Not sure about the security leak potential. Might be worth asking :tjr if it's something worth worrying about, but I'm a bit skeptical right now.

Caching, detaching children and all of this sounds very much alike what docshells do when we navigate them. Would it be an idea to instead implement BrowsingContext::Naviagate, and implement this case by navigating browsing contexts (or really only the browsing contexts highest up in the tree of browsing contexts) in a crashed process to the process-crashed page?

Perhaps, that actually is probably the correct behaviour - navigate to the process-crashed page, just without bothering to unload the previous document.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

(In reply to Andreas Farre [:farre] from comment #3)

I can't seem to find SubframeProcessCrash.

The SubframeCrashHandler / SubframeCrashChild JS Window Actor mechanism was added in bug 1533955 - here's the SubframeCrashHandler:

https://searchfox.org/mozilla-central/rev/40e889be8ff926e32f7567957f4c316f14f6fbef/browser/modules/ContentCrashHandlers.jsm#623-631

and here's the SubframeCrashChild actor:

https://searchfox.org/mozilla-central/rev/40e889be8ff926e32f7567957f4c316f14f6fbef/browser/actors/SubframeCrashChild.jsm

Comment 6

[Andreas Farre [:farre]]

Attachment: Bug 1563619 - Handle subframe crashes in BrowserParent::ActorDestroy.

Comment 7

[Andreas Farre [:farre]]

Try is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1ae2ce616969b87ee641387ac1bc1634e76f822

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d0e5336e455
Handle subframe crashes in BrowserParent::ActorDestroy. r=mconley,kmag,peterv

Comment 9

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/2d0e5336e455