Closed Bug 1525395 Opened 10 months ago Closed 9 months ago

Race condition in devtools/client/scratchpad/test/browser_scratchpad_recent_files.js

Categories

(DevTools :: Scratchpad, enhancement)

enhancement
Not set

Tracking

(firefox67 fixed)

RESOLVED FIXED
Firefox 67
Tracking Status
firefox67 --- fixed

People

(Reporter: jorendorff, Assigned: jorendorff)

References

Details

Attachments

(2 files)

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.

My current plan is to refactor it more than a bit.

I don't understand how the test ever worked. I think the idea was that each
operation would result in changes to the prefs, because those prefs are the
source of truth for the recent-files list. However, I don't understand why some
tests would not trigger multiple observer callbacks, which should have been a
huge mess.

The new code doesn't observe the prefs at all. Where possible, it waits for an
appropriate promise; in other places it uses sleep() to wait for the next
tick, relying on the Scratchpad implementation to be done reacting by then.

Since the original code was event-driven, most tests were split across two
functions. Each test function had the bottom half of one test and the top half
of the next test. The new code uses async/await and can therefore at least
group related functionality into single cohesive test functions. But those test
functions aren't as independent as they look -- most of them still depend on
previous tests to set up the expected starting state.

Depends on D20758

Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Pushed by jorendorff@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/af3e74a36fec
Part 1: Make Scratchpad.openFile return a promise. r=jimb
https://hg.mozilla.org/integration/autoland/rev/19d2e4ea75cf
Part 2: Rewrite a Scratchpad test to eliminate some race conditions. r=jimb

Amazing. The timeline is like this:

  • I made some changes to these two patches yesterday (using hg amend to commit them)
  • I uploaded those to phabricator with moz-phab submit .^ .
  • Then I pushed them to try server with mach try syntax

Try run (comment 4) came back clean. But the patches sent to the try server were not the ones in phabricator. The changes I had made were lost. And there was no trace of them in my local repository. I tried using hg log --hidden and hg obslog -r on the revisions that were sent to try server. No luck. I eventually had to pull them down from phabricator with arc patch.

👀 Feels bad.

Flags: needinfo?(jorendorff)

Of course, those changes did also have a real bug, which I think I've fixed now. Relanding.

Pushed by jorendorff@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0f09d2f3b19f
Part 1: Make Scratchpad.openFile return a promise. r=jimb
https://hg.mozilla.org/integration/autoland/rev/e647490b3655
Part 2: Rewrite a Scratchpad test to eliminate some race conditions. r=jimb
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
You need to log in before you can comment on or make changes to this bug.