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

RESOLVED FIXED in Firefox 67

Status

enhancement
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

unspecified
Firefox 67
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

3 months ago

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.)

Assignee

Comment 1

3 months ago

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);
Assignee

Updated

3 months ago
Flags: needinfo?(balbeza)
Assignee

Comment 3

3 months ago

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

Updated

2 months ago
Assignee: nobody → jorendorff
Assignee

Updated

2 months ago
Blocks: 1495072

Comment 5

2 months ago
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

Comment 6

2 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67

Updated

2 months ago
Flags: needinfo?(balbeza)
You need to log in before you can comment on or make changes to this bug.