Closed
Bug 1423008
Opened 7 years ago
Closed 7 years ago
Wait for the next event tick before resolving promise in event handler in EventEmitter.promise.once and waitForNEvents
Categories
(DevTools :: General, defect, P3)
DevTools
General
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: arai, Assigned: arai)
References
Details
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a005fb8aae235b850857ce9954c81f2f6e4ffc41&selectedJob=149019744
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a005fb8aae235b850857ce9954c81f2f6e4ffc41&selectedJob=149019822
EventEmitter.prototype.once in devtools/shared/old-event-emitter.js needs to wait for the next event tick before resolving the promise inside event handler,
to avoid the remaining part of the test runs too early.
also, another implementation of once uses waitForNEvents in devtools/client/framework/test/shared-head.js,
that also has to wait for the next event tick, for same reason.
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Comment 1•7 years ago
|
||
there are several `once` implementation in tree, and at least some of them are used in non-testing code.
So the change will affect so many part of devtools.
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 2•7 years ago
|
||
delaying Promise resolution in `once` in old-event-emitter.js fixed the following tests failure:
* devtools/client/webconsole/test/browser_console_keyboard_accessibility.js
* devtools/client/webconsole/test/browser_webconsole_clear_method.js
* devtools/client/inspector/boxmodel/test/browser_boxmodel_guides.js
now checking the new failure caused by the change.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=83a58e011b64ba3e68b849c4cdab3e0c6ad93372
also, here's the rough list of `once` functions that returns promise, including outside of devtools.
I wonder if it's better aligning the behavior across them...
https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/devtools/shared/old-event-emitter.js#75
https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/devtools/shared/async-utils.js#53
https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/devtools/client/commandline/test/helpers.js#272
https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/devtools/server/tests/browser/browser_canvasframe_helper_04.js#59
https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/devtools/client/debugger/new/debugger.js#19793
https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/devtools/client/debugger/new/debugger.js#45269
https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/devtools/client/performance/test/helpers/event-utils.js#20
https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/dom/browser-element/mochitest/browserElement_Find.js#17
https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/dom/media/mediasource/test/mediasource.js#64
https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/dom/media/test/eme.js#442
https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/dom/media/test/manifest.js#1579
https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/toolkit/modules/EventEmitter.jsm#92
Assignee | ||
Comment 3•7 years ago
|
||
here are the new failure
[dt1]
TEST-UNEXPECTED-FAIL | devtools/client/inspector/test/browser_inspector_breadcrumbs_mutations.js | Test timed out -
[dt2]
TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser_webconsole_completion.js | 'document.getElem' completion - Got entById, expected entsByTagNameNS
TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser_webconsole_completion.js | 'document.getElem' another tab completion - Got entsByTagNameNS, expected entsByTagName
TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser_webconsole_completion.js | 'document.getElem' completion - Got entById, expected entsByTagNameNS
[dt3]
TEST-UNEXPECTED-FAIL | devtools/client/inspector/markup/test/browser_markup_mutation_02.js | Test timed out -
[dt4]
TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_cycle-angle.js | Angle displayed as a turn value again. - Got 360deg, expected 1turn
TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_cycle-color.js | Color displayed as an authored value. - Got hsl(0, 100%, 50%), expected #f00
TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_cycle-color.js | Color displayed as an HSL value again. - Got rgb(255, 0, 0), expected hsl(0, 100%, 50%)
[dt6]
TEST-UNEXPECTED-FAIL | devtools/client/framework/test/browser_toolbox_window_title_changes.js | Test timed out -
TEST-UNEXPECTED-FAIL | devtools/client/framework/test/browser_toolbox_window_title_frame_select.js | Test timed out -
[dt7]
TEST-UNEXPECTED-FAIL | devtools/client/webconsole/new-console-output/test/mochitest/browser_jsterm_completion.js | 'document.getElem' completion - Got entById, expected entsByTagNameNS
TEST-UNEXPECTED-FAIL | devtools/client/webconsole/new-console-output/test/mochitest/browser_jsterm_completion.js | 'document.getElem' another tab completion - Got entsByTagNameNS, expected entsByTagName
TEST-UNEXPECTED-FAIL | devtools/client/webconsole/new-console-output/test/mochitest/browser_jsterm_completion.js | 'document.getElem' completion - Got entById, expected entsByTagNameNS
Assignee | ||
Comment 4•7 years ago
|
||
now I'm wondering, if it's better keeping the implementations not to wait for the next event tick, and adding `await waitForTick()` to caller side. so that the impact is smaller.
since waiting for the next event tick only in some impls maybe confusing,
and modifying all impls may be too much troublesome (and may delay the bug 1193394 fix)
I'll see if adding `await waitForTick()` to caller side can solve all failures.
Assignee | ||
Comment 5•7 years ago
|
||
now testing with small workarounds
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a8507efacd89af458c62a05a19725009c3cc768&group_state=expanded
if it works, we can defer the `once` API design decision and land bug 1193394 fix sooner.
Assignee | ||
Comment 6•7 years ago
|
||
forgot one more fix.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ecce716a4ea50bbb6b7219fe8cf12c202795a78
and checking other envs as well.
Assignee | ||
Comment 7•7 years ago
|
||
with the last one fixed by bug 1430383, I think we can avoid touching `once` implementation for now.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•