Closed Bug 1080025 Opened 5 years ago Closed 3 years ago

Debugger mochitest helper `initDebugger` is racy :(

Categories

(DevTools :: Debugger, defect)

x86
macOS
defect
Not set

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: fitzgen, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 6 obsolete files)

Because the debugger's source list is GC-sensitive (bug 944260), `initDebugger(someURL)` is inherently racy. Its current steps taken are:

1) adds a tab and navigates it to the provided URL
2) waits for the tab's load event
3) opens the debugger

Between 2 and 3, if a GC happens and collects all the scripts for a given source, then the debugger won't include that source in its list of sources. Obviously we should fix bug 944260, but that might be a little ways off. In the meantime, this isn't a theoretical race that could be triggered in our tests, but who knows if it actually is: I spent way too much of my time on bug 1076830. Ugh.

We could avoid the race if `initDebugger(someURL)` took these steps:

1) add a blank tab
2) open the debugger
3) navigate to the provided URL
4) wait for the load event
In the new steps, the debugger would always have the source do the the newSource RDP events.
Blocks: dbg-test
Never, ever provide GC-sensitive APIs...
Attached patch patch v1 (obsolete) — Splinter Review
A first batch. Not that trivial as we have to modify all tests.
Attached patch patch v2 (obsolete) — Splinter Review
There is so many weakness in so many tests.
Another set of fixes... but still many broken tests.
Attachment #8744908 - Attachment is obsolete: true
Attached patch patch v3 (obsolete) — Splinter Review
Rebased after eslint fixes.
Let's see how many tests still fail.
Attachment #8748167 - Attachment is obsolete: true
Attached patch patch v4 (obsolete) — Splinter Review
Should be mostly green this time.
Hopefully this is going to fix many intermittents and not introduce new ones!
Attachment #8755818 - Attachment is obsolete: true
Attached patch patch v5 (obsolete) — Splinter Review
Nick, would you be able to kindly review this patch?
It took me some time to improve every single test to be non-racy.
We now wait correctly for sources and also for caret position,
as well as scopes if a breakpoint is expected to be hit.

initDebugger now expects additional parameters to control that.

I got rid of browser_dbg_reload-preferred-script-01.js as the prepareDebugger trick
is not reliable and no longer works.
It's better to load the debugger with the natural prefered script and then switch
to the one you need for your test.

The improvements from initDebugger allows to get rid of many:
  - ensureIsSource() [replace by `source` option]
  - waitForSource*() [we now always wait for source to be loaded, unless `noSource` is given]
  - waitForCaret*() [if `line` is given we wait for expected caret position]

There is a common pattern that I didn't factored into initDebugger,
where, right after loading the panel and document, we call a content function or click on an element,
that to trigger a debugger statement.
Like this:
  let onCaretUpdated = waitForCaretAndScopes(gPanel, 17);
  callInTab(gTab, "ermahgerd");
  yield onCaretUpdated;

It may be worth having an helper for this.
You will notice that sometimes, I'm using waitForCaretAndScopes and some others waitForSourceAndCaretAndScopes.
I'm using the later one when we load a new source, whereas I use the first one when we hit a debugger statement or breakpoint on the same source.

We may kill a bunch of intermittents with this patch!!
Attachment #8756405 - Flags: review?(nfitzgerald)
Attachment #8755909 - Attachment is obsolete: true
Comment on attachment 8756405 [details] [diff] [review]
patch v5

Review of attachment 8756405 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for digging into this!

I went through about a third of the test changes, but all the ones I looked at were pretty mechanical, so I focused on the initDebugger changes themselves instead.

r=me with minor nits

Thanks again!

::: devtools/client/debugger/test/mochitest/head.js
@@ +525,5 @@
> + * @param {Object} options
> + *   Set of optional arguments:
> + *   - {Boolean} noSource
> + *     If true, do not expect any source to be loaded, so skip sources wait.
> + *   - {String} source

Can't the `noSource` option be replaced with `source === null || source === undefined`? That would be more intuitive to me.

@@ +529,5 @@
> + *   - {String} source
> + *     If given, assert the default loaded source once the debugger is loaded.
> + *     This string can be partial to only match a part of the source name.
> + *   - {String} line
> + *     If given, wait for the caret to be set on a precise line

String? Shouldn't this be an integer?

@@ +543,5 @@
> +  if (urlOrTab instanceof XULElement) {
> +    // Is a Tab
> +    tab = urlOrTab;
> +  } else {
> +    // open an empty document first

Nit: use capitalization, full sentences, and puncuation for code comments.
Attachment #8756405 - Flags: review?(nfitzgerald) → review+
Assignee: nobody → poirot.alex
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #13)
> ::: devtools/client/debugger/test/mochitest/head.js
> @@ +525,5 @@
> > + * @param {Object} options
> > + *   Set of optional arguments:
> > + *   - {Boolean} noSource
> > + *     If true, do not expect any source to be loaded, so skip sources wait.
> > + *   - {String} source
> 
> Can't the `noSource` option be replaced with `source === null || source ===
> undefined`? That would be more intuitive to me.

It would allow some test to silently not wait for SOURCE_SHOWN event.
With the current patch, we now always wait for it, unless you pass the very explicit noSource.
I would like to keep things safe and explicit.
One option could be to check for `source === null` and only that. So that we would explicitely pass `{ source: null }`. It isn't 100% intuitive but it would be safe.
Would you prefer that over noSource? Or do you have other safe options in mind?
Attached patch interdiff (obsolete) — Splinter Review
Addressing preview comments and going with source===null
Attachment #8756437 - Flags: review?(nfitzgerald)
Comment on attachment 8756437 [details] [diff] [review]
interdiff

Review of attachment 8756437 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, this is more intuitive to me. Thanks!
Attachment #8756437 - Flags: review?(nfitzgerald) → review+
Attached patch patch v6Splinter Review
Merged and rebased.
Also fixed browser_dbg_bug-896139.js intermittent on linux32.

This patch may change the behavior of existing intermittents
by converting timeout without much informations, or random assertion failures into:
"Expected source was not already shown: XXXXXXXXX"
Attachment #8756828 - Flags: review+
Attachment #8756405 - Attachment is obsolete: true
Attachment #8756437 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/1af204777b4ddb868882d972663a13cbb324ea0b
Bug 1080025 - Prevent debugger tests to race source shown event when using `initDebugger` helper. r=fitzgen
https://hg.mozilla.org/integration/fx-team/rev/6a6155e3afa6148ef93255e6d9e126c2974f0319
Bug 1080025 - Fix browser_dbg_bug-896139.js on Linux-asan. CLOSED TREE r=me
Depends on: 1276073
https://hg.mozilla.org/mozilla-central/rev/1af204777b4d
https://hg.mozilla.org/mozilla-central/rev/6a6155e3afa6
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.