Debugger mochitest helper `initDebugger` is racy :(

RESOLVED FIXED in Firefox 49

Status

()

Firefox
Developer Tools: Debugger
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: fitzgen, Assigned: ochameau)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 49
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

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.

Updated

3 years ago
Blocks: 1114553

Comment 2

2 years ago
Never, ever provide GC-sensitive APIs...
(Assignee)

Comment 3

2 years ago
Created attachment 8744908 [details] [diff] [review]
patch v1

A first batch. Not that trivial as we have to modify all tests.
(Assignee)

Comment 4

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e87fb9e6fc7f
(Assignee)

Comment 5

2 years ago
Created attachment 8748167 [details] [diff] [review]
patch v2

There is so many weakness in so many tests.
Another set of fixes... but still many broken tests.
(Assignee)

Updated

2 years ago
Attachment #8744908 - Attachment is obsolete: true
(Assignee)

Comment 6

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc28b9db17d5
(Assignee)

Comment 7

a year ago
Created attachment 8755818 [details] [diff] [review]
patch v3

Rebased after eslint fixes.
Let's see how many tests still fail.
(Assignee)

Updated

a year ago
Attachment #8748167 - Attachment is obsolete: true
(Assignee)

Comment 8

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6b98d6762ef
(Assignee)

Comment 9

a year ago
Created attachment 8755909 [details] [diff] [review]
patch v4

Should be mostly green this time.
Hopefully this is going to fix many intermittents and not introduce new ones!
(Assignee)

Updated

a year ago
Attachment #8755818 - Attachment is obsolete: true
(Assignee)

Comment 10

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4e14e6d8da1
(Assignee)

Comment 11

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c4aea5ca069
(Assignee)

Comment 12

a year ago
Created attachment 8756405 [details] [diff] [review]
patch v5

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

Updated

a year ago
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)

Updated

a year ago
Assignee: nobody → poirot.alex
(Assignee)

Comment 14

a year ago
(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?
(Assignee)

Comment 15

a year ago
Created attachment 8756437 [details] [diff] [review]
interdiff

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+
(Assignee)

Comment 17

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=56cf2703a222
(Assignee)

Comment 18

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3563f07e91d1
(Assignee)

Comment 19

a year ago
Created attachment 8756828 [details] [diff] [review]
patch v6

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+
(Assignee)

Updated

a year ago
Attachment #8756405 - Attachment is obsolete: true
Attachment #8756437 - Attachment is obsolete: true
(Assignee)

Comment 20

a year ago
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
(Assignee)

Comment 21

a year ago
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

Comment 22

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1af204777b4d
https://hg.mozilla.org/mozilla-central/rev/6a6155e3afa6
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
You need to log in before you can comment on or make changes to this bug.