1516022 - promiseDocumentFlushed callbacks will fire even if subframes might need flushes

Status: NEW

(Core :: DOM: Core & HTML, defect, P2)

Description

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

Bug 1448096 landed a patch that uses promiseDocumentFlushed to try to avoid sync layout flushes while resizing.

I reviewed that patch, and it landed, and I missed something pretty big - promiseDocumentFlushed isn't designed to work within subframes. In fact, the patch itself makes reference to that here:

https://searchfox.org/mozilla-central/rev/413dd3a1fab3446749f3690d93ee17e3196f8c37/devtools/client/inspector/inspector.js#624-625

so it reaches up to the window.top and uses its promiseDocumentFlushed instead, but as the bug referenced in that comment (that I filed! :/) mentions:

"sometimes a subframe will report that _it_ doesn't need a style or layout flush, even though its parent _does_, meaning that a style or layout flush could still occur when querying for style / layout information even though the nsGlobalWindowInner determines that no style or layout flushes are required"

We shouldn't be using .top.promiseDocumentFlushed here. Something like getBoundsWithoutFlushing is probably sufficient instead.

Comment 1

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

We should probably disallow .top.promiseDocumentFlushed somehow.

Comment 2

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

(In reply to Mike Conley (:mconley) (:⚙️) (Away from Bugmail Dec 24 - Jan 7) from comment #0)
> Bug 1448096 landed a patch that uses promiseDocumentFlushed to try to avoid
> sync layout flushes while resizing.
> 
> I reviewed that patch, and it landed, and I missed something pretty big -
> promiseDocumentFlushed isn't designed to work within subframes. In fact, the
> patch itself makes reference to that here:
> 
> https://searchfox.org/mozilla-central/rev/
> 413dd3a1fab3446749f3690d93ee17e3196f8c37/devtools/client/inspector/inspector.
> js#624-625
> 
> so it reaches up to the window.top and uses its promiseDocumentFlushed
> instead.
> 

So, this is accurate, but the last part of comment 0 where I quote bug 1441173 - I kinda jumped the gun there. That bug talks about how if we call promiseDocumentFlushed on the subframe, it won't know about parent frames that might need flushing.

I filed this originally because I was getting DevTools failures when trying to land bug 1441168. Using rr, those failures seem to be because promiseDocumentFlushed is running even though one of the subframes of the main window _does_ need a flush.

That sounds like a bug in promiseDocumentFlushed.

So maybe the bigger problem is - how do we make promiseDocumentFlushed resilient to this case?

Comment 3

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

Attachment: Bug 1516022 - promiseDocumentFlushed should ensure no subframes need flushing before running the callback. r?bzbarsky

Depends on D14101

Comment 4

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

Attachment: Bug 1516022 - Regression test. r?Gijs

Depends on D16092

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c683fbe0b984
promiseDocumentFlushed should ensure no subframes need flushing before running the callback. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/4fae3add610f
Regression test. r=Gijs

Comment 6

[Cosmin Sabou [:CosminS]]

Backed out for mochitest failures on browser_editcontrols_update. There is also this assertion starting from the above push:

Assertion failure: parsedPolicyStr.Find("default-src") >= 0 (about: page must contain a CSP including default-src), at /builds/worker/workspace/build/src/dom/base/Document.cpp:4844

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&fromchange=4fae3add610fe59fec7d7a59faaca897409ac078&tochange=0b3a6c7175ca027b27db5943f178f36c2c1e0672&searchStr=mochitest&selectedJob=222273547

Failure logs:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=222273547&repo=autoland&lineNumber=6911
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=222273055&repo=autoland&lineNumber=12565
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=222275200&repo=autoland&lineNumber=17955

Backkout link: https://hg.mozilla.org/integration/autoland/rev/0b3a6c7175ca027b27db5943f178f36c2c1e0672