1416446 - Fix Promise/Event handling in browser/base/content/test/general/browser_ctrlTab.js for bug 1193394

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Tooru Fujisawa [:arai]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc35b181a4a55c0bd3cca8a19346ff224545a649&selectedJob=143221687
> FAIL | browser/base/content/test/general/browser_ctrlTab.js | With 4 tabs open and tab 1 selected, Ctrl+Tab*2 goes 2 tabs back in most-recently-selected order
> Got 0, expected 2 Stack trace: chrome://mochikit/content/browser-test.js:test_is:1264 chrome://mochitests/content/browser/browser/base/content/test/general/browser_ctrlTab.js:ctrlTabTest:181
> FAIL | browser/base/content/test/general/browser_ctrlTab.js | With 4 tabs open and tab 0 selected, Ctrl+Tab*4 goes 0 tabs back in most-recently-selected order
> Got 1, expected 2 Stack trace: chrome://mochikit/content/browser-test.js:test_is:1264 chrome://mochitests/content/browser/browser/base/content/test/general/browser_ctrlTab.js:ctrlTabTest:181
> FAIL | browser/base/content/test/general/browser_ctrlTab.js | Ctrl+Tab -> Ctrl+Shift+Tab keeps the selected tab
> Got 0, expected 1 Stack trace: chrome://mochikit/content/browser-test.js:test_is:1264 chrome://mochitests/content/browser/browser/base/content/test/general/browser_ctrlTab.js:null:19
> FAIL | browser/base/content/test/general/browser_ctrlTab.js | Ctrl+Tab*2 -> Ctrl+W removes the second most recently selected tab

looks like the behavior around asynthesizing/waiting event seems to be changed.

Comment 1

[Tooru Fujisawa [:arai]]

waiting for next tick for each synthesis fixes the issue.

Comment 2

[Tooru Fujisawa [:arai]]

there are 2 solutions.

A. add the following to the testcase, and wait for it in pressCtrlTab, releaseCtrl, synthesizeCtrlW
  function waitForTick() {
    return new Promise(resolve => executeSoon(resolve));
  }
B. add the above to BrowserTestUtils and wait for it in BrowserTestUtils.waitForEvent

B may fix the other tests as well, but not sure if there's any unexpected case.

Comment 3

[Tooru Fujisawa [:arai]]

try run with B
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a256f95712e8844d888ec18891822e949bc85c6d

Comment 4

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #2)
> there are 2 solutions.
> B. add the above to BrowserTestUtils and wait for it in
> BrowserTestUtils.waitForEvent
> 
> B may fix the other tests as well, but not sure if there's any unexpected
> case.

if the testcase is going to wait for 2 events that is going to be dispatched continuously,
waiting for the next event tick before adding event listener for the 2nd event might miss the event.

here's ContentTaskUtils.waitForEvent's case (not BrowserTestUtils.waitForEvent)
https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/browser/components/preferences/in-content/tests/browser_subdialogs.js#23-33

Comment 5

[Tooru Fujisawa [:arai]]

this test fails differently after https://hg.mozilla.org/try/rev/2ba730be64c45bcbf8e1deacca4b5bdc3c1efe34
https://dxr.mozilla.org/mozilla-central/source/browser/components/search/test/browser_oneOffHeader.js

Comment 6

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #5)
> this test fails differently after
> https://hg.mozilla.org/try/rev/2ba730be64c45bcbf8e1deacca4b5bdc3c1efe34
> https://dxr.mozilla.org/mozilla-central/source/browser/components/search/
> test/browser_oneOffHeader.js

sorry, it seems to be something related to bug 1361276.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Wait for the next tick in BrowserTestUtils.waitForEvent.

In BrowserTestUtils.waitForEvent, the promise resolution/rejection handlers are supposed to be executed after all event handlers are executed and everything beome ready, but after bug 1193394, resolving/rejecting the promise in the event handler doesn't result in it.
instead, it needs to wait for the next event tick.

then, there are some case that way doesn't work,
especially when the testcase specifies `capture=true`,
in that case the resolution handler is expected to be executed earlier.
so, I left the previous behavior that resolves/rejects promise instantly, only if `capture==true`.
that almost works.

there's only one case the testcase needs to be fixed, in browser_bug703210.js.
flipping `capture=false` to `capture=true` solves it.

Comment 8

[:Paolo Amadini]

Comment on attachment 8929345 [details] [diff] [review]
Wait for the next tick in BrowserTestUtils.waitForEvent.

It makes sense that waitForEvent would give a chance to all the other registered event listeners to be executed before continuing. Please update the comment to that effect, since this assumption is now part of the interface. There should be a simple regression test that uses event listeners and waitForEvent, and verifies that the handlers are called in the right order.

With bug 1193394 solved, for the test to succeed we should wait for the next task, in other words a tick of the event loop, instead of just the next microtask. However, we should do this regardless of the "capture" parameter, which is irrelevant for microtask handling. If any tests in the tree have to be updated because of this timing change, it's a bug in the test, and the issue should be fixed there.

Comment 9

[Tooru Fujisawa [:arai]]

thank you for reviewing :)
I'll fix each testcase.

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Part 0.1: Fix DateTimeTestHelper.waitForPickerReady to wait for events in correct order and timing.

With updated BrowserTestUtils.waitForEvent, it waits for the next event tick,
so event handler for PickerReady should be added directly inside the load event handler, to avoid missing it.

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Part 0.2: Fix browser_981418-widget-onbeforecreated-handler.js to wait for events in correct order and timing.

Just like Part 0.1, popupshown event handler (inside promisePanelElementShown) should be added directly inside ViewShown event handler.

also renamed tempPanel to widgetPanel.

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Wait for the next tick in BrowserTestUtils.waitForEvent.

Now BrowserTestUtils.waitForEvent can just wait for the next event tick before resolving promise inside event handler.

Comment 13

[:Paolo Amadini]

Comment on attachment 8932816 [details] [diff] [review]
Part 0.1: Fix DateTimeTestHelper.waitForPickerReady to wait for events in correct order and timing.

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

Unfortunately, this code will always have a race condition because we can't access the dateTimePopupFrame before the first time we try to open the picker.

Please add a comment to say that this isn't a complete solution, and file a bug to fix this test properly.

Comment 14

[:Paolo Amadini]

Comment on attachment 8932817 [details] [diff] [review]
Part 0.2: Fix browser_981418-widget-onbeforecreated-handler.js to wait for events in correct order and timing.

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

A comment about why we are using this method would be useful, so we won't introduce intermettents if someone changes it back to use waitForEvent.

Comment 15

[:Paolo Amadini]

Comment on attachment 8932818 [details] [diff] [review]
Part 1: Wait for the next tick in BrowserTestUtils.waitForEvent.

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

Can you please add a dedicated test case under the "testing" folder?

Something like calling waitForEvent first, then registering another event handler that sets a boolean, and checking immediately that the boolean is set when waitForEvent resolves. Cross-check that this test fails with the correct Promise event ordering, unless we patch the waitForEvent function.

Comment 16

[:Paolo Amadini]

Ah, and update the waitForEvent description to explain this new guarantee.

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Wait for the next tick in BrowserTestUtils.waitForEvent.

Thank you for reviewing.

Added comment and testcase.

Comment 18

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #17)
> Created attachment 8934891 [details] [diff] [review]
> Part 1: Wait for the next tick in BrowserTestUtils.waitForEvent.
> 
> Thank you for reviewing.
> 
> Added comment and testcase.

forgot to mention that, the testcase fails if BrowserTestUtils.waitForEvent is not modified.

Comment 19

[:Paolo Amadini]

Comment on attachment 8934891 [details] [diff] [review]
Part 1: Wait for the next tick in BrowserTestUtils.waitForEvent.

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

Looks great, thanks!

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
@@ +736,5 @@
> +   * guaranteed not to be called until the next event tick after the event
> +   * listener gets called, so that all other event listeners for the element
> +   * are executed before the handler is executed.
> +   *
> +   *    let promiseEvent BrowserTestUtils.waitForEvent(element, "eventName");

typo: =

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b8f0fd1a9f1
Part 0.1: Fix DateTimeTestHelper.waitForPickerReady to wait for events in correct order and timing. r=Paolo
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ad560283f5e
Part 0.2: Fix browser_981418-widget-onbeforecreated-handler.js to wait for events in correct order and timing. r=Paolo
https://hg.mozilla.org/integration/mozilla-inbound/rev/44737c79883b
Part 1: Wait for the next tick in BrowserTestUtils.waitForEvent. r=Paolo

Comment 21

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b8f0fd1a9f19f586a44740541b2cd7e88ec30eb
Bug 1416446 - Part 0.1: Fix DateTimeTestHelper.waitForPickerReady to wait for events in correct order and timing. r=Paolo

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ad560283f5e53c49cd50510407d3560a003c6fd
Bug 1416446 - Part 0.2: Fix browser_981418-widget-onbeforecreated-handler.js to wait for events in correct order and timing. r=Paolo

https://hg.mozilla.org/integration/mozilla-inbound/rev/44737c79883b35ba848dc7ccfd5731d345619f64
Bug 1416446 - Part 1: Wait for the next tick in BrowserTestUtils.waitForEvent. r=Paolo

Comment 22

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/07c179b8383c
Backed out changeset 44737c79883b for failing browser-chrome's browser/base/content/test/permissions/browser_temporary_permissions_expiry.js on Windows 7 debug without e10s  r=backout on a CLOSED TREE
https://hg.mozilla.org/integration/mozilla-inbound/rev/32957bc1d493
Backed out changeset 8ad560283f5e for failing browser-chrome's browser/base/content/test/permissions/browser_temporary_permissions_expiry.js on Windows 7 debug without e10s  r=backout on a CLOSED TREE
https://hg.mozilla.org/integration/mozilla-inbound/rev/72a645dab5eb
Backed out changeset 6b8f0fd1a9f1 for failing browser-chrome's browser/base/content/test/permissions/browser_temporary_permissions_expiry.js on Windows 7 debug without e10s  r=backout on a CLOSED TREE

Comment 23

[Andreea Pavel [:apavel]]

Backed out changeset 56343b0de5c2 (bug 1424083) for failing browser-chrome's browser/base/content/test/permissions/browser_temporary_permissions_expiry.js on Windows 7 debug without e10s r=backout on a CLOSED TREE 

Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1c819b56fe973b077d64737db186b066850e1197

Failure log: https://hg.mozilla.org/integration/mozilla-inbound/rev/a73f5a62f971efb9afa3b2e70b5d01b447ba3ed3

Backouts: https://hg.mozilla.org/integration/mozilla-inbound/rev/07c179b8383c51db7a898d862ac6c2e625e045e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/32957bc1d4938d429c13734abedae15ddf8e52bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/72a645dab5eba9e07df5bd7bfdcadaa6f10aa7d1

Comment 24

[Tooru Fujisawa [:arai]]

Thanks, it's bug 1424595 that test harness catch the promise rejection after this patch, because of timing issue.
the rejection has been happening from before, just that the test harness haven't caught it.

Comment 25

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/af4dd1a620a2b85e628d382b896da7bffcac9aab
Bug 1416446 - Part 0.1: Fix DateTimeTestHelper.waitForPickerReady to wait for events in correct order and timing. r=Paolo

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc1f1628787ed0e2172ca02d06a7767807bba11a
Bug 1416446 - Part 0.2: Fix browser_981418-widget-onbeforecreated-handler.js to wait for events in correct order and timing. r=Paolo

https://hg.mozilla.org/integration/mozilla-inbound/rev/98df07d2aa855c80f82aaab1593b0ddc17c335cd
Bug 1416446 - Part 1: Wait for the next tick in BrowserTestUtils.waitForEvent. r=Paolo

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/af4dd1a620a2
https://hg.mozilla.org/mozilla-central/rev/cc1f1628787e
https://hg.mozilla.org/mozilla-central/rev/98df07d2aa85