1646567 - Fix GetInProcessParentDocshell usage in nsDocShell::HasUnloadedParent

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P3)

Comment 1

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

Should be possible to recursively check if our parent WindowContext is the current on in it's BrowsingContext up the tree, only checking the docshell if it's present.

Comment 2

[Steven MacLeod [:smacleod]]

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

... only checking the docshell if it's present.

If I understand correctly, this means we'd only be checking same origin docshells up the tree. So if we had the situation a.com <- b.com <- a.com, e.g.:

<!-- http://a.com -->
<iframe src="b.com"></iframe>

<!-- http://b.com -->
<iframe src="http://a.com/frame.html"></iframe>

If nsDocShell::HasUnloadedParent is called on the http://a.com/frame.html docshell, we'd end up not checking if the docshell for b.com is unloaded. Is this okay because if it was unloaded http://a.com/frame.html's parent WindowContext (http://b.com) wouldn't be the current WindowContext of the BrowsingContext, so that's how we're checking it? or is there some other reason we can skip the check for OOP docshells in the tree?

Comment 3

[Steven MacLeod [:smacleod]]

Reading my last comment back it's a little less understandable than I was hoping. Basically I'm trying verify my understanding that checking if the parent WindowContext is the current WindowContext of its BrowsingContext is a sufficient proxy for checking if the OOP docshell is unloaded.

Comment 4

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

Yeah, that's pretty much right. If it was unloaded, the parent WindowContext wouldn't be current anymore (and/or any of our ancestors would be discarded). This doesn't handle the case where the cross-process frame is currently running unload event handlers, but that is only relevant for in-process frames, as running unload is synchronous(*)

*: JS can spin a nested event loop, but it's still conceptually synchronous.

Comment 5

[Steven MacLeod [:smacleod]]

Attachment: Bug 1646567 - fix nsDocShell::HasUnloadedParent to work for OOP frames. r?nika

With fission enabled, walking the docshell tree to find unloaded
parents would stop at the first OOP parent. Instead we now walk
the BrowsingContext/WindowContext tree. To still check parents
which are OOP, we take advantage of the fact that a parent
WindowContext will no longer be the current WindowContext if
the parent was unloaded. For in process docshell's we additionally
check the docshell directly.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6164e09df60e
fix nsDocShell::HasUnloadedParent to work for OOP frames. r=nika

Comment 7

[Atila Butkovits]

Backed out for failures at nsDocShell.
The push caused mass failures.

Backout link: https://hg.mozilla.org/integration/autoland/rev/3524c48add1142ccd10b6b114864966ea93b2612

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedTaskRun=YFbj8K9-QnSQEfxdcQN0tw.0&resultStatus=testfailed%2Cbusted%2Cexception&revision=6164e09df60e24f48ce427c7224e1ddeaff0d6fe

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=311677598&repo=autoland&lineNumber=1586

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=311678033&repo=autoland&lineNumber=752

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=311677365&repo=autoland&lineNumber=1265

Comment 8

[Steven MacLeod [:smacleod]]

(In reply to Atila Butkovits from comment #7)

The push caused mass failures.

Woops, sorry about that!

Will land the fixed up patch after a try run.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0d06bfd1fa94
fix nsDocShell::HasUnloadedParent to work for OOP frames. r=nika

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0d06bfd1fa94