1276073 - Intermittent browser_dbg_pretty-print-on-paused.js | Expected source was not already shown: code_script-switching-02.js

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

https://treeherder.mozilla.org/logviewer.html#?job_id=9577304&repo=fx-team#L2703

Comment 1

[Alexandre Poirot [:ochameau]]

James, Aren't the sources expected to be loaded in a deterministic order?
When a page loads multiples scripts via <script> tag, without defer nor anything special, the debugger is opening the same script, always?

It doesn't seem to be the case?

This test:
http://mxr.mozilla.org/mozilla-central/source/devtools/client/debugger/test/mochitest/browser_dbg_pretty-print-on-paused.js
http://mxr.mozilla.org/mozilla-central/source/devtools/client/debugger/test/mochitest/doc_pretty-print-on-paused.html?force=1

Loads two scripts:
    <script src="code_ugly-2.js"></script>
    <script src="code_script-switching-02.js"></script>

I'm expecting it to always end up opening the last one, code_script-switching-02.js, but these intermittents seem to prove it doesn't work like that. (sometimes it ends up on code_ugly-2.js)

If we can't make the debugger be deterministic about that, I'll have to tune initDebugger accordingly.

For now, I immediately assert the current source after SOURCE_SHOWN. Instead I would have to only see if we ended up on the expected one and if not, switch to it and may be wait for NEW_SOURCE if not already there.

Comment 2

[James Long (:jlongster)]

> When a page loads multiples scripts via <script> tag, without defer nor anything special, the debugger is opening the same script, always?

No, because SpiderMonkey may still introduce scripts in random order. Those scripts should block the HTML parsing and be executed in that order, but we get script notifications before they execute. For whatever reason, we'll get these notifications in various orders depending on how platform has optimized things. It made have gone ahead and downloaded those two scripts because it figured out it could, but not executed them until they can be executed in order. We'll get notifications based on how the low-level system creates these scripts, not how they are executed.

(You can probably tell I'm not exactly sure why, but simply put we can't depend on order)

All of the debugger tests should be written in a way that is deterministic, though. If you look at the sample code they use, all of them have `debugger` statements so we know that the state of the debugger after startup will be that it is paused at that location. Tests will wait for that paused state (i.e. that source is shown), resume it and go from there.

I'd be interested what changes you want to make to `initDebugger`.

Comment 3

[Intermittent Failures Robot]

5 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 4
* fx-team: 1

Platform breakdown:
* linux64: 5

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1276073&startday=2016-05-23&endday=2016-05-29&tree=all

Comment 4

[Alexandre Poirot [:ochameau]]

I'll do something similar to bug 1230990 and also what I did for browser_dbg_bug-896139.js:
http://mxr.mozilla.org/mozilla-central/source/devtools/client/debugger/test/mochitest/browser_dbg_bug-896139.js#35

The expected source may not even be loaded so that we would have to sometime wait for NEW_SOURCE.

Comment 5

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e08747c0f0d1

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

It should also fix bug 1276081 and possibly various others...

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

See comment 4. I apply some existing patterns to make all test more resiliant.
I had to ensure passing the full url in all tests in order to be able to
select the expected source if one another was selected by default.
(we need the url to create the source actor required to change the selected source)

Comment 8

[Alexandre Poirot [:ochameau]]

Note that I already tweaked initDebugger in bug 1080025.

Comment 9

[James Long (:jlongster)]

Comment on attachment 8758046 [details] [diff] [review]
patch v2

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

Thanks for looking at this. I hate how complicated our sources setup is, and yeah, our current tests are pretty fickle. The new debugger interface is going to avoid all of this mess.

I like the changes, but are you sure you can change all those tests? If I remember right, many of them wait for a specific source manually, so it might be waiting on a NEW_SOURCE event that has already been handled. Although maybe they check to see if the source is already selected first, so this will work. I'm assuming you've ran this locally so it probably works.

Had one question about the code, but the direction seems good.

::: devtools/client/debugger/test/mochitest/head.js
@@ +577,5 @@
>                                  url,
>                                  panelWin.EVENTS.SOURCE_SHOWN);
>      }
>      if (source) {
> +      let isSelected = Sources.selectedItem.attachment.source.url === source;

It might be worth extracting this logic into another helper function that tests can manually use. I know there are some places would use this instead of resorting to other tricks.

@@ +584,5 @@
> +          item.attachment.source.url === source);
> +        if (!isLoaded) {
> +          yield waitForDebuggerEvents(debuggerPanel, panelWin.EVENTS.NEW_SOURCE);
> +          // Wait for it to be added to the UI
> +          yield waitForTick();

I don't see how waiting for NEW_SOURCE works? You're just waiting for any new sources to be loaded. You don't check to make sure that the new source is the source you're looking for.

Comment 10

[Alexandre Poirot [:ochameau]]

(In reply to James Long (:jlongster) from comment #9)
> I like the changes, but are you sure you can change all those tests? If I
> remember right, many of them wait for a specific source manually, so it
> might be waiting on a NEW_SOURCE event that has already been handled.
> Although maybe they check to see if the source is already selected first, so
> this will work. I'm assuming you've ran this locally so it probably works.

There is only very few if any test that wait for NEW_SOURCE.
Most were waiting for SOURCE_SHOWN.
Note that I already refactored if not all, most debugger tests in bug 1080025, where I removed all the duplicated wait code from all test in order to factor this out into initDebugger!

I tested locally and got a green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a10f7fe204a1

> ::: devtools/client/debugger/test/mochitest/head.js
> @@ +577,5 @@
> >                                  url,
> >                                  panelWin.EVENTS.SOURCE_SHOWN);
> >      }
> >      if (source) {
> > +      let isSelected = Sources.selectedItem.attachment.source.url === source;
> 
> It might be worth extracting this logic into another helper function that
> tests can manually use. I know there are some places would use this instead
> of resorting to other tricks.

If you are refering to isSelected, `ensureSourceIs` does that and more.

> 
> @@ +584,5 @@
> > +          item.attachment.source.url === source);
> > +        if (!isLoaded) {
> > +          yield waitForDebuggerEvents(debuggerPanel, panelWin.EVENTS.NEW_SOURCE);
> > +          // Wait for it to be added to the UI
> > +          yield waitForTick();
> 
> I don't see how waiting for NEW_SOURCE works? You're just waiting for any
> new sources to be loaded. You don't check to make sure that the new source
> is the source you're looking for.

Right. This is a bit weak. It depends on the fact that it is already an intermittent.
In most case the source is already loaded. And when the intermittent happens, waiting for just the next NEW_SOURCE is enough.
I'm asserting for the source in the following code related to SOURCE_SHOWN.
But I'll change that to use a new waitForSourceLoaded, similar to waitForSourceShow but for SHOUCE_SHOWN.

Comment 11

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7880b4db1914

Comment 12

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

Comment 13

[James Long (:jlongster)]

Comment on attachment 8758344 [details] [diff] [review]
patch v3

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

You have my gratitude for cleaning this up! This looks great.

Comment 14

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/ef722ca61b23d3e8a41bd157701914fab4940205
Bug 1276073 - Ensure source is loaded and selected before running debugger tests. r=jlongster

Comment 15

[Carsten Book [:Tomcat]]

sorry had to back this out since this caused frequent timeouts like in https://treeherder.mozilla.org/logviewer.html#?job_id=9679318&repo=fx-team

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/08084e9e30d2
Backed out changeset ef722ca61b23 for frequent timeouts like in browser_dbg_search-symbols.js

Comment 17

[Alexandre Poirot [:ochameau]]

Attachment: patch v4

Oops. Looks like there is some debugger tests that do not run on Linux...
So I missed this one. It was always failing, but not on linux.

Comment 18

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8926bb17e58

Comment 19

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/034b45ace455de8b62f389fc0b940583d9c62ea6
Bug 1276073 - Ensure source is loaded and selected before running debugger tests. r=jlongster

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/034b45ace455

Comment 21

[Intermittent Failures Robot]

9 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 7
* mozilla-central: 1
* fx-team: 1

Platform breakdown:
* linux64: 9

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1276073&startday=2016-05-30&endday=2016-06-05&tree=all