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

Status: RESOLVED FIXED

(DevTools :: about:debugging, enhancement)

Description

[Jason Orendorff [:jorendorff]]

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

Comment 1

[Jason Orendorff [:jorendorff]]

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

Comment 2

[Jason Orendorff [:jorendorff]]

Example failure:

https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception%2Csuccess%2Cretry%2Crunning%2Cpending%2Crunnable&revision=3cdb3dfd1ad7813452eaaee9dbcf859a660967b5&selectedJob=232653666

Comment 3

[Jason Orendorff [:jorendorff]]

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

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1533574 - Work around a race condition in an about:debugging test. r?ladybenko

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8d439676de0
Work around a race condition in an about:debugging test. r=jlast

Comment 6

[Bogdan Tara[:bogdan_tara | bogdant]]

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