Closed Bug 1563619 Opened 5 years ago Closed 5 years ago

Destroy BrowsingContexts from crashed processes

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla70
Fission Milestone M4
Tracking Status
firefox70 --- fixed

People

(Reporter: nika, Assigned: farre)

References

Details

Attachments

(1 file)

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.

Blocks: 1560106
Assignee: nobody → afarre

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.

Flags: needinfo?(nika)

(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.

Flags: needinfo?(nika)

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?

Flags: needinfo?(nika)

(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.

Flags: needinfo?(nika)

(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

Attachment #9083561 - Attachment description: Bug 1563619 - Handle subframe crashes in ContentParent::ActorDestroy. → Bug 1563619 - Handle subframe crashes in BrowserParent::ActorDestroy.
Pushed by afarre@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2d0e5336e455 Handle subframe crashes in BrowserParent::ActorDestroy. r=mconley,kmag,peterv
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Regressions: 1577129
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: