Intermittent mochitest-browser-chrome browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | The correct script was loaded initially. - Didn't expect -1, but got it

RESOLVED FIXED in Firefox 14

Status

()

Firefox
Developer Tools: Debugger
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: RyanVM, Assigned: msucan)

Tracking

({intermittent-failure})

Trunk
Firefox 14
intermittent-failure
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
https://tbpl.mozilla.org/php/getParsedLog.php?id=9969190&tree=Mozilla-Inbound

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | The correct script was loaded initially. - Didn't expect -1, but got it
Stack trace:
    JS frame :: chrome://mochikit/content/browser-test.js :: test_isnot :: line 450
    JS frame :: chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js :: onScriptsAdded :: line 49
(Assignee)

Comment 1

5 years ago
I'm almost certain this is some kind of a duplicate of bug 728926 - both tests have the same problem.
(Reporter)

Comment 2

5 years ago
https://tbpl.mozilla.org/php/getParsedLog.php?id=9978563&tree=Mozilla-Inbound
(Reporter)

Updated

5 years ago
Whiteboard: [orange]
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(In reply to Mihai Sucan [:msucan] from comment #1)
> I'm almost certain this is some kind of a duplicate of bug 728926 - both
> tests have the same problem.

I think you are right, it looks like a race between updating the editor contents and the script list.
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Priority: -- → P2
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Assignee: nobody → mihai.sucan
Status: NEW → ASSIGNED
Comment hidden (Treeherder Robot)
(Assignee)

Comment 40

5 years ago
Created attachment 611412 [details] [diff] [review]
proposed fix

This patch adds a "Debugger:ScriptShown" event and makes changes to tests so they hopefully no longer fail.

I also fixed bug 736038 and bug 739316 with this patch.

Try server results:

https://tbpl.mozilla.org/?tree=Try&rev=2b341d5ea67e

Results seem good: none of the runs show failures in any of the three tests I attempted fixing.

I'm looking forward to your review - thank you!
Attachment #611412 - Flags: review?(rcampbell)
Comment hidden (Treeherder Robot)
Comment on attachment 611412 [details] [diff] [review]
proposed fix

in: /browser_dbg_stack-05.js

+
+  function runTest()
+  {
+    if (scriptShown && framesAdded) {
+      Services.tm.currentThread.dispatch({ run: testRecurse }, 0);
+    }
+  }

took me a second to figure out what's going on in this set of changes. Had to make sure this was still testing in the same way, i.e., recursively. I see now that the addOneTimeListener was added a few lines above. This is still essentially the same, and will timeout if the listener isn't caught.

This looks good. Was a little leery of passing around a showOptions parameter in each event, but I think the benefits of green tests outweighs any objections to that.
Attachment #611412 - Flags: review?(rcampbell) → review+
Whiteboard: [orange] → [orange][land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/0b426db13d90
Whiteboard: [orange][land-in-fx-team] → [orange][fixed-in-fx-team]
Comment hidden (Treeherder Robot)
https://hg.mozilla.org/mozilla-central/rev/61ec98df2dce
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [orange][fixed-in-fx-team] → [orange]
Target Milestone: --- → Firefox 14
https://hg.mozilla.org/mozilla-central/rev/0b426db13d90
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
This still fails intermittently.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Assignee)

Updated

5 years ago
Blocks: 744697
(Assignee)

Comment 56

5 years ago
Created attachment 615821 [details] [diff] [review]
moar fixes

This patches does more fixing for the debugger tests that have started to fail again.

Reason: the code refactoring for the UI changes things slightly - causing tests to get back to their usual intermittent failures. Mochitests are usually "flimsy".

Current problem: the ScriptShown event handler waits for when the desired script is shown, then it removes itself. This can happen to be during scriptsadded, before framesadded. By the time of framesadded the editor can show a different script than the desired one.

Solution: made the tests to wait more nicely for resumed which fires after scriptsadded (IIANM), then do the firstCall() and wait for framesadded. Tests now also wait for all ScriptShown events, so the scriptShown flag is always correct.

In SourceScripts._addScript() the editor.getCharCount() method is called sometimes before the editor completes initialization - this causes breakage. I made a change to avoid such cases.

Hopefully, the intermittent failures are fixed now. Try push:

https://tbpl.mozilla.org/?tree=Try&rev=9d3f867fb101

Looking forward to your review. Thank you!
Attachment #615821 - Flags: review?(rcampbell)
(In reply to Mihai Sucan [:msucan] from comment #56)
> Created attachment 615821 [details] [diff] [review]
> moar fixes
> 
> This patches does more fixing for the debugger tests that have started to
> fail again.
> 
> Reason: the code refactoring for the UI changes things slightly - causing
> tests to get back to their usual intermittent failures. Mochitests are
> usually "flimsy".
> 
> Current problem: the ScriptShown event handler waits for when the desired
> script is shown, then it removes itself. This can happen to be during
> scriptsadded, before framesadded. By the time of framesadded the editor can
> show a different script than the desired one.
> 
> Solution: made the tests to wait more nicely for resumed which fires after
> scriptsadded (IIANM), then do the firstCall() and wait for framesadded.
> Tests now also wait for all ScriptShown events, so the scriptShown flag is
> always correct.
> 
> In SourceScripts._addScript() the editor.getCharCount() method is called
> sometimes before the editor completes initialization - this causes breakage.
> I made a change to avoid such cases.
> 
> Hopefully, the intermittent failures are fixed now. Try push:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=9d3f867fb101
> 
> Looking forward to your review. Thank you!

I don't know if this was changed lately, but the sequence of events has always been paused -> framesadded -> scriptsadded -> resumed, with the caveat that scriptsadded is currently fired only in the first pause (but this will change soon). That said, these changes make sense to me, since waiting for all events should cater for corner cases with weird reordering of events.
(Assignee)

Comment 58

5 years ago
(In reply to Panos Astithas [:past] from comment #57)
> > Looking forward to your review. Thank you!
> 
> I don't know if this was changed lately, but the sequence of events has
> always been paused -> framesadded -> scriptsadded -> resumed, with the
> caveat that scriptsadded is currently fired only in the first pause (but
> this will change soon). That said, these changes make sense to me, since
> waiting for all events should cater for corner cases with weird reordering
> of events.

You mean the initial sequence of events? Or in general?

Interesting clarification, Panos. Thanks!

I see two ScriptShown events before I call content.firstCall(). Then framesadded when execution pauses at the debugger statement. Still, debug_tab_pane() from head.js abstracts some of the events away - it waits for resume before invoking the callback. So, your sequence of events can be right: paused > framesadded > scriptsadded > resumed > then the callback of the test.
Comment hidden (Treeherder Robot)
Attachment #615821 - Flags: review?(rcampbell) → review+
Comment on attachment 615821 [details] [diff] [review]
moar fixes

hiding for newer patch
Attachment #615821 - Attachment is obsolete: true
Whiteboard: [orange] → [orange][autoland:611412:-b do -p all -u mochitest-o -t none]
Comment hidden (Treeherder Robot)
Whiteboard: [orange][autoland:611412:-b do -p all -u mochitest-o -t none] → [autoland:611412:-b do -p all -u mochitest-o -t none]
Whiteboard: [autoland:611412:-b do -p all -u mochitest-o -t none] → [orange]
Attachment #615821 - Attachment is obsolete: false
Whiteboard: [orange] → [autoland:615821:-b do -p all -u mochitest-o -t none]
Whiteboard: [autoland:615821:-b do -p all -u mochitest-o -t none] → [orange]
(Assignee)

Updated

5 years ago
Whiteboard: [orange] → [orange][land-in-fx-team]
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
https://tbpl.mozilla.org/php/getParsedLog.php?id=11208805&tree=Mozilla-Beta
Comment hidden (Treeherder Robot)
(Assignee)

Comment 70

5 years ago
Created attachment 618631 [details] [diff] [review]
moar fixes - rebased

Rebased the patch and landed.

https://hg.mozilla.org/integration/fx-team/rev/455a266e6618

Hopefully this will fix several intermittent failures. Try results looked good.
Attachment #615821 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Whiteboard: [orange][land-in-fx-team] → [orange][fixed-in-fx-team]
Comment hidden (Treeherder Robot)
https://hg.mozilla.org/mozilla-central/rev/455a266e6618
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [orange][fixed-in-fx-team] → [orange]
Comment hidden (Treeherder Robot)
This patch was in a range which caused a Ts regression, so I backed out the whole range:

https://hg.mozilla.org/mozilla-central/rev/24a6a53c714a

Please reland after investigating and fixing the regression.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/integration/fx-team/rev/5fb5a30f1b8a
Whiteboard: [orange] → [orange][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/5fb5a30f1b8a
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [orange][fixed-in-fx-team] → [orange]
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Keywords: intermittent-failure
Whiteboard: [orange]
You need to log in before you can comment on or make changes to this bug.