Closed Bug 1311737 Opened 6 years ago Closed 6 years ago

Node picker is dismissed during page load


(DevTools :: Inspector, defect, P2)



(firefox52 fixed)

Firefox 52
Tracking Status
firefox52 --- fixed


(Reporter: ochameau, Assigned: ochameau)



(1 file)

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 *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 and various personnal pages.

Note that this STR also triggers bug 1249119 which totally breaks the inspector.
Assignee: nobody → poirot.alex
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:
    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.
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.
Priority: -- → P2
Comment on attachment 8804488 [details]
Bug 1311737 - Prevent toolbox.selectTool from racing when calling multiple times.

::: 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)));`
Attachment #8804488 - Flags: review?(pbrosset) → review+
Everything addressed and rebased. Pushing one last time to try.
Pushed by
Prevent toolbox.selectTool from racing when calling multiple times. r=pbro
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
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

Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.