Node picker is dismissed during page load

RESOLVED FIXED in Firefox 52

Status

P2
normal
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

unspecified
Firefox 52

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Assignee: nobody → poirot.alex
Comment hidden (mozreview-request)
(Assignee)

Comment 3

2 years ago
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.
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 5

2 years ago
mozreview-review
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)));`
Attachment #8804488 - Flags: review?(pbrosset) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 7

2 years ago
Everything addressed and rebased. Pushing one last time to try.
Comment hidden (mozreview-request)

Comment 9

2 years ago
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ea1b1d7d1f78
Prevent toolbox.selectTool from racing when calling multiple times. r=pbro

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ea1b1d7d1f78
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52

Comment 11

2 years ago
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]

Updated

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