Closed Bug 1205305 Opened 6 years ago Closed 6 years ago

Fix race conditions in the debugger tests.


(DevTools :: Debugger, defect)

Not set


(firefox44 fixed)

Firefox 44
Tracking Status
firefox44 --- fixed


(Reporter: ejpbruel, Assigned: ejpbruel)




(6 files)

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.
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.
Attachment #8661813 - Flags: review?(jlong)
Attachment #8661813 - Attachment description: patch → browser_dbg_server-conditional-bp-02.js
Attachment #8661813 - Attachment is patch: true
Attachment #8661813 - Attachment mime type: text/x-patch → text/plain
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.
Attachment #8661819 - Flags: review?(jlong)
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.
Attachment #8661820 - Flags: review?(jlong)
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.
Attachment #8661824 - Flags: review?(jlong)
Essentially the same story as the previous patch.
Attachment #8661825 - Flags: review?(jlong)
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.
Attachment #8661830 - Flags: review?(jlong)
Try push for the browser_dbg_server-conditional-bp-02.js patch:
Comment on attachment 8661819 [details] [diff] [review]

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.
Attachment #8661819 - Flags: review?(jlong) → review+
Comment on attachment 8661813 [details] [diff] [review]

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

Let's hold off on this one until my refactor lands. It'll be quite different.
Attachment #8661813 - Flags: review?(jlong) → review-
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.
Attachment #8661820 - Flags: review?(jlong) → review-
Comment on attachment 8661824 [details] [diff] [review]

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

Looks fine to me, you can land individual test refactoring before I start touching the tests.
Attachment #8661824 - Flags: review?(jlong) → review+
Attachment #8661825 - Flags: review?(jlong) → review+
Attachment #8661830 - Flags: review?(jlong) → review+
Try push for the browser_dbg_search-symbols.js patch:
New try push for the browser_dbg_server_conditional-bp-02.js patch:
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.