Closed Bug 1199180 Opened 8 years ago Closed 8 years ago

Intermittent browser_markupview_keybindings_04.js | The <div> node is selected - Got body, expected div

Categories

(DevTools :: Inspector, defect)

Product:
DevTools
Component:
Inspector
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox43 fixed, firefox44 fixed, firefox45 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 43
Tracking Flags:
Tracking Status
firefox43 --- fixed
firefox44 --- fixed
firefox45 --- fixed

People

(Reporter: RyanVM, Assigned: Honza)

Details

(Keywords: intermittent-failure)

Attachments

(5 files, 1 obsolete file)

test screenshot
1.52 MB, image/png
Details
bug1199180-2.patch
7.61 KB, patch
pbro
: review+
Details | Diff | Splinter Review
bug1199180-fix-arguments.patch
993 bytes, patch
pbro
: review+
Details | Diff | Splinter Review
MozReview Request: Bug 1199180 - Revert rev f508744adc9f as it did not help with the intermittent failures. r?pbrosset
40 bytes, text/x-review-board-request
pbro
: review+
Details
MozReview Request: Bug 1199180 - Wait for the inspector-updated event after selecting nodes with UP key. r?pbrosset
40 bytes, text/x-review-board-request
pbro
: review+
Details
Attached image test screenshot — 
      No description provided.
Flags: needinfo?(odvarko)
Patrick, I've checked the patch from bug 1184316 and I don't see any reason why it should break the test. It looks like synthesize mouse click or key press in selectWithElementPicker() method doesn't work properly. The screenshot shows that body is selected while "div" is the selector in that method.

Do you have any ideas why treeherder is failing sometimes?

Honza
Flags: needinfo?(odvarko) → needinfo?(pbrosset)
(In reply to Jan Honza Odvarko [:Honza] from comment #7)
> Patrick, I've checked the patch from bug 1184316 and I don't see any reason
> why it should break the test. It looks like synthesize mouse click or key
> press in selectWithElementPicker() method doesn't work properly. The
> screenshot shows that body is selected while "div" is the selector in that
> method.
> 
> Do you have any ideas why treeherder is failing sometimes?
Hard to say. I agree that the patch in bug 1184316 doesn't seem suspicious, but you never know, it could be synthesizing the event slightly differently.
In any case, that's the kind of frustrating intermittent failures that can only really be investigated by adding detailed logs to the test and inspector/highlighter and running the test many times on TRY, to hopefully get some insights on what's really happening.
Unless of course the failure can be reproduced locally.
Flags: needinfo?(pbrosset)
Looks like BrowsereTestUtils.js + AsyncUtilsContent.js are using:
chrome://mochikit/content/tests/SimpleTest/EventUtils.js

...while devtools/shared/frame-script-utils.js is using:
chrome://marionette/content/EventUtils.js

Two try pushes:

(chrome://marionette/content/EventUtils.js) https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6a605bd354e

(chrome://mochikit/content/tests/SimpleTest/EventUtils.js) https://treeherder.mozilla.org/#/jobs?repo=try&revision=5fa7a733c082


Let's see the failures now.

Honza
Attached patch bug1199180-1.patch (obsolete) — — Splinter Review 
Patrick, the main difference (after bug 1184316 landed) is that our previous implementation of "Test:SynthesizeMouse" used:
chrome://marionette/content/EventUtils.js

...while the BrowsereTestUtils.js + AsyncUtilsContent.js are using:
chrome://mochikit/content/tests/SimpleTest/EventUtils.js

There are a lot of differences between those two EventUtils implementations (why there are two?), but I don't see any specific one that could cause the intermittent.

I am attaching a patch that uses the old way for the markupview, which helps to avoid the failure. It's rather a workaround, but at least it stops the test suite to fail and we don't have to backout the patch for bug 1184316.

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6a605bd354e

Honza
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Attachment #8657784 - Flags: review?(pbrosset)
Comment on attachment 8657784 [details] [diff] [review]
bug1199180-1.patch

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

I think I need some more clarifications, maybe I missed something.

::: browser/devtools/markupview/test/browser_markupview_keybindings_04.js
@@ +50,5 @@
> +    selector: "div",
> +    options: {type: "contextmenu", button: 2}
> +  });
> +
> +  // See: https://bugzilla.mozilla.org/show_bug.cgi?id=1199180

Instead of commented out code, can you add a comment before executeInContent (above) that explains why we're using this instead of BrowserTestUtils here.

@@ +83,5 @@
> +    selector: "div",
> +    options: {type: "mousemove"}
> +  });
> +
> +  // See: https://bugzilla.mozilla.org/show_bug.cgi?id=1199180

Same remark here.

::: browser/devtools/markupview/test/frame-script-utils.js
@@ +13,5 @@
> +EventUtils.window = {};
> +EventUtils.parent = EventUtils.window;
> +EventUtils._EU_Ci = Components.interfaces;
> +EventUtils._EU_Cc = Components.classes;
> +subScriptLoader.loadSubScript("chrome://mochikit/content/tests/SimpleTest/EventUtils.js", EventUtils);

I'm confused, you said that previously (before bug 1184316), markupview tests were using executeInContent from shared/frame-script-utils which uses EventUtils from:

let EventUtils = {};
subScriptLoader.loadSubScript("chrome://marionette/content/EventUtils.js", EventUtils);

Then after bug 1184316, occurrences of executeInContent got removed to use BrowserTestUtils instead, but that caused an intermittent here. And to fix it, you're re-introducing a frame script, just for the markup-view (btw, I'm ok with the general approach as a stop-gap) that can be used to synthesize events, but you're using EventUtils from:

subScriptLoader.loadSubScript("chrome://mochikit/content/tests/SimpleTest/EventUtils.js", EventUtils);

Shouldn't you be loading EventUtils from marionette instead, which is what used to work?

Also, I'm not sure why this is needed (it at least would require a comment):

EventUtils.window = {};
EventUtils.parent = EventUtils.window;
EventUtils._EU_Ci = Components.interfaces;
EventUtils._EU_Cc = Components.classes;
Attachment #8657784 - Flags: review?(pbrosset)
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #72)
> Comment on attachment 8657784 [details] [diff] [review]
> bug1199180-1.patch
> 
> Review of attachment 8657784 [details] [diff] [review]:
> -----------------------------------------------------------------
You are right, the idea is to use the:
chrome://marionette/content/EventUtils.js
(I somehow introduced last minute changes in the previous patch I guess).

The comments are also updated.

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae3179234840

Let's see how the try looks like now.

Thanks Patrick!
Honza
Attachment #8657784 - Attachment is obsolete: true
Attachment #8660755 - Flags: review?(pbrosset)
Attachment #8660755 - Flags: review?(pbrosset) → review+
Thanks Patrick, let's see if this helps!

Honza
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f508744adc9f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Patrick, I've spent some time on this to figure out why the test still fails. I don't see any difference in the code between now and before the Bug 1184316 was fixed, but I've found a place where arguments are wrongly passed into `executeInContent` method.

See my patch.

The `selectWithElementPicker` is calling `executeInContent("Test:SynthesizeKey"` and since it isn't using yield - I think it isn't supposed to wait for confirmation message sent from the content. There was a `false` argument, but passed as the *third* instead of *fourth* arg to the method. The message coming back could be accidentally handled by new `executeInContent` call, I guess.

I don't know if this helps, but it should be fixed anyway.

Honza
Attachment #8673081 - Flags: review?(pbrosset)
Attachment #8673081 - Flags: review?(pbrosset) → review+
Thanks for the review Patrick.
Let's see if this helps.
Honza
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bde1a471e509
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Bug 1199180 - Revert rev f508744adc9f as it did not help with the intermittent failures. r?pbrosset
Attachment #8684566 - Flags: review?(pbrosset)
Bug 1199180 - Wait for the inspector-updated event after selecting nodes with UP key. r?pbrosset

The likely steps that lead to intermittent failures in
browser_markupview_keybindings_04.js are:
1) UP key is pressed, the test waits for child updates and
   node-highlight event.
2) Once those have finished, the selection has changed to <body> BUT
   the inspector-updated event for the change has not been emitted
   (child node update finishes before the event is emitted).
3) The test calls selectWithElementPicker() presses ENTER to pick the
   <div> element and starts to wait for an inspector-updated event.
4) The inspector-updated event from (1) is finally emitted and the test
   continues BUT the selection change at (3) has not yet completed.
   This means <body> is still selected and the assertion fails.

Since a new selection will always cause the inspector-updated event to
be emitted it makes sense to wait for it after pressing UP.
Attachment #8684567 - Flags: review?(pbrosset)
(In reply to Sami Jaktholm from comment #239)
> Created attachment 8684567 [details]
> MozReview Request: Bug 1199180 - Wait for the inspector-updated event after
> selecting nodes with UP key. r?pbrosset
> 
> Bug 1199180 - Wait for the inspector-updated event after selecting nodes
> with UP key. r?pbrosset
> 
> The likely steps that lead to intermittent failures in
> browser_markupview_keybindings_04.js are:
> 1) UP key is pressed, the test waits for child updates and
>    node-highlight event.
> 2) Once those have finished, the selection has changed to <body> BUT
>    the inspector-updated event for the change has not been emitted
>    (child node update finishes before the event is emitted).
> 3) The test calls selectWithElementPicker() presses ENTER to pick the
>    <div> element and starts to wait for an inspector-updated event.
> 4) The inspector-updated event from (1) is finally emitted and the test
>    continues BUT the selection change at (3) has not yet completed.
>    This means <body> is still selected and the assertion fails.
> 
> Since a new selection will always cause the inspector-updated event to
> be emitted it makes sense to wait for it after pressing UP.
Ah, this sounds like a very good explanation of the problem. The try build is green so far, I've re triggered some more run just in case, but looks like the problem is gone. Thanks Sami.
Attachment #8684566 - Flags: review?(pbrosset) → review+
Comment on attachment 8684566 [details]
MozReview Request: Bug 1199180 - Revert rev f508744adc9f as it did not help with the intermittent failures. r?pbrosset

https://reviewboard.mozilla.org/r/24607/#review22171
Comment on attachment 8684567 [details]
MozReview Request: Bug 1199180 - Wait for the inspector-updated event after selecting nodes with UP key. r?pbrosset

https://reviewboard.mozilla.org/r/24609/#review22173
Attachment #8684567 - Flags: review?(pbrosset) → review+
Keywords: checkin-needed
This is great, thanks Sami for the fix!

Honza
https://hg.mozilla.org/mozilla-central/rev/4156d6463c5a
https://hg.mozilla.org/mozilla-central/rev/d1a9eaecbe0f
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.