1205305 - Fix race conditions in the debugger tests.

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Eddy Bruel [:ejpbruel]]

Some of our debugger tests are racy, in the sense that they assume that the frontend uses synchronous promises.

For instance, we have tests that simulate a button click, but don't wait for that button click to be handled. This just happens to work with synchronous promises, since then all the handler code is called within a single tick of the event loop, but this is no longer true for async promises.

We need to fix these issues before we can switch the frontend to using asynchronous promises.

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: browser_dbg_server-conditional-bp-02.js

browser_dbg_server-conditional-bp-02.js simulates a breakpoint click, but doesn't wait for that click to be fully handled. This leads to problems with asynchronous promises.

To work around this, I've added a BREAKPOINT_CLICKED event that is fired when the breakpoint is handled. Note that this is still not completely safe as it does not wait for popups to be opened/hidden by conditional breakpoints. Since most of the breakpoint code will be refactored soon anyway, this is good enough for now.

Comment 2

[Eddy Bruel [:ejpbruel]]

Attachment: browser_dbg_search-symbols.js

browser_dbg_search-symbols.js simulates a button press, but does not wait for the editor to update its location in response before checking said location. This is fine when the button press is handled within a single tick of the event loop, but that will no longer be the case with async promises.

The solution is to explicitly wait for the EDITOR_LOCATION_SET event. Unfortunately, said event is emitted twice when setting the editor location also causes the displayed source to be update (see comment in the patch to that effect), so I needed some special case handling here.

Comment 3

[Eddy Bruel [:ejpbruel]]

Attachment: Fix a promise bug in debugger-controller.js

In debugger-controller.js, we set a property on deferred.promise in our pretty-printing code. This works fine with the deprecated-sync-thenables, but promise.jsm promises are non-extensible.

The solution I came up with is to wrap both the property and the promise in an object, and store that instead.

Comment 4

[Eddy Bruel [:ejpbruel]]

Attachment: browser_dbg_search-basic-02.js

The problem with this test is that we're putting some assertions in Promise.all that should always be run before the event promises within the same list are resolved. However, Promise.all does not offer such ordering guarantees: promises may be resolved in any order. Apparently, this causes the tests to break when using async promises.

I've rewritten the tests so that the intent is more clearly stated. I was originally going to let Victor rewrite these patches, so I refactored the tests using Task.js to make them easier for him to review.

Comment 5

[Eddy Bruel [:ejpbruel]]

Attachment: browser_dbg_search-basic-03.js

Essentially the same story as the previous patch.

Comment 6

[Eddy Bruel [:ejpbruel]]

Attachment: browser_dbg_blackboxing-04.js

This test assumes that toggleBlackboxing is a synchronous operation. It currently is, because we use sync promises, but that assumption is not safe, and will no longer hold when we switch to async promises.

Trying to toggle blackboxing while another toggle is still in process is not something we are prepared for, apparently, and causes the test to fail. The solution is simply to wait for each toggle to finish before triggering the next.

Comment 7

[Eddy Bruel [:ejpbruel]]

Try push for the browser_dbg_server-conditional-bp-02.js patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=51e341b5c5e2

Comment 8

[James Long (:jlongster)]

Comment on attachment 8661819 [details] [diff] [review]
browser_dbg_search-symbols.js

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

You can go ahead and land this one if you want, since there are no changes to the actual source code and I'm probably going to have to change this too.

Comment 9

[James Long (:jlongster)]

Comment on attachment 8661813 [details] [diff] [review]
browser_dbg_server-conditional-bp-02.js

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

Let's hold off on this one until my refactor lands. It'll be quite different.

Comment 10

[James Long (:jlongster)]

Comment on attachment 8661820 [details] [diff] [review]
Fix a promise bug in debugger-controller.js

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

This code is completely gone in my refactor, and I will have fixed async problems as well.

Comment 11

[James Long (:jlongster)]

Comment on attachment 8661824 [details] [diff] [review]
browser_dbg_search-basic-02.js

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

Looks fine to me, you can land individual test refactoring before I start touching the tests.

Comment 12

[Eddy Bruel [:ejpbruel]]

Try push for the browser_dbg_search-symbols.js patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e322457cfa69

Comment 13

[Eddy Bruel [:ejpbruel]]

New try push for the browser_dbg_server_conditional-bp-02.js patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea8c7397c7d6

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e0a91b61e9c2

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e0a91b61e9c2