Closed Bug 1533574 Opened 6 years ago Closed 6 years ago

Still a race condition in devtools/client/aboutdebugging-new/test/browser/browser_aboutdebugging_telemetry_runtime_updates.js

Categories

(DevTools :: about:debugging, enhancement)

enhancement
Not set
normal

Tracking

(firefox67 fixed)

RESOLVED FIXED
Firefox 67
Tracking Status
firefox67 --- fixed

People

(Reporter: jorendorff, Assigned: jorendorff)

References

Details

Attachments

(1 file)

In bug 1495072, I'm changing await to be about 2x faster (each await currently creates an extra Promise, for no good reason -- it's redundant -- and the change is to stop doing that).

This breaks some tests, including this one.

The issue seems to be that the UI updates (and therefore removeUsbRuntime thinks it's done) before all Telemetry events have been recorded; making await faster alters the order of events enough to make the race condition bite.

(Note: This was previously reported in bug 1530833, and a patch was landed there, but that doesn't fix the issue for me.)

This patch makes it pass again, for me locally, but—well, it's not a very good fix, obviously...

diff --git a/devtools/client/aboutdebugging-new/test/browser/browser_aboutdebugging_telemetry_runtime_updates.js b/dev
--- a/devtools/client/aboutdebugging-new/test/browser/browser_aboutdebugging_telemetry_runtime_updates.js
+++ b/devtools/client/aboutdebugging-new/test/browser/browser_aboutdebugging_telemetry_runtime_updates.js
@@ -89,16 +89,17 @@ add_task(async function testUsbRuntimeUp
   // for all the events related to RUNTIME_2.
   const runtime2Id = evts.filter(e => e.method === "runtime_added")[0].extras.runtime_id;
   const runtime2Extras = Object.assign({}, RUNTIME_2_EXTRAS, {
     "runtime_id": runtime2Id,
   });
 
   info("Remove runtime 1");
   await removeUsbRuntime(USB_RUNTIME_1, mocks, document);
+  await Promise.resolve(0);
 
   checkTelemetryEvents([
     { method: "runtime_disconnected", extras: runtime1ConnectedExtras },
     { method: "runtime_removed", extras: runtime1Extras },
   ], sessionId);
 
   info("Remove runtime 2");
   await removeUsbRuntime(USB_RUNTIME_2, mocks, document);
Flags: needinfo?(balbeza)

I really want to land the new faster await for Firefox 67, so I'm going to post a patch here, along the lines of comment 1, to work around this for the time being. That's what we did in bug 1525395, for another devtools test that was waiting for the UI to update.

I wish I knew a better way to do it, but a just slightly quirky test shouldn't hold up this big performance win...

Assignee: nobody → jorendorff
Blocks: 1495072
Pushed by jorendorff@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a8d439676de0 Work around a race condition in an about:debugging test. r=jlast
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
Flags: needinfo?(balbeza)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: