1311737 - Node picker is dismissed during page load

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Alexandre Poirot [:ochameau]]

It is pretty commong to me, while debugging a personal web page, to get the node picker to be dismissed. It feels like it is very common and occurs very often, but I was only able to reproduce this precise STR:

 * Open a new tab
 * Load mozilla.org *and* immediately open the inspector *and* click on the node picker

During the page load, you will see the highlighter kicks in, but after 1 or 1.5s, when the page finishes loading, it will stop and the node picker is dismissed. The toolbar icon goas back to black.

Reproduced on today's nightly on Windows7.
It reproduces on mozilla.org and various personnal pages.

Note that this STR also triggers bug 1249119 which totally breaks the inspector.

Comment 1

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7235c997bc33

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1311737 - Prevent toolbox.selectTool from racing when calling multiple times.

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

Comment 3

[Alexandre Poirot [:ochameau]]

I think it really relates to opening the inspector on a new tab, or while switching to it for the first time as it involves toolbox.selectTool.

My STR was failing over here:
http://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox-highlighter-utils.js#126-127
    yield toolbox.selectTool("inspector");
    toolbox.on("select", cancelPicker);

selectTool was already called by showToolbox, when pressing CTRL+I to open the inspector immediately.
And we reached this code in startPicker, when I click also immediately on the element picker.
startPicker called toolbox.selectTool before the first call was done, so the tool is still loading,
but selectTool logic was returning early as it considered it as the already selected one (this.currentToolId == id).
In order to fix that I'm improving toolbox.selectTool to avoid races and resolves only once the tool is fully loaded.

Comment 4

[Patrick Brosset <:pbro>]

Reproduced this too. I understand the frustration. Let's triage this as a P2 for now. I see you already have a patch for review anyway, so I'll review this now.

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8804488 [details]
Bug 1311737 - Prevent toolbox.selectTool from racing when calling multiple times.

https://reviewboard.mozilla.org/r/88436/#review88058

::: devtools/client/framework/test/browser_toolbox_select_event.js:51
(Diff revision 1)
> +  yield testSelectToolRace();
> +
> +  /**
> +   * Assert that two calls to selectTool won't race
> +   */
> +  function* testSelectToolRace() {

Can you move this function at the end of the test after `testToolSelectEvent` ?

::: devtools/client/framework/test/browser_toolbox_select_event.js:82
(Diff revision 1)
> +
>    /**
>     * Assert that selecting the given toolId raises a select event
>     * @param {toolId} Id of the tool to test
>     */
>    function testSelectEvent(toolId) {

optional: since you're changing this test, maybe switch `testSelectEvent` and `testToolSelectEvent` to be generators instead of using `return new Promise`.

::: devtools/client/framework/toolbox.js:1436
(Diff revision 1)
> +
> -      // re-focus tool to get key events again
> +        // re-focus tool to get key events again
> -      this.focusTool(id);
> +        this.focusTool(id);
>  
> -      // Return the existing panel in order to have a consistent return value.
> +        // Return the existing panel in order to have a consistent return value.
> -      return promise.resolve(this._toolPanels.get(id));
> +        return promise.resolve(this._toolPanels.get(id));

nit: `panel` already exists, so this can be simplified to:

`return promise.resolve(panel);`

::: devtools/client/framework/toolbox.js:1440
(Diff revision 1)
> +      return this.once("select").then(() => {
> +        return promise.resolve(this._toolPanels.get(id));
> +      });

Maybe better on one line:

`return this.once("select").then(() => promise.resolve(this._toolPanels.get(id)));`

Comment 6

[Alexandre Poirot [:ochameau]]

Comment on attachment 8804488 [details]
Bug 1311737 - Prevent toolbox.selectTool from racing when calling multiple times.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88436/diff/1-2/

Comment 7

[Alexandre Poirot [:ochameau]]

Everything addressed and rebased. Pushing one last time to try.

Comment 8

[Alexandre Poirot [:ochameau]]

Comment on attachment 8804488 [details]
Bug 1311737 - Prevent toolbox.selectTool from racing when calling multiple times.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88436/diff/2-3/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea1b1d7d1f78
Prevent toolbox.selectTool from racing when calling multiple times. r=pbro

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ea1b1d7d1f78

Comment 11

[Tanvir Rahman]

I have reproduced this bug with Nightly 52.0a1 (2016-10-20) on Windows 10, 64 bit!

The Bug's fix is now verified on Beta 52.0b9

Build ID 	20170223185858
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0

[bugday-20170222]