Closed Bug 1328004 Opened 7 years ago Closed 7 years ago

Holding F12 breaks devtools toolbox and it can't be closed


(DevTools :: Framework, defect)

27 Branch
Not set


(firefox50 wontfix, firefox51 wontfix, firefox52 verified, firefox53 verified)

Firefox 53
Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- verified
firefox53 --- verified


(Reporter: arni2033, Assigned: ochameau)


(Blocks 1 open bug)


(Keywords: regression)


(1 file)

>>>   My Info:   Win7_64, Nightly 52, 32bit, ID 20161028030204 (2016-10-28)
1. Open url   data:text/html,<body></body>
2. Hold F12 for 3 seconds
3. Click close button in devtools toolbox

1. Open url   data:text/html,<body></body>
2. Execute the following code in scratchpad in browser context:
   function T(){try{gDevToolsBrowser.toggleToolboxCommand(gBrowser);}catch(e){}}T();setTimeout(T,100);
3. Click close button in devtools toolbox

AR:  Devtools toolbox breaks and can't be closed
ER:  Devtools toolbox should close

This is regression. So far Alexandre Poirot doesn't care about quality at all. Hopefully he'll
pay attention to this bug and fix it ASAP. If you do care about quality, please talk to him.

Wording:  (added, because some people may misunderstand)
1) "Alexandre Poirot doesn't care about quality at all" - that is true. Explanation:
   For any person, tracking new bugs (especially serious ones) that are set as blocking for bugs he
   implemented is a trivial task. If a person implements "improvement" like bug 1161072 (and therefore
   hang in bug 1197789, that was present for ONE YEAR with zero activity), that can only mean A or B
   A) he was aware of bug 1197789 => he intentionally added hang in browser (by keeping patch from
      bug 1161072 in browser) => therefore he doesn't care about quality at all.
   B) he wasn't aware of that bug => he didn't even care tracking whether he broke something =>
      had no intention of creating reliable product.
2) `"improvement"` - this expression contains quotes, because bug 1161072 only added hang.
No longer blocks: 1277113
Component: Untriaged → Developer Tools: Framework
 4:42.63 INFO: Last good revision: a98a1d78817f (2014-02-27)
 4:42.63 INFO: First bad revision: 58eca03214a6 (2014-02-28)
 4:42.63 INFO: Pushlog:

I was able to narrow down the regression window.
Version: Trunk → 27 Branch
The regression range you got is a bit strange, because I assumed the it's regression from bug 1266134:

Then OK, apparently it's not
See Also: → 1326697
The added test covers the closeToolbox codepath which happen to be related to this bug report.
It should also cover bug 1319928.

Otherwise the patch now waits for a still loading toolbox to be fully ready before trying to close it.
Comment on attachment 8825520 [details]
Bug 1328004 - Fix race when closing the devtools while a previous instance is still loading.

Great, thanks for the fix!

::: devtools/client/framework/devtools.js:470
(Diff revision 1)
>     *         associated to the target. true, if the toolbox was successfully
>     *         closed.
>     */
> -  closeToolbox: function DT_closeToolbox(target) {
> -    let toolbox = this._toolboxes.get(target);
> +  closeToolbox: Task.async(function* (target) {
> +    let toolbox = yield this._creatingToolboxes.get(target);
>      if (toolbox == null) {

Can we just use `if (!toolbox)` here and below?  I don't think we need to specifically call out null here...?
Attachment #8825520 - Flags: review?(jryans) → review+
Assignee: nobody → poirot.alex
Pushed by
Fix race when closing the devtools while a previous instance is still loading. r=jryans
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Please request Aurora approval on this when you get a chance.
Flags: needinfo?(poirot.alex)
Given that's the second regression around toolbox creation and destruction, I would like to be safe here and get some QA verification.

This is an indirect regression from bug 1266134. By the time bug 1266134 landed it wasn't that visible, some other change later make the regression more visible.
This is all about races, especially visible on slow hardware, but may be also on very fast one.

It would be good to verify that opening/closing the devtools are no longer subject to any race where we end up with broken toolbox or tool. It shouldn't be specific to the way you open/close it (like F12 versus Ctrl+I), but there may be bugs specific to which tool is opened by default.
Flags: qe-verify?
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(poirot.alex)
Flags: needinfo?(andrei.vaida)
Successfully managed to reproduce this bug on 52.0a1 (2016-10-28) (64-bit) (Build ID: 20161028030204) by the following Comment 0's instruction!

This issue is now Verified as Fixed on Latest Firefox Nightly 53.0a1 (2017-01-19) (64-bit)

Build ID: 20170119222621
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0
OS: Linux 4.4.0-57-generic; elementary OS 0.4 (64 Bit)
QA Whiteboard: [testday-20170120]
Verified as fixed on latest Nightly 53.0a1, Buid ID 20170120030214 on Windows 7 x64bit and Mac OS X 10.10.

One note here: 
  If I hold F12 for 3 or more seconds, the devtools toolbox keeps closing and opening, and most of the time it remains closed. There are cases when the devtools toolbox remains opened, and I can close it successfully.
  Is this the intended behavior for the devtools toolbox to keep closing and opening while I hold F12 pressed?
Flags: needinfo?(andrei.vaida) → needinfo?(poirot.alex)
Alex, please fill the uplift request when you are ok!
Comment on attachment 8825520 [details]
Bug 1328004 - Fix race when closing the devtools while a previous instance is still loading.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1266134
[User impact if declined]: Possible broken toolbox on slow hardware.
[Is this code covered by automated tests?]: With this bug, yes!
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]:  Already verified.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Involved the toolbox close codepath, but this is something that is experienced a lot by most tests. I'm still surprise this regression wasn't catched by any existing test... This patch now baked for quite a bit on nightly.
[String changes made/needed]: None.
Flags: needinfo?(poirot.alex)
Attachment #8825520 - Flags: approval-mozilla-beta?
Attachment #8825520 - Flags: approval-mozilla-aurora?
Attachment #8825520 - Flags: approval-mozilla-aurora?
Based on previous comments, mark this verified on Fx 53.
Comment on attachment 8825520 [details]
Bug 1328004 - Fix race when closing the devtools while a previous instance is still loading.

fix race when closing devtools, beta52+
Attachment #8825520 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I have reproduced this bug from STR 1 of Comment 0 with Nightly 52.0a1 (2016-10-28) on Windows 7, 64 bit.

The Bug's fix is now verified on Latest Beta 52.0b8.

Build ID   :    20170220070057
User Agent : 	Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0
I have reproduced this bug from STR 1 of Comment 0 with Nightly 53.0a1 (2017-01-01) on Ubuntu 16.04 LTS, 64 bit!

The bug's fix is now verified on Latest Beta 52.0b8!

Build   ID :  20170220070057
User Agent :  Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0

Thanks Ikram and Maruf for verification!
Updating the tracking flags.
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.