1717983 - descendant documents of top OOP iframe document keeps inactive if the top OOP iframe document is initially invisible

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

This is the reason why test_bug1451199-1.html (and test_bug1451199-2.html) sometimes timed out on fis-xorig runs.

So what's going on when the time out happens;

1. in fis-xorig runs test_bug1451199-1.html document is loaded in an OOP
2. and the iframe in the test_bug1451199-1.html document is also loaded in the same process of the test_bug1451199-1.html
3. when the presshell for the test_bug1451199-1.html document gets initialized, SetIsActive is called with false since the BrowerChild is invisible at that moment
4. same thing happens for the iframe's preshell, i.e. SetIsActive is called with false
5. then after that, when the BrowserChild::MakeVisible gets called we do call PresShell::SetIsActive(true) for the test_bug1451199-1.html presshell
6. but it doesn't descend down its child presshell (i.e. the iframe presshell)
7. thus the iframe's presshell keeps inactive
8. so even after the iframe's width changes, it doesn't call FlushPendingNotifications for the iframe's presshell since mIsActive check in WillPaint (Note that the WillPaint gets called from nsViewManager::ProcessPendintUpdates in nsRefreshDriver::Tick)
9. thus we never get media query change events for the iframe document

I am quite unsure what the active presshell means. If we don't have situation where an ancestor is inactive but a descendant is active, we should propagate it into descendants?

Comment 1

[Emilio Cobos Álvarez (:emilio)]

but it doesn't descend down its child presshell (i.e. the iframe presshell)

Yeah, that seems broken, we should probably:

• make QueryIsActive handle the subdocument case so that it computes false properly (which right now wouldn't, IIUC).
• Make SetIsActive call QueryIsActive on in-process subdocuments.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1717983 - Improve PresShell active flag handling. r=nika

This moves the logic of whether a pres shell should be active to a
single place to make it sane to reason about, and fixes the
subdocument propagation when a BrowserChild becomes visible.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/jobs?repo=try&revision=6008f5bc4bd46c521591d590ece123da70d6769c

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b46a490223a1
Improve PresShell active flag handling. r=nika

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Thank you Emilio! (Though I didn't expect you to respond in your off. :)).

Comment 6

[Sandor Molnar[:smolnar]]

Backed out for causing Android failures.

Backout link: https://hg.mozilla.org/integration/autoland/rev/57427436cfd82390c961ac0b86b996cb9671c97d

Push with failures

Failure log

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1717983 - Don't consider a browser active if the tab is inactive but we're preserving layers. r=nika

GeckoView always calls preserveLayers(true) on all <browser> elements,
which causes the puppet widget to always be considered visible.

Given how the code worked before, aBrowsingContext.isActive = false
after that call would deactivate the pres shell, but after my patch it
stops doing so.

We don't really want to un-throttle the refresh driver etc just because
we're preserving layers, so propagate the state to the child process and
account for that in the logic to determine PresShell activeness.

Depends on D118703

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1717983 - Pack bool fields better in BrowserChild. r=nika

Not a big deal, but given we have a lot of them, BrowserParent does it,
and it's low effort, seemed worth doing.

Depends on D118884

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cb704553fc64
Improve PresShell active flag handling. r=nika
https://hg.mozilla.org/integration/autoland/rev/effd76640491
Don't consider a browser active if the tab is inactive but we're preserving layers. r=nika
https://hg.mozilla.org/integration/autoland/rev/49c60a0b4565
Pack bool fields better in BrowserChild. r=nika

Comment 10

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/cb704553fc64
https://hg.mozilla.org/mozilla-central/rev/effd76640491
https://hg.mozilla.org/mozilla-central/rev/49c60a0b4565