Closed Bug 1276073 Opened 4 years ago Closed 4 years ago

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

Categories

(DevTools :: Debugger, defect)

defect
Not set

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: RyanVM, Assigned: ochameau)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 3 obsolete files)

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.
Flags: needinfo?(poirot.alex) → needinfo?(jlong)
> 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`.
Flags: needinfo?(jlong)
Assignee: nobody → poirot.alex
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.
Attached patch patch v1 (obsolete) — Splinter Review
It should also fix bug 1276081 and possibly various others...
Attached patch patch v2 (obsolete) — Splinter Review
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)
Attachment #8758046 - Flags: review?(jlong)
Attachment #8757992 - Attachment is obsolete: true
Note that I already tweaked initDebugger in bug 1080025.
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.
(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.
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #8758344 - Flags: review?(jlong)
Attachment #8758046 - Attachment is obsolete: true
Attachment #8758046 - Flags: review?(jlong)
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.
Attachment #8758344 - Flags: review?(jlong) → review+
https://hg.mozilla.org/integration/fx-team/rev/ef722ca61b23d3e8a41bd157701914fab4940205
Bug 1276073 - Ensure source is loaded and selected before running debugger tests. r=jlongster
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
Flags: needinfo?(poirot.alex)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/08084e9e30d2
Backed out changeset ef722ca61b23 for frequent timeouts like in browser_dbg_search-symbols.js
Attached patch patch v4Splinter Review
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.
Attachment #8758733 - Flags: review+
Attachment #8758344 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/034b45ace455de8b62f389fc0b940583d9c62ea6
Bug 1276073 - Ensure source is loaded and selected before running debugger tests. r=jlongster
https://hg.mozilla.org/mozilla-central/rev/034b45ace455
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Flags: needinfo?(poirot.alex)
Blocks: 1276081
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.