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

VERIFIED FIXED in Firefox 47

Status

P1
normal
VERIFIED FIXED
3 years ago
5 months ago

People

(Reporter: kit, Assigned: pbro)

Tracking

({regression})

47 Branch
Firefox 47
x86_64
All
regression

Firefox Tracking Flags

(firefox46 unaffected, firefox47+ fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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.
(Reporter)

Updated

3 years ago
OS: Unspecified → Windows 7
Hardware: Unspecified → x86_64

Updated

3 years ago
Component: Untriaged → Developer Tools
Did this just start happening?  Are there any other details?

It seems to work okay here, so something must be missing.
Flags: needinfo?(kit)

Comment 2

3 years ago
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
(Reporter)

Comment 3

3 years ago
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.
Flags: needinfo?(kit)
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.
Status: UNCONFIRMED → NEW
status-firefox47: --- → ?
Component: Developer Tools → Developer Tools: Inspector
Ever confirmed: true
Flags: needinfo?(pbrosset)
OS: Windows 7 → All

Comment 5

3 years ago
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
Blocks: 1245420
status-firefox46: --- → unaffected
status-firefox47: ? → affected
tracking-firefox47: --- → ?
Keywords: regression
Summary: Developer tools cannot be reopened once closed (per tab). → Detached Developer tools window cannot be reopened once closed (per tab).
(Assignee)

Comment 6

3 years ago
(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
Blocks: 1229913
No longer blocks: 1245420
Flags: needinfo?(pbrosset)
(Assignee)

Comment 7

3 years ago
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.
Priority: -- → P1
(Assignee)

Comment 8

3 years ago
Created attachment 8721901 [details] [diff] [review]
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?
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Attachment #8721901 - Flags: review?(poirot.alex)
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.
Attachment #8721901 - Flags: review?(poirot.alex) → review+
(Assignee)

Comment 11

3 years ago
(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 13

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/457f567214ca
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Kit, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(kit)

Updated

3 years ago
tracking-firefox47: ? → +
(Reporter)

Comment 15

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

Thanks!
Flags: needinfo?(kit)

Updated

3 years ago
Status: RESOLVED → VERIFIED
(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!

Updated

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