Closed Bug 1442020 Opened 6 years ago Closed 6 years ago

promiseDocumentFlushed needs to wait until the PresShell no longer needs style _and_ layout flushes

Categories

(Core :: General, enhancement)

Product:
Core
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

Details

Attachments

(2 files)

Bug 1442020 - Ensure neither style nor layout flushes are required when running promiseDocumentFlushed callbacks.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1442020 - Make PanelMultiView's description height workaround account for the possibility that panels have closed before a refresh driver tick.
59 bytes, text/x-review-board-request
Paolo
: review+
Details 
In bug 1434376, I added promiseDocumentFlushed. It's supposed to run JS when we know it's safe to query for style and layout information.

I was using nsIPresShell::NeedFlush(FlushType::Style) to check if flushes were necessary - I assumed that if this returned false, then we could assume that neither style _nor_ layout flushes were required.

That's definitely not true. Look at how NeedFlush is defined:

https://searchfox.org/mozilla-central/rev/14d933246211b02f5be21d2e730a57cf087c6606/layout/base/nsIPresShell.h#608-609

FlushType::InterruptibleLayout > FlushType::Style, and so it's possible for us to run promiseDocumentFlushed callback when no style flushes are required, but when layout flushes _are_.

We should ensure that neither style nor layout flushes are required before running promiseDocumentFlushed callbacks.
Hey bz, do you have the time to look at this very small addendum / correction to promiseDocumentFlushed?
Flags: needinfo?(bzbarsky)
Blocks: 1358719
I have no permission to comment on mozreview therefore here:

>  Assert.ok(!gWindowUtils.needsFlush(Ci.nsIDOMWindowUtils.FLUSH_LAYOUT), "No style flushes are required.");


typo: style -> layout
(In reply to maggus.staab from comment #3)
> I have no permission to comment on mozreview therefore here:
> 
> >  Assert.ok(!gWindowUtils.needsFlush(Ci.nsIDOMWindowUtils.FLUSH_LAYOUT), "No style flushes are required.");
> 
> 
> typo: style -> layout

That's a great catch - thanks, maggus.staab!
Comment on attachment 8954934 [details]
Bug 1442020 - Ensure neither style nor layout flushes are required when running promiseDocumentFlushed callbacks.

https://reviewboard.mozilla.org/r/224108/#review230288
Attachment #8954934 - Flags: review+
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/322fb820d019
Ensure neither style nor layout flushes are required when running promiseDocumentFlushed callbacks. r=bz
Backed out on suspicion of causing site identity related browser-chrome failures, e.g. in browser/base/content/test/siteIdentity/browser_bug822367.js:

https://hg.mozilla.org/integration/autoland/rev/aacc7cd0f266ac061ef38f6baa9cb9f479d3749c

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=322fb820d019333004f757229ac1b2c47fa05df9&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=165252128&repo=autoland

17:42:02     INFO -  112 INFO TEST-PASS | browser/base/content/test/siteIdentity/browser_bug822367.js | securityView-body has expected attr for passiveLoaded -
17:42:02     INFO -  113 INFO TEST-PASS | browser/base/content/test/siteIdentity/browser_bug822367.js | CC using secure icon -
17:42:02     INFO -  114 INFO TEST-PASS | browser/base/content/test/siteIdentity/browser_bug822367.js | CC using secure icon -
17:42:02     INFO -  Buffered messages finished
17:42:02    ERROR -  115 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/siteIdentity/browser_bug822367.js | Test timed out -

Flush triggering popup closing?
Flags: needinfo?(mconley)
Looking, thanks.
Flags: needinfo?(mconley)
Flags: needinfo?(bzbarsky)
So I think I've got a reproducible test case here.

Hey hiro, this is related to the mNeedThrottledAnimationFlush question I asked you in IRC earlier. If you run the test case I supplied with:

./mach mochitest dom/base/test/browser_promiseDocumentFlushed.js

on Windows[1], you'll note that the test will time out. The promiseDocumentFlushed Promise will not resolve while the sync animation is occurring, _UNTIL_ you move the mouse over the browser window.

Why is this? I worry that this means that promiseDocumentFlushed Promises will not resolve unless the mouse is over the associated window, which would be bad. When you have time, can you help me understand why this is?

[1]: If you're like me, you have to backout f3bfda748e33 to run tests on Windows due to bug 1439727
Flags: needinfo?(hikezoe)
So, the problem here from what I can tell is that mNeedThrottledAnimationFlush flag that is being checked in NeedFlush() is generally true if there is any animations running on the compositor.  As Markus pointed out to me on IRC, it gets cleared when we process styling in refresh driver's tick, but we do *skip* styling process for animations running on the compositor.  So we could easily get stucked to wait for being the flag cleared. 

As for promiseDocumentFlushed we should ignore mNeedThrottledAnimationFlush, I think animations on the compositor doesn't affect layout information what we want when we use promiseDocumentFlushed, right?  Transform animation on the compositor might affect layout information, but for such case we do update the transform animation on the main-thread periodically (every 200ms) to update scroll bar position and to notify IntersectionObserver in these days.  So it will not be a big problem for promiseDocumentFlushed either?
Flags: needinfo?(hikezoe)
Depends on: 1442861
(In reply to Hiroyuki Ikezoe (:hiro) from comment #14)

Hi hiro,

I don't think I 100% understand how to proceed here based on this feedback. Even with the patch from bug 
1442861, the test case I asked about in comment 13 fails. So I think I must be misunderstanding.

Forgive me if I'm asking you to repeat yourself (I recall discussing this with you in IRC), but I want to ensure I fully understand; is the idea promiseDocumentFlushed needs to only care about mNeedStyleFlush or mNeedLayoutFlush? Or that promiseDocumentFlushed only needs to care about nsIPresShell::NeedFlush(FlushType::Style) and shell->HasPendingReflow()?

How do you suggest I modify promiseDocumentFlushed so that:

1) the JS callback only ever runs when style and layout flushes are not required, and
2) such that the test case passes?
Flags: needinfo?(hikezoe)
No worries!

(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #15)
> Forgive me if I'm asking you to repeat yourself (I recall discussing this
> with you in IRC), but I want to ensure I fully understand; is the idea
> promiseDocumentFlushed needs to only care about mNeedStyleFlush or
> mNeedLayoutFlush?

Yes, right. That's what I am suggesting.  Even with the third revision of the patch for this bug, does the test still fail?

> Or that promiseDocumentFlushed only needs to care about
> nsIPresShell::NeedFlush(FlushType::Style) and shell->HasPendingReflow()?

I don't quite understand about reflow things, so HasPendingReflow might be necessary.  But as far as I can tell, we shouldn't use NeedFlush() there.

> How do you suggest I modify promiseDocumentFlushed so that:
> 
> 1) the JS callback only ever runs when style and layout flushes are not
> required, and

Theoretically we should probably consider throttled *transform* animations *needs* layout flush, but I think we can ignore throttled *transform* animation for the purpose of promiseDocumentFlushed.
Flags: needinfo?(hikezoe)
So as I gather, it turns out that it's possible for mNeedStyleFlush to be false, but for a layout flush to still be required. I ran into that while writing a reflow test for a removal I'm doing in bug 1358719.

This patch makes it so that we check both mNeedStyleFlush and mNeedLayoutFlush when determining whether or not we can run the JS callbacks. This means exposing mNeedLayoutFlush on nsIPresShell to nsGlobalWindowInner.

Can I carry your r+ forward, bz?
Flags: needinfo?(bzbarsky)
It's not clear to me what the semantic difference is between HasPendingReflow() and NeedLayoutFlush().  Might be worth asking whoever has blame on HasPendingReflow() how they differ and documenting...
Flags: needinfo?(bzbarsky)
Oh, and if they don't differ, maybe we can get rid of one or the other.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #19)
> It's not clear to me what the semantic difference is between
> HasPendingReflow() and NeedLayoutFlush().  Might be worth asking whoever has
> blame on HasPendingReflow() how they differ and documenting...

Having talked to a few people (emilio, dbaron, hiro), what I've come to conclude is that:

1. mNeedLayoutFlush is true if the frames are dirty, and any JS that queries size or position information will cause a synchronous layout flush.

2. HasPendingReflows is true if we're scheduled to run a reflow (or will soon be scheduled to run a reflow). This can occur if, for example, we interrupted a reflow, and will soon resume it.

According to dbaron, there are a few edge cases where mNeedLayoutFlush will be true, but HasPendingReflows will be false. One of those is window resizes, where apparently we set mNeedLayoutFlush, but don't immediately queue up the reflow. I think that's what I hit in bug 
1358719 - that bug adds a test where the window is resized, and that meant that nsGlobalWindowInner::DidRefresh would sometimes evaluate this condition:

https://searchfox.org/mozilla-central/rev/efce4ceddfbe5fe4e2d74f1aed93894bada9b273/dom/base/nsGlobalWindowInner.cpp#7526-7532

as false, and call the promiseDocumentFlushed callback, _even though_ mNeedLayoutFlush was true (and thus putting us in danger of a sync layout flush).

So I'm now quite certain that mNeedLayoutFlush is what we want the promiseDocumentFlushed stuff to check, and I'm also reasonably certain that HasPendingReflows is different enough that we can't just flat out get rid of one or the other.

Given the above, do you think I'm okay to land?
Flags: needinfo?(bzbarsky)
Yes, that sounds fine.  Thank you for digging through that!
Flags: needinfo?(bzbarsky)
And as I said above, turning some of that into comments would be super.  ;)
Added some comments in the latest iteration. Should I go further in depth? If so, where? The definition of NeedLayoutFlush?
Flags: needinfo?(bzbarsky)
Comment on attachment 8954934 [details]
Bug 1442020 - Ensure neither style nor layout flushes are required when running promiseDocumentFlushed callbacks.

https://reviewboard.mozilla.org/r/224108/#review231774

::: layout/base/nsIPresShell.h:630
(Diff revision 6)
>      if (!ObservingStyleFlushes())
>        DoObserveStyleFlushes();
>    }
>  
>    bool NeedStyleFlush() const { return mNeedStyleFlush; }
> +  bool NeedLayoutFlush() const { return mNeedLayoutFlush; }

I think it really would be good to explain how this differs from HasPendingReflow.  Fundamentally, this should probably say that this returns true if we might need to do a reflow, even if we haven't scheduled it yet.
Attachment #8954934 - Flags: review+
Can do, thanks!
Flags: needinfo?(bzbarsky)
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/93a515ea1721
Ensure neither style nor layout flushes are required when running promiseDocumentFlushed callbacks. r=bz
https://hg.mozilla.org/mozilla-central/rev/93a515ea1721
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Backed out for bc failures on /browser_devices_get_user_media_tear_off_tab.js.

Log: https://treeherder.mozilla.org/logviewer.html#?job_id=166778296&repo=autoland&lineNumber=2488
Flags: needinfo?(mconley)
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/0e28b41ea4c8
Backed out changeset 93a515ea1721 for bc failures on /browser_devices_get_user_media_tear_off_tab.js. a=backout
Those failures were on Windows 7 pgo, at least dominantly.
Status: RESOLVED → REOPENED
status-firefox60: fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla60 → ---
Okay, I think I have a patch that addresses the failures. Posted for review from Paolo.
Flags: needinfo?(mconley)
Comment on attachment 8957269 [details]
Bug 1442020 - Make PanelMultiView's description height workaround account for the possibility that panels have closed before a refresh driver tick.

https://reviewboard.mozilla.org/r/226190/#review232308

r+ with an issue because, while I'd like you to try an alternative, you should also feel free to land this sooner rather than later if it turns out to be more difficult than expected.

::: browser/components/customizableui/PanelMultiView.jsm:1326
(Diff revision 1)
> +      // Bail out if the window has gone away in the mean time
> +      if (!this.window) {
> +        return;
> +      }

Hm, this.window is actually this.node.ownerGlobal, so I guess this means the node has been removed from the document rather than the window gone away entirely?

I think the comments, or at least the first one to avoid repeating ourselves, should be updated to describe what we're trying to fix, so it's clear if we could remove these checks at some point in the future.

See also the "isOpenIn" function that we use elsewhere in the same file. I wonder if checking "!this.node.panelMultiView" here would work as well? It would be better as it's more consistent with the other checks.

Also, if we made "this.window" a lazy getter we would make this check ineffective without noticing.

nit: "in the meantime."
Attachment #8957269 - Flags: review?(paolo.mozmail) → review+
Can't see the failure on try with pgo, so attempting to re-land.
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ef78c1a7149d
Ensure neither style nor layout flushes are required when running promiseDocumentFlushed callbacks. r=bz
https://hg.mozilla.org/integration/autoland/rev/e506d81c512b
Make PanelMultiView's description height workaround account for the possibility that panels have closed before a refresh driver tick. r=Paolo
https://hg.mozilla.org/mozilla-central/rev/ef78c1a7149d
https://hg.mozilla.org/mozilla-central/rev/e506d81c512b
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Assignee: nobody → mconley
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: