1249719 - Detached Developer tools window cannot be reopened once closed (per tab).

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P1)

Description

[kit]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160219030248

Steps to reproduce:

Open a new tab (it is not necessary to go to a webpage)
Open the Developer Tools via any method (e.g. Ctrl-Shift-I)
Close the Developer Tools via any method (e.g. Ctrl-Shift-I again)
Attempt to reopen the Developer Tools via any method (e.g. Ctrl-Shift-I again)

Tools cannot be reopened for this tab.


Actual results:

Developer Tools window opens on the first attempt, but not the second.


Expected results:

It should be possible to open and close the Developer Tools window any number of times.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Did this just start happening?  Are there any other details?

It seems to work okay here, so something must be missing.

Comment 2

[Loic]

I can't reproduce this issue with FF47 32/64b e10s on/off.

Could you test:
1) in safe mode
https://support.mozilla.org/en-US/kb/troubleshoot-firefox-issues-using-safe-mode

2) with a fresh profile:
https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles

Comment 3

[kit]

I would guess it has been happening for at least a week.

I have a nightly build from 2016-01-05 (FF 46) that does not have the issue.
I tried with e10s on/off and the problem occurs either way.
I tested in Safe Mode and the problem is still present.

I did notice some additional things:

The problem only occurs when the Developer Tools are configured to open in a separate window - if they are docked then everything works fine.

I'm surprised I didn't notice this at first but it seems to be specific to the "Inspector" tab on the Developer Tools.

I originally thought it was any tab - but as long as I don't show the Inspector tab, I have no problems. As soon as I switch to the Inspector tab, and close the Dev Tools window, I am unable to reopen the Dev Tools window.

Also, once a browser tab has been 'broken' by open the Dev Tools to the Inspector tab and then closing the Dev Tools window, the "Toggle Tools" option in the dropdown menu stays checked - presumably Nightly thinks the window is already visible.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Ah great, those details help quite a lot!

Guessing :pbro may be the best to investigate here, since it seems to require the Inspector tab.

Comment 5

[Loic]

STR:
1) Ctrl+Shift+C
2) Detach the devtool window ("Show in separate window")
3) Repeat Ctrl+Shift+I

Result: devtool window disappears but never opens again after the 1st time.

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1dbe350b57b17ec1ce2887441b79c6f51b429378&tochange=6148b5848349edf97e3cb4b1a604d8a7e5b50544

Patrick Brosset — Bug 1245420 - Sort devtools/client/framework tests and clean-up browser_toolbox_tabsswitch_shortcuts.js; r=ochameau

Comment 6

[Patrick Brosset <:pbro>]

(In reply to Loic from comment #5)
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=1dbe350b57b17ec1ce2887441b79c6f51b429378&tochange=6148
> b5848349edf97e3cb4b1a604d8a7e5b50544
> 
> Patrick Brosset — Bug 1245420 - Sort devtools/client/framework tests and
> clean-up browser_toolbox_tabsswitch_shortcuts.js; r=ochameau
The regression comes from this changeset instead:
3c02ec7bd6b8	Patrick Brosset — Bug 1229913 - Prevent race conditions when there are many animation mutations; r=miker

Comment 7

[Patrick Brosset <:pbro>]

It looks like the destroy process of the inspector-panel never ends (or silently fails), and that causes the toolbox to be stuck in an in-between state, already hidden, but still there.

Comment 8

[Patrick Brosset <:pbro>]

Attachment: Bug_1249719_-_Use_promise.all_instead_of_Promise.a.diff

In animation-controller.js, the destroy method seems to hang on this line:
yield Promise.all(this.nonBlockingPlayerReleases);
https://dxr.mozilla.org/mozilla-central/source/devtools/client/animationinspector/animation-controller.js#187

In the current use case, this.nonBlockingPlayerReleases is actually an empty array!
So, for some reasons, doing 
yield Promise.all([]);
here never resolves.

I noticed that DOM promises have been used here (capital *P*romise), instead of the other promise implementation which is imported in this module (lowercase *p*romise).
And changing this around solves the issue. There is definitely a timing issue here, but I don't understand what's happening exactly. I just know that this patch solves the problem.

Alex: do you know the difference between the 2 promise implementations?

Comment 9

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fd31e31ec56&group_state=expanded

Comment 10

[Alexandre Poirot [:ochameau]]

Comment on attachment 8721901 [details] [diff] [review]
Bug_1249719_-_Use_promise.all_instead_of_Promise.a.diff

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

Are you sure it is because of empty array?

The following code resolves immediately:
  Promise.all([]).then(console.log.bind(console, "plop"))

But I can easily see failures if we start mixing promises...
That wouldn't surprise me at all, even if I don't know about any particular case where that fails.

In any case, you fix looks good, we should stick to use a single type of promise in a given tool.

Comment 11

[Patrick Brosset <:pbro>]

(In reply to Alexandre Poirot [:ochameau] from comment #10)
> Comment on attachment 8721901 [details] [diff] [review]
> Bug_1249719_-_Use_promise.all_instead_of_Promise.a.diff
> 
> Review of attachment 8721901 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Are you sure it is because of empty array?
> 
> The following code resolves immediately:
>   Promise.all([]).then(console.log.bind(console, "plop"))
Yeah, this resolves for me too.
You're right, the fact that the array is empty isn't the reason for this bug, it also happens when the array has values. I just happened to have noticed this and found it surprising.
My fix also works when the array isn't empty.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/457f567214ca

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/457f567214ca

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Kit, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 15

[kit]

Confirmed - this is working as expected in latest Nightly - 47.0a1 (2016-03-04)

Thanks!

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to kit from comment #15)
> Confirmed - this is working as expected in latest Nightly - 47.0a1
> (2016-03-04)
> 
> Thanks!

Thank you for a prompt verification. This is amazing!