Closed Bug 846606 Opened 11 years ago Closed 11 years ago

Intermittent browser_dbg_bug723069_editor-breakpoints.js | Test timed out | correct number of editor breakpoint changes - Got 3, expected 4

Categories

(DevTools :: Debugger, defect)

Product:
DevTools
Component:
Debugger
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 22

People

(Reporter: RyanVM, Assigned: msucan)

Details

(Keywords: intermittent-failure)

Attachments

(3 files, 1 obsolete file)

fix for permaorange (fx-team)
2.81 KB, patch
Details | Diff | Splinter Review
use sendMouseEventToWindow()
1.82 KB, patch
vporof
: review+
Details | Diff | Splinter Review
use mozRequestAnimationFrame() - version 2
3.10 KB, patch
vporof
: review+
Details | Diff | Splinter Review 
https://tbpl.mozilla.org/php/getParsedLog.php?id=20197733&tree=Mozilla-Inbound

Rev5 MacOSX Mountain Lion 10.8 mozilla-inbound opt test mochitest-browser-chrome on 2013-02-28 14:36:10 PST for push a8c4125b0e5a
slave: talos-mtnlion-r5-008

14:46:25     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | first script location is not the currently selected script
14:46:25     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | breakpoint2 added, client received
14:46:25     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | breakpoint2 added without errors
14:46:25     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | breakpoint2 client url is correct
14:46:25     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | breakpoint2 client line is correct
14:46:25     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | breakpoint2 client found in the list of debugger breakpoints
14:46:25     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | one breakpoint in the debugger
14:46:25     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | getBreakpoint(locations[0], 5) returns the correct breakpoint
14:46:25     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | switch to the second script
14:46:25     INFO -  DBG-FRONTEND: Setting the DebuggerView editor source: http://example.com/browser/browser/devtools/debugger/test/test-script-switching-01.js, loaded: true, options: ({})
14:46:25     INFO -  DBG-FRONTEND: Setting the DebuggerView editor mode: http://example.com/browser/browser/devtools/debugger/test/test-script-switching-01.js, content type:
14:46:25     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | The second script is no longer displayed.
14:46:25     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | The first script is displayed.
14:46:25     INFO -  DBG-FRONTEND: Updating the DebuggerView editor: http://example.com/browser/browser/devtools/debugger/test/test-script-switching-02.js @ 6, flags: ({noSwitch:true})
14:46:25     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | breakpoint2 added to the editor
14:46:25     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | one breakpoint added to the editor
14:46:25     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | no breakpoint was removed from the editor
14:46:25     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | editor breakpoint line is correct
14:46:25     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | editor.getBreakpoints().length is correct
14:46:25     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | remove the second breakpoint using the mouse
14:46:25     INFO -  DBG-FRONTEND: Updating the DebuggerView editor: http://example.com/browser/browser/devtools/debugger/test/test-script-switching-02.js @ 6, flags: ({noSwitch:true})
14:46:25     INFO -  DBG-FRONTEND: Updating the DebuggerView editor: http://example.com/browser/browser/devtools/debugger/test/test-script-switching-02.js @ 6, flags: ({noSwitch:true})
14:46:25     INFO -  DBG-FRONTEND: Updating the DebuggerView editor: http://example.com/browser/browser/devtools/debugger/test/test-script-switching-02.js @ 6, flags: ({noSwitch:true})
14:46:25     INFO -  DBG-FRONTEND: Updating the DebuggerView editor: http://example.com/browser/browser/devtools/debugger/test/test-script-switching-02.js @ 6, flags: ({noSwitch:true})
14:46:54  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | Test timed out
14:46:54  WARNING -  This is a harness error.
14:46:54     INFO -  args: ['/usr/sbin/screencapture', '-C', '-x', '-t', 'png', '/var/folders/ps/4zmxp1m54bqdghpn6_s57fph00000w/T/mozilla-test-fail_pRbOVE']
14:46:54     INFO -  libpng warning: zero length keyword
14:46:54     INFO -  libpng warning: Empty language field in iTXt chunk
14:46:56     INFO -  SCREENSHOT: <see log>
14:46:57     INFO -  DBG-FRONTEND: Destroying the DebuggerView
14:46:57     INFO -  DBG-FRONTEND: Destroying the ToolbarView
14:46:57     INFO -  DBG-FRONTEND: Destroying the OptionsView
14:46:57     INFO -  DBG-FRONTEND: Destroying the ChromeGlobalsView
14:46:57     INFO -  DBG-FRONTEND: Destroying the SourcesView
14:46:57     INFO -  DBG-FRONTEND: Destroying the FilterView
14:46:57     INFO -  DBG-FRONTEND: Destroying the FilteredSourcesView
14:46:57     INFO -  DBG-FRONTEND: Destroying the StackFramesView
14:46:57     INFO -  DBG-FRONTEND: Destroying the BreakpointsView
14:46:57     INFO -  DBG-FRONTEND: Destroying the WatchExpressionsView
14:46:57     INFO -  DBG-FRONTEND: Destroying the GlobalSearchView
14:46:57     INFO -  DBG-FRONTEND: Destroying the DebuggerView window
14:46:57     INFO -  DBG-FRONTEND: Destroying the DebuggerView panes
14:46:57     INFO -  DBG-FRONTEND: Destroying the DebuggerView editor
14:46:57     INFO -  DBG-FRONTEND: SourceScripts is disconnecting...
14:46:57     INFO -  DBG-FRONTEND: StackFrames is disconnecting...
14:46:57     INFO -  DBG-FRONTEND: ThreadState is disconnecting...
14:46:57     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | correct number of breakpoints have been added
14:46:57     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | correct number of breakpoints have been removed
14:46:57  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | correct number of editor breakpoint changes - Got 3, expected 4
14:46:57  WARNING -  This is a harness error.
14:46:57     INFO -  Stack trace:
14:46:57     INFO -      JS frame :: chrome://mochikit/content/browser-test.js :: test_is :: line 486
14:46:57     INFO -      JS frame :: chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js :: <TOP_LEVEL> :: line 312
14:46:57     INFO -      JS frame :: chrome://mochikit/content/browser-test.js :: Tester_nextTest :: line 250
14:46:57     INFO -      JS frame :: chrome://mochikit/content/browser-test.js :: <TOP_LEVEL> :: line 419
14:46:57     INFO -      native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
14:46:57     INFO -  INFO TEST-END | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | finished in 30064ms
Panos, maybe related to the fixup you landed in bug 832920?
And by Panos, I meant Mihai of course. Long day, sorry.
Yes and no. This test is a generic intermittent failures "generator".
Assignee: nobody → mihai.sucan
Status: NEW → ASSIGNED
This test turned into a permaorange on fx-team due to the landing of Bug 795368 which changes the name of an event used by the test.
Landed this in fx-team:
https://hg.mozilla.org/integration/fx-team/rev/86c98c4d36da

I also included a change suggested in bug 832920 comment 40 which will hopefully help with lowering the intermittent failures happening in other repos.
The permaorange is fixed, but the intermittent failure continues to happen.

bz: I tried synthesizeMouse() at iframe.contentDocument locally and that didn't work. I added an iframe.contentDocument.documentElement.getBoundClientRect() call before synthesizeMouse() in the patch and that seemed to work, locally. However, I still get the intermittent failure with the landed patch (in fx-team). Anything else I could try to make that synthesizeMouse() to always work? Thanks!
Flags: needinfo?(bzbarsky)
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/86c98c4d36da

Thanks for working on this, Mihai!
> Anything else I could try to make that synthesizeMouse() to always work?

Note sure.  Olli?
Flags: needinfo?(bzbarsky) → needinfo?(bugs)
Thank you Olli!

This is a patch for the test to try sendMouseEventToWindow(). It works locally, but here I never reproduced the intermittent failure.

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=1fb013af0df7
Attachment #721860 - Flags: review?(vporof)
Comment on attachment 721860 [details] [diff] [review]
use sendMouseEventToWindow()

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

::: browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js
@@ +281,5 @@
> +                  .getInterface(Ci.nsIDOMWindowUtils);
> +
> +      let rect = iframe.getBoundingClientRect();
> +      let left = rect.left + 10;
> +      let top = rect.top + 70;

lol
Attachment #721860 - Flags: review?(vporof) → review+
Thank you Victor. Landed the patch:

https://hg.mozilla.org/integration/mozilla-inbound/rev/81f5d3c84996
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/81f5d3c84996
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Guess not.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Made a small change locally, and pushed it to the try server. [1] I am running out of ideas of things we could try to do.

Panos, Victor, any ideas? Can any of you reproduce this failure? I'm having a hard time trying to fix it, while I cannot repro.

We might need to disable the test until we find a way to fix it.


[1] https://tbpl.mozilla.org/?tree=Try&rev=edeb297ec202
Flags: needinfo?
(In reply to Mihai Sucan [:msucan] from comment #107)
> Made a small change locally, and pushed it to the try server. [1] I am
> running out of ideas of things we could try to do.
> 
> Panos, Victor, any ideas? Can any of you reproduce this failure? I'm having
> a hard time trying to fix it, while I cannot repro.
> 
> We might need to disable the test until we find a way to fix it.

I think that would probably be the best thing to do at this point, with the upcoming work week and all.
(In reply to Panos Astithas [:past] from comment #116)
> 
> I think that would probably be the best thing to do at this point, with the
> upcoming work week and all.

I'm feeling a bit uneasy about disabling since this tests important functionality and there aren't any similar tests in the debugger suite. That said, I also tried and failed to reproduce this locally, and my guesses as to how it should be fixed are as good as anyone's. 

Maybe
> let left = rect.left + 10; and
> let top = rect.top + 70;
are a bad idea, since they may not always map to the right location when placing a breakpoint? I'm talking minor (random) pixel deltas here, which are out of our control.

Wouldn't the source editor's API to get coordinates based on line positions be more appropriate? Get the x/y of the line we want to add a breapoint and then subtract, let's say, half of the gutter width from the x coordinate, instead of approaching things the other way around.
The problem here is not about the correctness of coordinates. Coordinates always match correctly on the try servers - it's not like font sizes differ sometimes.

Those hard coded coords are indeed a problem we need to fix for the hidpi support on Macs.

The problem with this test is an older and well known issue (from my experience): synthesizeMouse(), in some cases, has no effect. This is why we get the time out - not because of some wrong coord. The UI is not ready, and I don't know right now what better events to wait for. This is why we tried flushing the layout, as bz suggested.

A possible fix would be to use EventUtils.sendMouseEvent() which bypasses the actual click on the UI element - we would just dispatch the DOM click event. That is usually a good workaround, but in this special case dispatching the mouse event is far from ideal: I would have to go down into Orion's editor private DOM elements and pick which element to sendMouseEvent to. Ugly.

Thoughts?
(In reply to Mihai Sucan [:msucan] from comment #137)
> A possible fix would be to use EventUtils.sendMouseEvent() which bypasses
> the actual click on the UI element - we would just dispatch the DOM click
> event. That is usually a good workaround, but in this special case
> dispatching the mouse event is far from ideal: I would have to go down into
> Orion's editor private DOM elements and pick which element to sendMouseEvent
> to. Ugly.
> 
> Thoughts?

I'm OK with that.
This hasn't happened in almost 2 weeks on release, maybe fixed by bug 854185?
(In reply to Victor Porof [:vp] from comment #175)
> This hasn't happened in almost 2 weeks on release, maybe fixed by bug 854185?

I'm not sure, but maybe you are right. Yesterday I prepared a patch for this bug and I pushed it to the try server (green results). https://tbpl.mozilla.org/?tree=Try&rev=d1f88a2217fe

I'm going to attach the patch. Maybe someone wants it in aurora as well.
Attached patch use mozRequestAnimationFrame() (obsolete) — — Splinter Review 
Two changes in this patch:

1. use mozRequestAnimationFrame() to delay when the click is sent, to make sure the layout is flushed. I also added a waitForFocus().

Based on some testing this approach should always work.

2. get the line height from orion, to determine the correct coordinates - where to click.
Attachment #732250 - Flags: review?(vporof)
Comment on attachment 732250 [details] [diff] [review]
use mozRequestAnimationFrame()

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

::: browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js
@@ +239,5 @@
> +    let window = gEditor.editorElement.contentWindow;
> +    executeSoon(() => {
> +      gEditor.focus();
> +      waitForFocus(() => window.mozRequestAnimationFrame(onReadyForClick),
> +                   window)

I don't think we need gEditor.focus() and waitForFocus(). I'm pretty sure the editor is focused at this point in the test.

@@ +260,4 @@
>  
> +    let rect = iframe.getBoundingClientRect();
> +    let left = rect.left + 10;
> +    let top = rect.top + gEditor._view.getLineHeight() * 4 + 5;

Ugh, _view? I'm not sure this is a good idea. Can you use getLocationAtOffset instead? Either that or expose gEditor._view.getLineHeight as public API.
Attachment #732250 - Flags: review?(vporof) → review-
(In reply to Victor Porof [:vp] from comment #178)
> Comment on attachment 732250 [details] [diff] [review]
> use mozRequestAnimationFrame()
> 
> Review of attachment 732250 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js
> @@ +239,5 @@
> > +    let window = gEditor.editorElement.contentWindow;
> > +    executeSoon(() => {
> > +      gEditor.focus();
> > +      waitForFocus(() => window.mozRequestAnimationFrame(onReadyForClick),
> > +                   window)
> 
> I don't think we need gEditor.focus() and waitForFocus(). I'm pretty sure
> the editor is focused at this point in the test.

Sure, we can try without that.


> @@ +260,4 @@
> >  
> > +    let rect = iframe.getBoundingClientRect();
> > +    let left = rect.left + 10;
> > +    let top = rect.top + gEditor._view.getLineHeight() * 4 + 5;
> 
> Ugh, _view? I'm not sure this is a good idea. Can you use
> getLocationAtOffset instead? Either that or expose
> gEditor._view.getLineHeight as public API.

In tests I allow access to private source editor properties, but I do try to avoid it. See the source editor tests. Also, tests themselves do not always warrant the addition of new source editor APIs.

If we want to expose getLineHeight() we need to add a source editor test, and I would like us to avoid adding new APIs in an intermittent test failure fix.

Thank you!
(In reply to Mihai Sucan [:msucan] from comment #179)
> 
> In tests I allow access to private source editor properties, but I do try to
> avoid it. See the source editor tests. Also, tests themselves do not always
> warrant the addition of new source editor APIs.
> 

Well, if we're switching to CodeMirror and if that's not providing us with such a method, we're gonna have a bad time :) This may be the case for a simple method like getLineHeight, but it seems like a funky exception to a good rule IMHO. But this is your territory so you know best.
s/This may be the case/This may *not* be the case/
Removed waitForFocus() and avoided the use of getLineHeight().
Attachment #732250 - Attachment is obsolete: true
Attachment #732751 - Flags: review?(vporof)
(In reply to Victor Porof [:vp] from comment #180)
> (In reply to Mihai Sucan [:msucan] from comment #179)
> > 
> > In tests I allow access to private source editor properties, but I do try to
> > avoid it. See the source editor tests. Also, tests themselves do not always
> > warrant the addition of new source editor APIs.
> > 
> 
> Well, if we're switching to CodeMirror and if that's not providing us with
> such a method, we're gonna have a bad time :) This may be the case for a
> simple method like getLineHeight, but it seems like a funky exception to a
> good rule IMHO. But this is your territory so you know best.

The switch to CodeMirror will break almost all tests anyway. Anyhow, it seemed I found sufficient source editor public APIs for this test, so this is a definite improvement. Thanks for pointing it out!
Comment on attachment 732751 [details] [diff] [review]
use mozRequestAnimationFrame() - version 2

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

Yay for getLocationAtOffset. R+ with a green try.
Attachment #732751 - Flags: review?(vporof) → review+
(In reply to Victor Porof [:vp] from comment #184)
> Comment on attachment 732751 [details] [diff] [review]
> use mozRequestAnimationFrame() - version 2
> 
> Review of attachment 732751 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yay for getLocationAtOffset. R+ with a green try.

https://tbpl.mozilla.org/?tree=Try&rev=d1f88a2217fe
Comment on attachment 732751 [details] [diff] [review]
use mozRequestAnimationFrame() - version 2

Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/818e9a44b2ba
https://hg.mozilla.org/mozilla-central/rev/818e9a44b2ba
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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: