Closed
Bug 1442218
Opened 7 years ago
Closed 7 years ago
tests that relies on descriptionHeightWorkaround fails after bug 1193394
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: arai, Assigned: arai)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 5 obsolete files)
4.01 KB,
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
Comment 1•7 years ago
|
||
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)
Assignee | ||
Comment 2•7 years ago
|
||
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
Comment 3•7 years ago
|
||
(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.
Comment 4•7 years ago
|
||
browser/components/extensions/test/browser/browser_ext_popup_background.js also failed.
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=9683f24ff8ec5ba768c4a0a124d8439c228b2c8b&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&selectedJob=165196596
Assignee | ||
Updated•7 years ago
|
Summary: Enable tests that relies on descriptionHeightWorkaround → tests that relies on descriptionHeightWorkaround fails after bug 1193394
Assignee | ||
Comment 5•7 years ago
|
||
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.
Assignee | ||
Comment 6•7 years ago
|
||
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;
> }
Assignee | ||
Comment 7•7 years ago
|
||
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.
Assignee | ||
Comment 8•7 years ago
|
||
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
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=29e1fceaf48d9a09662a18789d034f3093c4b5e8&selectedJob=165155675
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=29e1fceaf48d9a09662a18789d034f3093c4b5e8&selectedJob=165155725
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=29e1fceaf48d9a09662a18789d034f3093c4b5e8&selectedJob=165155564
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=29e1fceaf48d9a09662a18789d034f3093c4b5e8&selectedJob=165155555
Assignee | ||
Comment 9•7 years ago
|
||
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.
Assignee | ||
Comment 10•7 years ago
|
||
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.
Assignee | ||
Comment 11•7 years ago
|
||
I'll check if putting `nsAutoMicroTask mt;` to nsGlobalWindowInner::CallDocumentFlushedResolvers() fixes.
smaug, how do you think about this approach?
Flags: needinfo?(bugs)
Comment 12•7 years ago
|
||
CCing mconly who introduced promiseDocumentFlushed and bz.
Assignee | ||
Comment 13•7 years ago
|
||
(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
Assignee | ||
Comment 14•7 years ago
|
||
here's current WIP patch.
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Assignee | ||
Comment 15•7 years ago
|
||
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.
Comment 16•7 years ago
|
||
(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()"
Assignee | ||
Comment 17•7 years ago
|
||
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
Assignee | ||
Comment 18•7 years ago
|
||
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();
Assignee | ||
Comment 19•7 years ago
|
||
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.
Assignee | ||
Comment 20•7 years ago
|
||
Here's another fix, that just allows calling promiseDocumentFlushed inside callback, or MicroTask checkpoint for the call.
Attachment #8955367 -
Attachment is obsolete: true
Assignee | ||
Comment 21•7 years ago
|
||
try run with the patch and bug 1442187's one
https://treeherder.mozilla.org/#/jobs?repo=try&revision=51c3cb736e418415fd44a239ab220c82fe62b991
Assignee | ||
Comment 22•7 years ago
|
||
Modified testcase to expect the new behavior
Attachment #8955398 -
Attachment is obsolete: true
Comment 23•7 years ago
|
||
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)
Assignee | ||
Comment 24•7 years ago
|
||
Assignee | ||
Comment 25•7 years ago
|
||
and try run on inbound with beta simulation
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c8c384feb24ef960905dd43c09616dc80caceec
Assignee | ||
Comment 26•7 years ago
|
||
(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
Assignee | ||
Comment 27•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=028e20e4e03b9f1496deb6f1e543bc3783c15ac2&group_state=expanded&selectedJob=165454955
browser_devices_get_user_media_tear_off_tab.js seems to be intermittent.
Comment 28•7 years ago
|
||
(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.
Assignee | ||
Comment 29•7 years ago
|
||
(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 30•7 years ago
|
||
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 31•7 years ago
|
||
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-
Comment 32•7 years ago
|
||
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.
Comment 33•7 years ago
|
||
Also see bug 1441168, where we'd like to make all DOM modifications fail during the callbacks.
Assignee | ||
Comment 34•7 years ago
|
||
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
Assignee | ||
Comment 35•7 years ago
|
||
(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.
Assignee | ||
Comment 36•7 years ago
|
||
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(() => {});
Assignee | ||
Comment 37•7 years ago
|
||
that should because CallDocumentFlushedResolvers won't be called for them.
I think we should repeat until there's no new mDocumentFlushedResolvers item there.
Assignee | ||
Comment 38•7 years ago
|
||
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
Assignee | ||
Comment 39•7 years ago
|
||
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.
Assignee | ||
Comment 40•7 years ago
|
||
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
Assignee | ||
Comment 41•7 years ago
|
||
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
Assignee | ||
Comment 42•7 years ago
|
||
merged into helper function.
Attachment #8955589 -
Attachment is obsolete: true
Assignee | ||
Comment 43•7 years ago
|
||
(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
Assignee | ||
Comment 44•7 years ago
|
||
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 45•7 years ago
|
||
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-
Comment 46•7 years ago
|
||
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 47•7 years ago
|
||
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+
Assignee | ||
Comment 48•7 years ago
|
||
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.
Comment 49•7 years ago
|
||
> 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?
Assignee | ||
Comment 50•7 years ago
|
||
(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.
Assignee | ||
Comment 51•7 years ago
|
||
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
Updated•7 years ago
|
Component: Toolbars and Customization → DOM
Product: Firefox → Core
Comment 52•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•