Closed Bug 734641 Opened 12 years ago Closed 12 years ago

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

Categories

(DevTools :: Debugger, defect, P2)

Product:
DevTools
Component:
Debugger
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 14

People

(Reporter: RyanVM, Assigned: msucan)

References

Details

(Keywords: intermittent-failure)

Attachments

(2 files, 1 obsolete file)

proposed fix
19.25 KB, patch
rcampbell
: review+
Details | Diff | Splinter Review
moar fixes - rebased
9.97 KB, patch
Details | Diff | Splinter Review 
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
I'm almost certain this is some kind of a duplicate of bug 728926 - both tests have the same problem.
Whiteboard: [orange]
(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.
Priority: -- → P2
Assignee: nobody → mihai.sucan
Status: NEW → ASSIGNED
Attached patch proposed fix — — Splinter Review 
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 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]
https://hg.mozilla.org/mozilla-central/rev/61ec98df2dce
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [orange][fixed-in-fx-team] → [orange]
Target Milestone: --- → Firefox 14
This still fails intermittently.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 744697
Attached patch moar fixes (obsolete) — — Splinter Review 
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.
(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.
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]
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]
Whiteboard: [orange] → [orange][land-in-fx-team]
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
Whiteboard: [orange][land-in-fx-team] → [orange][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/455a266e6618
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Whiteboard: [orange][fixed-in-fx-team] → [orange]
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
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Whiteboard: [orange][fixed-in-fx-team] → [orange]
Whiteboard: [orange]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: