Closed Bug 1328004 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools :: Framework, defect)

Product:
DevTools
Component:
Framework
Version:
27 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

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

Status:
VERIFIED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- verified
firefox53 --- verified

People

(Reporter: arni2033, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

Bug 1328004 - Fix race when closing the devtools while a previous instance is still loading.
59 bytes, text/x-review-board-request
jryans
: review+
jcristau
: approval-mozilla-beta+
Details 
>>>   My Info:   Win7_64, Nightly 52, 32bit, ID 20161028030204 (2016-10-28)
STR_1:
1. Open url   data:text/html,<body></body>
2. Hold F12 for 3 seconds
3. Click close button in devtools toolbox

STR_2:
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:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a98a1d78817f&tocha
nge=58eca03214a6

I was able to narrow down the regression window.
status-firefox50: --- → wontfix
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
Version: Trunk → 27 Branch
The regression range you got is a bit strange, because I assumed the it's regression from bug 1266134:
> https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=d9413eb3db64787916c60efcb7b766cc24420d94&tochange=2268d466af6cbf62fc48006d3ff00d087c339e8b

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.

https://reviewboard.mozilla.org/r/103636/#review104618

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 apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7dc8c7773abf
Fix race when closing the devtools while a previous instance is still loading. r=jryans
https://hg.mozilla.org/mozilla-central/rev/7dc8c7773abf
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Please request Aurora approval on this when you get a chance.
status-firefox51: affected → wontfix
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!
Merci
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.
status-firefox53: fixed → verified
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

[bugday-20170222]
Status: RESOLVED → VERIFIED
Thanks Ikram and Maruf for verification!
Updating the tracking flags.
status-firefox52: fixed → verified
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: