Closed Bug 1442218 Opened 6 years ago Closed 6 years ago

tests that relies on descriptionHeightWorkaround fails after bug 1193394

Categories

(Core :: DOM: Core & HTML, enhancement)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

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

People

(Reporter: arai, Assigned: arai)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

Defer MicroTask checkpoint to the outside of loop in nsGlobalWindowInner::CallDocumentFlushedResolvers and nsGlobalWindowInner::CancelDocumentFlushedResolvers, and add observer if Promise callbacks adds document flushed resolvers.
(deleted), patch
mconley
: review-
bzbarsky
: review+
Details | Diff | Splinter Review 
In bug 1193394, I've disabled the following tests.
we should investigate the actual reason why they started failing.

  browser/base/content/test/permissions/browser_permissions.js
  browser/base/content/test/siteIdentity/browser_check_identity_state.js
  browser/base/content/test/webrtc/browser_devices_get_user_media.js
  browser/base/content/test/webrtc/browser_devices_get_user_media_in_frame.js
  browser/base/content/test/webrtc/browser_devices_get_user_media_screen.js
  browser/components/customizableui/test/browser_synced_tabs_menu.js
  browser/modules/test/browser/browser_BrowserUITelemetry_syncedtabs.js

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a54ef2d8f89626be6809676ff3534f077910052c&selectedJob=165136563
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a54ef2d8f89626be6809676ff3534f077910052c&selectedJob=165136511
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a54ef2d8f89626be6809676ff3534f077910052c&selectedJob=165136561

looks like it's something related to bug 1420939,
that modifies descriptionHeightWorkaround
Paolo, can you please take a look at this (and bug 1442187)?  It seems that the new PanelMultiView.jsm does not work fine with the new microtask handling.
Thanks!
Flags: needinfo?(paolo.mozmail)
disabled 2 more
  browser/base/content/test/webrtc/browser_devices_get_user_media_unprompted_access.js
  browser/components/extensions/test/browser/browser_ext_commands_execute_browser_action.js
(In reply to Tooru Fujisawa [:arai] from comment #2)
> disabled 2 more
>  
> browser/base/content/test/webrtc/
> browser_devices_get_user_media_unprompted_access.js
>  
> browser/components/extensions/test/browser/
> browser_ext_commands_execute_browser_action.js

I just realized both fail only on linux.
Summary: Enable tests that relies on descriptionHeightWorkaround → tests that relies on descriptionHeightWorkaround fails after bug 1193394
some note from fix attempt in bug 1193394.

either of the following call to promiseDocumentFlushed throws NS_ERROR_FAILURE for each test.
https://searchfox.org/mozilla-central/rev/61d400da1c692453c2dc2c1cf37b616ce13dea5b/browser/components/customizableui/PanelMultiView.jsm#1321,1340

waiting an event tick before that call solved failing tests, but caused other failures.
when testing with browser/base/content/test/permissions/browser_permissions.js,
the latter one fails in testCancelPermission and testPermissionHints.

https://searchfox.org/mozilla-central/rev/61d400da1c692453c2dc2c1cf37b616ce13dea5b/browser/components/customizableui/PanelMultiView.jsm#1340

because mIteratingDocumentFlushedResolvers is true.

https://searchfox.org/mozilla-central/rev/61d400da1c692453c2dc2c1cf37b616ce13dea5b/dom/base/nsGlobalWindowInner.h#1436-1439
>   // This flag keeps track of whether or not we're going through
>   // promiseDocumentFlushed resolvers. When true, promiseDocumentFlushed
>   // cannot be called.
>   bool                          mIteratingDocumentFlushedResolvers;

https://searchfox.org/mozilla-central/rev/61d400da1c692453c2dc2c1cf37b616ce13dea5b/dom/base/nsGlobalWindowInner.cpp#7403-7406
> already_AddRefed<Promise>
> nsGlobalWindowInner::PromiseDocumentFlushed(PromiseDocumentFlushedCallback& aCallback,
>                                             ErrorResult& aError)
> {
> ...
>   if (mIteratingDocumentFlushedResolvers) {
>     aError.Throw(NS_ERROR_FAILURE);
>     return nullptr;
>   }
If one of mDocumentFlushedResolvers contains MicroTask checkpoint,
it means that the Promise resolution handler is executed while mIteratingDocumentFlushedResolvers==true.

https://searchfox.org/mozilla-central/rev/61d400da1c692453c2dc2c1cf37b616ce13dea5b/dom/base/nsGlobalWindowInner.cpp#7455-7465
> void
> nsGlobalWindowInner::CallDocumentFlushedResolvers()
> {
>   MOZ_ASSERT(!mIteratingDocumentFlushedResolvers);
>   mIteratingDocumentFlushedResolvers = true;
>   for (const auto& documentFlushedResolver : mDocumentFlushedResolvers) {
>     documentFlushedResolver->Call();
>   }
>   mDocumentFlushedResolvers.Clear();
>   mIteratingDocumentFlushedResolvers = false;
> }

https://searchfox.org/mozilla-central/rev/61d400da1c692453c2dc2c1cf37b616ce13dea5b/dom/base/nsGlobalWindowInner.cpp#7467-7477
> void
> nsGlobalWindowInner::CancelDocumentFlushedResolvers()
> {
>   MOZ_ASSERT(!mIteratingDocumentFlushedResolvers);
>   mIteratingDocumentFlushedResolvers = true;
>   for (const auto& documentFlushedResolver : mDocumentFlushedResolvers) {
>     documentFlushedResolver->Cancel();
>   }
>   mDocumentFlushedResolvers.Clear();
>   mIteratingDocumentFlushedResolvers = false;
> }

I'll check how that happens, and also see if it's possible to leave the checkpoint without breaking other tests.
so, for failing cases in browser_permissions.js, 2 collectItems were queued to mDocumentFlushedResolvers,
and after executing the 2nd collectItems, the MicroTask checkpoint happens and the Promise resolution handler for the 1st case is executed there, while mIteratingDocumentFlushedResolvers==true, and fails at the 2nd promiseDocumentFlushed call.

because of there's no JS callstack for the promise, executing MicroTask checkpoint should be fine,
so I think we should leave the checkpoint somehow, without breaking other cases.
or, maybe we can stop invoking MicroTask checkpoint for each call by putting extra tier outside of mIteratingDocumentFlushedResolvers=true and mIteratingDocumentFlushedResolvers=false ?
since it's not web-exposed, tweaking the MicroTask checkpoint might be fine.
I'll check if putting `nsAutoMicroTask mt;` to nsGlobalWindowInner::CallDocumentFlushedResolvers() fixes.

smaug, how do you think about this approach?
Flags: needinfo?(bugs)
CCing mconly who introduced promiseDocumentFlushed and bz.
(In reply to Tooru Fujisawa [:arai] from comment #8)
> the following was the failed tests after the workaround:
> 
>   browser/components/extensions/test/browser/browser_ext_commands_execute_browser_action.js
>   browser/base/content/test/webrtc/browser_devices_get_user_media_tear_off_tab.js
>   browser/base/content/test/performance/browser_appmenu_reflows.js

browser_ext_commands_execute_browser_action.js was different one
here's current WIP patch.
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
The following failures are solved by the patch.

  browser/base/content/test/permissions/browser_permissions.js
  browser/base/content/test/siteIdentity/browser_check_identity_state.js
  browser/base/content/test/webrtc/browser_devices_get_user_media.js
  browser/base/content/test/webrtc/browser_devices_get_user_media_in_frame.js
  browser/base/content/test/webrtc/browser_devices_get_user_media_screen.js
  browser/base/content/test/webrtc/browser_devices_get_user_media_unprompted_access.js

The following still fail:

  browser/components/customizableui/test/browser_synced_tabs_menu.js
  browser/modules/test/browser/browser_BrowserUITelemetry_syncedtabs.js
  browser/components/extensions/test/browser/browser_ext_popup_background.js

I'll check the detail for them.
(In reply to Tooru Fujisawa [:arai] from comment #15)
> The following still fail:
> 
>   browser/components/customizableui/test/browser_synced_tabs_menu.js
>   browser/modules/test/browser/browser_BrowserUITelemetry_syncedtabs.js
>   browser/components/extensions/test/browser/browser_ext_popup_background.js

I am pretty sure the last one will/should be solved by bug 1442187. I think the first one will also be solved by that bug.  I see a suspicious code; "await document.getElementById("nav-bar").overflowable.show()"
yes, the following is fixed by bug 1442187 patch.
  browser/modules/test/browser/browser_BrowserUITelemetry_syncedtabs.js
  browser/components/extensions/test/browser/browser_ext_popup_background.js

unfortunately browser/components/customizableui/test/browser_synced_tabs_menu.js is still failing
for browser_permissions.js testCancelPermission case, 2 items in mDocumentFlushedResolvers are added in the following flow.

[the 1st item]

https://searchfox.org/mozilla-central/rev/61d400da1c692453c2dc2c1cf37b616ce13dea5b/browser/components/customizableui/PanelMultiView.jsm#1321
>   async descriptionHeightWorkaround(allowSyncReflows = false) {
> ...
>       await this.window.promiseDocumentFlushed(collectItems);

https://searchfox.org/mozilla-central/rev/61d400da1c692453c2dc2c1cf37b616ce13dea5b/browser/base/content/browser.js#8202
>   _createPermissionItem(aPermission) {
> ...
>       PanelView.forNode(this._identityPopupMainView)
>                .descriptionHeightWorkaround();

https://searchfox.org/mozilla-central/rev/61d400da1c692453c2dc2c1cf37b616ce13dea5b/browser/base/content/test/permissions/browser_permissions.js#102
> add_task(async function testCancelPermission() {
>   await BrowserTestUtils.withNewTab(PERMISSIONS_PAGE, async function() {
> ...
>     cancelButtons[0].click();


[the 2nd item]

https://searchfox.org/mozilla-central/rev/61d400da1c692453c2dc2c1cf37b616ce13dea5b/browser/components/customizableui/PanelMultiView.jsm#1321
>   async descriptionHeightWorkaround(allowSyncReflows = false) {
> ...
>       await this.window.promiseDocumentFlushed(collectItems);

https://searchfox.org/mozilla-central/rev/61d400da1c692453c2dc2c1cf37b616ce13dea5b/browser/base/content/browser.js#8202
>   _createPermissionItem(aPermission) {
> ...
>       PanelView.forNode(this._identityPopupMainView)
>                .descriptionHeightWorkaround();

https://searchfox.org/mozilla-central/rev/61d400da1c692453c2dc2c1cf37b616ce13dea5b/browser/base/content/test/permissions/browser_permissions.js#105
> add_task(async function testCancelPermission() {
>   await BrowserTestUtils.withNewTab(PERMISSIONS_PAGE, async function() {
> ...
>     cancelButtons[1].click();
the above patch introduces another issue that sub menu doesn't open :P
I'll investigate.

then, about browser_permissions.js testCancelPermission case,
the testcase clicks 2 buttons in the same event tick, that shouldn't happen in practice.
so, apart from the failure, I think the test needs to be fixed to wait before clicking the 2nd one.
if all other cases are the same issue, we could fix those tests, as a workaround for this bug.
Here's another fix, that just allows calling promiseDocumentFlushed inside callback, or MicroTask checkpoint for the call.
Attachment #8955367 - Attachment is obsolete: true
Modified testcase to expect the new behavior
Attachment #8955398 - Attachment is obsolete: true
Comment on attachment 8955403 [details] [diff] [review]
Call PromiseDocumentFlushedCallback immediately when iterating over existing callbacks.

Hey, Paolo, mconley and Florian.  I think promiseDocumentFlushed should allow to be resolved soon when we are already processing other promiseDocumentFlusheds instead of disallowing re-entrancy.  The reentrant calls seems to easily happen with the new micro task handling. 

Note that we'd like to land this change once one of you guys stamped r+.
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(bugs)
Attachment #8955403 - Flags: review?(paolo.mozmail)
Attachment #8955403 - Flags: review?(mconley)
Attachment #8955403 - Flags: review?(florian)
(In reply to Tooru Fujisawa [:arai] from comment #25)
> and try run on inbound with beta simulation
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=5c8c384feb24ef960905dd43c09616dc80caceec

oops, the try syntax was wrong :P

https://treeherder.mozilla.org/#/jobs?repo=try&revision=028e20e4e03b9f1496deb6f1e543bc3783c15ac2
(In reply to Tooru Fujisawa [:arai] from comment #27)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=028e20e4e03b9f1496deb6f1e543bc3783c15ac2&group_state=e
> xpanded&selectedJob=165454955
> browser_devices_get_user_media_tear_off_tab.js seems to be intermittent.

If it happens only stylo-disabled, it's not a big deal.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #28)
> (In reply to Tooru Fujisawa [:arai] from comment #27)
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=028e20e4e03b9f1496deb6f1e543bc3783c15ac2&group_state=e
> > xpanded&selectedJob=165454955
> > browser_devices_get_user_media_tear_off_tab.js seems to be intermittent.
> 
> If it happens only stylo-disabled, it's not a big deal.

thanks.
seems to be pre-existing (bug 1442390)
Comment on attachment 8955403 [details] [diff] [review]
Call PromiseDocumentFlushedCallback immediately when iterating over existing callbacks.

Review of attachment 8955403 [details] [diff] [review]:
-----------------------------------------------------------------

This seems reasonable to me, but I'm not comfortable reviewing changes in nsGlobalWindowInner.cpp, and you may need a review from a DOM peer for the Window.webidl change, even if it's just a comment change.

::: dom/base/test/browser_promiseDocumentFlushed.js
@@ +181,5 @@
>    assertFlushesRequired();
>  
>    let promise = window.promiseDocumentFlushed(() => {
> +    let s = "";
> +    s += "A";

nit: merge these 2 lines to: let s = "A";
Attachment #8955403 - Flags: review?(florian)
Comment on attachment 8955403 [details] [diff] [review]
Call PromiseDocumentFlushedCallback immediately when iterating over existing callbacks.

(In reply to Tooru Fujisawa [:arai] from comment #10)
> or, maybe we can stop invoking MicroTask checkpoint for each call by putting
> extra tier outside of mIteratingDocumentFlushedResolvers=true and
> mIteratingDocumentFlushedResolvers=false ?

I think this is the correct behavior - promiseDocumentFulshed is designed to coalesce all the callbacks together so that we don't invoke other unrelated code that may modify the DOM in the meantime.

If we just had one JavaScript object per window that kept an array of callbacks and then iterated over them, we wouldn't get a microtask checkpoint in-between anyways. This just happens to be implemented in a different language. I think it's totally fine to not have a microtask checkpoint here, even if the JavaScript stack is empty.
Attachment #8955403 - Flags: review?(paolo.mozmail)
Attachment #8955403 - Flags: review?(mconley)
Attachment #8955403 - Flags: review-
And by the way, thanks for figuring this out! I originally recommended moving all the Promise resolutions after the callback invocations, but I didn't realize that other code scheduled in advance could get in the way.
Also see bug 1441168, where we'd like to make all DOM modifications fail during the callbacks.
Blocks: 1434376
See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=1441168
thank you for reviewing!

(In reply to :Paolo Amadini from comment #31)
> Comment on attachment 8955403 [details] [diff] [review]
> Call PromiseDocumentFlushedCallback immediately when iterating over existing
> callbacks.
> 
> (In reply to Tooru Fujisawa [:arai] from comment #10)
> > or, maybe we can stop invoking MicroTask checkpoint for each call by putting
> > extra tier outside of mIteratingDocumentFlushedResolvers=true and
> > mIteratingDocumentFlushedResolvers=false ?
> 
> I think this is the correct behavior - promiseDocumentFulshed is designed to
> coalesce all the callbacks together so that we don't invoke other unrelated
> code that may modify the DOM in the meantime.

okay, I'll try that way.
actually I've tried that once, and it introduced other failure [1].
I'll look into them this time.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb5bcaaa88c8629fe6d3326a8f8be586d77181bd
(In reply to Tooru Fujisawa [:arai] from comment #34)
> actually I've tried that once, and it introduced other failure [1].

the issue was that, with that change (attachment 8955367 [details] [diff] [review]), submenu doesn't open for hamburger menu.
it stuck at the 2nd promiseDocumentFlushed there,
I think it's because the call is done inside the MicroTask after the loop,
and the promise isn't get resolved.

https://searchfox.org/mozilla-central/rev/61d400da1c692453c2dc2c1cf37b616ce13dea5b/browser/components/customizableui/PanelMultiView.jsm#934
>   async _transitionViews(previousViewNode, viewNode, reverse) {
> ...
>     if (reverse) {
> ...
>     } else if (viewNode.customRectGetter) {
> ...
>     } else {
> ...
>       viewRect = await window.promiseDocumentFlushed(() => {
>         return this._dwu.getBoundsWithoutFlushing(viewNode);
>       });
> ...
>     }
> ...
>     await window.promiseDocumentFlushed(() => {});
that should because CallDocumentFlushedResolvers won't be called for them.
I think we should repeat until there's no new mDocumentFlushedResolvers item there.
Here's the patch that coalesces MicroTask checkpoints and perform after the loop,
and keep looping as long as there's new item.

it solves all testcases in comments above, but now the following test fails,
because the test modifies the style inside Promise callback, that is performed inside MicroTask checkpoint.

https://searchfox.org/mozilla-central/rev/61d400da1c692453c2dc2c1cf37b616ce13dea5b/dom/base/test/browser_promiseDocumentFlushed.js#75

> add_task(async function test_basic() {
> ...
>   await window.promiseDocumentFlushed(() => {});
>   assertNoFlushesRequired();
> 
>   dirtyStyle();
>   assertFlushesRequired();
> 
>   await window.promiseDocumentFlushed(() => {});
>   assertNoFlushesRequired();
> ...
> });

maybe we should stop looping if flush is required.
(or perhaps we can prohibit such case?)
Attachment #8955403 - Attachment is obsolete: true
err, maybe that was wrong.
the promise is not resolved because mObservingDidRefresh is true at the point of calling PromiseDocumentFlushed inside MicroTask checkpoint,
and PostRefreshObserver is not added for them.

So, we should add it if there's new items and return, instead of keep looping.
fixed the patch to add observer if new items are added by Promise callback.
there's an exception that it keep looping when the window is being destroyed.

(we might have to create helper template, since CallDocumentFlushedResolvers and CancelDocumentFlushedResolvers have same structure.
Attachment #8955569 - Attachment is obsolete: true
try run with updated patch (with template helper function)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dbfc41b7bad98381ee4107abd96b658c80266ef

now bug 1439472 is backed out, so browser_ext_popup_select.js will fail there
(In reply to Tooru Fujisawa [:arai] from comment #41)
> now bug 1439472 is backed out, so browser_ext_popup_select.js will fail there

actually, I triggered only linux, and browser_ext_popup_select.js is enabled only on windows,
it won't appear there.
anyway, I'm about to disable browser_ext_popup_select.js on debug build, in bug 1439472
Comment on attachment 8955625 [details] [diff] [review]
Defer MicroTask checkpoint to the outside of loop in nsGlobalWindowInner::CallDocumentFlushedResolvers and nsGlobalWindowInner::CancelDocumentFlushedResolvers, and add observer if Promise callbacks adds document flushed resolvers.

okay, looks like this works :)

the summary of the change is:
  * enclose the loop for calling/canceling mDocumentFlushedResolvers with nsAutoMicroTask, so that the MicroTask checkpoint won't be performed for each function call, but all of them are coalesced to a checkpoint after the loop
  * if Promise callbacks in the MicroTask checkpoint after the loop calls promiseDocumentFlushed after modifying document, those are queued to mDocumentFlushedResolvers but observer is not added, so, detect the case and add observer
Attachment #8955625 - Flags: review?(paolo.mozmail)
Comment on attachment 8955625 [details] [diff] [review]
Defer MicroTask checkpoint to the outside of loop in nsGlobalWindowInner::CallDocumentFlushedResolvers and nsGlobalWindowInner::CancelDocumentFlushedResolvers, and add observer if Promise callbacks adds document flushed resolvers.

Review of attachment 8955625 [details] [diff] [review]:
-----------------------------------------------------------------

Hey arai,

I have some feedback for you. I understand that you and hiro are in a bit of a crunch here to try to get the microtask work in for 60. I'll be happy to consult with both of you over the next few days over Bugzilla, e-mail and IRC should you need further input.

::: dom/base/nsGlobalWindowInner.cpp
@@ +7471,5 @@
> +      mIteratingDocumentFlushedResolvers = false;
> +    }
> +
> +    // Leaving nsAutoMicroTask above will perform MicroTask checkpoint, and
> +    // Promise callbacks there may create mDocumentFlushedResolvers items.

If there _are_ new mDocumentFlushedResolver items, I think we should wait until the next DidRefresh to attempt to run them. At this point, since a microtask checkpoint occurred, we have _no_ guarantees that style and layout are still "untouched" and might not cause flushes when querying.

Could we, perhaps, move the RemovePostRefreshObserver logic from both FreeInnerObjects and DidRefresh into this helper function, so that we only remove the observer if the mDocumentFlushedResolver array is empty after the microtask checkpoint?

We might want to also protect against mDocumentFlushedResolver entries being added when callin CallDocumentFlushedResolvers from FreeInnerObjects - we definitely want to remove the observer when we do that, which means (I think) making PromiseDocumentFlushed return an error result if mInnerObjectsFreed is true.

Also, I need to stress, I'm not a DOM peer. I wrote promiseDocumentFlushed with coaching from bz and smaug. You might want to confer with them as well that my feedback makes sense.

@@ +7474,5 @@
> +    // Leaving nsAutoMicroTask above will perform MicroTask checkpoint, and
> +    // Promise callbacks there may create mDocumentFlushedResolvers items.
> +
> +    // If there's no new item, there's nothing to do here.
> +    if (!mDocumentFlushedResolvers.Length()) {

So here, what I'd suggest is that if there are mDocumentFlushedResolvers, we bail out, and wait for the next DidRefresh...

@@ +7480,5 @@
> +    }
> +
> +    // If there are new items, the observer is not added for them when calling
> +    // PromiseDocumentFlushed.  Add here and leave.
> +    if (mDoc) {

... otherwise, here we'd _remove_ the refresh observer.
Attachment #8955625 - Flags: review?(paolo.mozmail) → review-
bz, can you please take time to review this?

I did discuss with mconley on IRC, and he told me he is fine with fixing concerned parts he mentioned in 1442218 comment 45 in a follow up bug. 
So, I think we can land this change once a DOM peer thinks this change is good.
Flags: needinfo?(bzbarsky)
Comment on attachment 8955625 [details] [diff] [review]
Defer MicroTask checkpoint to the outside of loop in nsGlobalWindowInner::CallDocumentFlushedResolvers and nsGlobalWindowInner::CancelDocumentFlushedResolvers, and add observer if Promise callbacks adds document flushed resolvers.

>+    // If there are new items, the observer is not added for them when calling
>+    // PromiseDocumentFlushed.

Why not?  Can we fix that?

Seems to me that we should unset mObservingDidRefresh before the automicrotask goes out of scope and then we woudn't need this bit.

>+  template<bool call>
>+  void CallOrCancelDocumentFlushedResolvers();

This needs to be documented.
Flags: needinfo?(bzbarsky)
Attachment #8955625 - Flags: review+
Thank you for reviewing :)

already explained on IRC tho, commenting here as well for clarification.

(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #45)
> ::: dom/base/nsGlobalWindowInner.cpp
> @@ +7471,5 @@
> > +      mIteratingDocumentFlushedResolvers = false;
> > +    }
> > +
> > +    // Leaving nsAutoMicroTask above will perform MicroTask checkpoint, and
> > +    // Promise callbacks there may create mDocumentFlushedResolvers items.
> 
> If there _are_ new mDocumentFlushedResolver items, I think we should wait
> until the next DidRefresh to attempt to run them. At this point, since a
> microtask checkpoint occurred, we have _no_ guarantees that style and layout
> are still "untouched" and might not cause flushes when querying.

A new item inside mDocumentFlushedResolver itself means the style and layout
are *touched*.  since otherwise the callback should be called inside promiseDocumentFlushed and it won't be added to mDocumentFlushedResolver.

This patch waits until the next refresh on normal case, and loops only on the exceptional case that we cannot add observer, because mDoc or shell are not available, that may happen when window is being destroyed.  It loops even if violating the constraint that the callback won't be called on such situation, to resolve returned Promises, otherwise they will never resolved.


(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #47)
> Comment on attachment 8955625 [details] [diff] [review]
> Defer MicroTask checkpoint to the outside of loop in
> nsGlobalWindowInner::CallDocumentFlushedResolvers and
> nsGlobalWindowInner::CancelDocumentFlushedResolvers, and add observer if
> Promise callbacks adds document flushed resolvers.
> 
> >+    // If there are new items, the observer is not added for them when calling
> >+    // PromiseDocumentFlushed.
> 
> Why not?  Can we fix that?

Yeah, I'll file a bug to move this logic into PromiseDocumentFlushed.
Blocks: 1442824
> Yeah, I'll file a bug to move this logic into PromiseDocumentFlushed.

Why?  Again, you just need to unset mObservingDidRefresh before processing the promise reactions, no?
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #49)
> > Yeah, I'll file a bug to move this logic into PromiseDocumentFlushed.
> 
> Why?  Again, you just need to unset mObservingDidRefresh before processing
> the promise reactions, no?

yes, I meant that, sorry for unclear comment.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0add475b998c8e692db1ed12c698ba7c735c6515
Bug 1442218 - Defer MicroTask checkpoint to the outside of loop in nsGlobalWindowInner::CallDocumentFlushedResolvers and nsGlobalWindowInner::CancelDocumentFlushedResolvers, and add observer if Promise callbacks adds document flushed resolvers. r=bz
Component: Toolbars and Customization → DOM
Product: Firefox → Core
https://hg.mozilla.org/mozilla-central/rev/0add475b998c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: