1437714 - Fix timeout in browser_inspector_highlighter-keybinding_03.js with the patch for bug 1193394

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=701a987d4fcddd24a4f67f08df6f9ee2b9b81f3a&selectedJob=161714284

https://treeherder.mozilla.org/logviewer.html#?job_id=161714284&repo=try&lineNumber=7140

This might be related to bug 1437712

Comment 1

[Hiroyuki Ikezoe (:hiro)]

The timeout happened at the place waiting for moveMouseOver promise [1].

CCing Patrick who are the reviewer for the patch that introduced this test in bug 1120111.

[1] https://hg.mozilla.org/mozilla-central/file/6d8f470b2579/devtools/client/inspector/test/browser_inspector_highlighter-keybinding_03.js#l25

Comment 2

[Hiroyuki Ikezoe (:hiro)]

I started looking into this (honestly just run the test locally).

Comment 3

[Hiroyuki Ikezoe (:hiro)]

We have to wait for 'picker-stopped' event after doKeyPick() there before calling startPicker() in the next test case, otherwise the state of the picker will be something broken.

From the failure test;

  info("Testing enter/return key as pick-node command");
  yield doKeyPick({key: "VK_RETURN", options: {}});
  is(inspector.selection.nodeFront.id, "another",
     "The #another node was selected. Passed.");

In this case, we don't wait for 'picker-stopped'.  Whereas;

  info("Testing escape key as cancel-picker command");
  yield startPicker(toolbox);
  yield moveMouseOver("#ahoy");
  yield doKeyStop({key: "VK_ESCAPE", options: {}});
  is(inspector.selection.nodeFront.id, "another",
     "The #another DIV is still selected. Passed.");

in this case we do wait for 'picker-stopped' in doKeyStop().

I haven't checked all code but as per the try in comment 0, this is the only place that we do care about.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

A try based on the patch for bug 1193394;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=386ad407dad9835b899e576905ffd3eff4b25a56

And a try based on bare m-c;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=70ee94daa809379472d91043fc0a4069baa8502f

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Gah, the patch causes timeout if the patch for bug 1193394 is not applied.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

We have to wait the event in doKeyPick().
https://treeherder.mozilla.org/#/jobs?repo=try&revision=100c5ce01addc23872cfb32a29d622ac33d0f713

Try based on bare m-c;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d99adf2ddcbd913d10696a3c5da1ca91ae5f45e4

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Both tries look green.

Patrick, Though I haven't dug into detail how the test proceeds before 'picker-stopped' event is dispatched, does it make sense to wait for 'picker-stopped' event in doKeyPick() along with 'inspector-updated' and 'new-node-front'?  If it doesn't I'd like to leave devtools guys to investigate this issue.

Comment 8

[Patrick Brosset <:pbro>]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> Both tries look green.
> 
> Patrick, Though I haven't dug into detail how the test proceeds before
> 'picker-stopped' event is dispatched, does it make sense to wait for
> 'picker-stopped' event in doKeyPick() along with 'inspector-updated' and
> 'new-node-front'?  If it doesn't I'd like to leave devtools guys to
> investigate this issue.
Yes it does! This change looks great to me, and both tries are green, so I think we're good!

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Thank you Patrick!

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Hmm, MozReview doesn't allow to push review requests once the requests specified reviewers that does not accept review request even after the specified reviewers were dropped.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1437714 - Wait for 'picker-stopped' event before starting the new picker in the next test case.

Review commit: https://reviewboard.mozilla.org/r/221046/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/221046/

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Leaving NI instead of review request.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8951781 [details]
Bug 1437714 - Wait for 'picker-stopped' event before starting the new picker in the next test case.

:jlast would you mind reviewing this on behalf of :pbro?

Comment 14

[Patrick Brosset <:pbro>]

Comment on attachment 8951781 [details]
Bug 1437714 - Wait for 'picker-stopped' event before starting the new picker in the next test case.

https://reviewboard.mozilla.org/r/221046/#review228660

Thanks Hiro.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5f7512c0b66a
Wait for 'picker-stopped' event before starting the new picker in the next test case. r=pbro

Comment 16

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/5f7512c0b66a