Last Comment Bug 1328004 - Holding F12 breaks devtools toolbox and it can't be closed
: Holding F12 breaks devtools toolbox and it can't be closed
Status: RESOLVED FIXED
: regression
Product: Firefox
Classification: Client Software
Component: Developer Tools: Framework (show other bugs)
: 27 Branch
: Unspecified Unspecified
-- normal (vote)
: Firefox 53
Assigned To: Alexandre Poirot [:ochameau]
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
: 1330573 (view as bug list)
Depends on:
Blocks: 1327717
  Show dependency treegraph
 
Reported: 2017-01-01 03:21 PST by arni2033
Modified: 2017-02-06 07:22 PST (History)
8 users (show)
poirot.alex: qe‑verify+
See Also:
Crash Signature:
(edit)
QA Whiteboard: [testday-20170120]
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
fixed
verified

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
Bug 1328004 - Fix race when closing the devtools while a previous instance is still loading. (59 bytes, text/x-review-board-request)
2017-01-10 12:44 PST, Alexandre Poirot [:ochameau]
jryans: review+
jcristau: approval‑mozilla‑beta+
Details | Review

Description User image arni2033 2017-01-01 03:21:27 PST
>>>   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.
Comment 1 User image Justin [:JW_SoftvisionQA] [PTO 2/20/2017-2/21/2017] 2017-01-03 15:21:08 PST
 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.
Comment 2 User image arni2033 2017-01-03 15:29:25 PST
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
Comment 3 User image Alexandre Poirot [:ochameau] 2017-01-05 10:31:17 PST
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbe2bc5b031c
Comment 4 User image Alexandre Poirot [:ochameau] 2017-01-10 08:48:31 PST
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b923eba09483
Comment 5 User image Alexandre Poirot [:ochameau] 2017-01-10 12:44:03 PST Comment hidden (mozreview-request)
Comment 6 User image Alexandre Poirot [:ochameau] 2017-01-10 12:46:02 PST
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 7 User image J. Ryan Stinnett [:jryans] (use ni?) 2017-01-11 09:42:51 PST
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...?
Comment 8 User image Alexandre Poirot [:ochameau] 2017-01-12 02:42:57 PST Comment hidden (mozreview-request)
Comment 9 User image Julian Descottes [:jdescottes] 2017-01-12 02:58:36 PST
*** Bug 1330573 has been marked as a duplicate of this bug. ***
Comment 10 User image Pulsebot 2017-01-12 08:50:54 PST
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
Comment 11 User image Wes Kocher (:KWierso) 2017-01-12 15:45:07 PST
https://hg.mozilla.org/mozilla-central/rev/7dc8c7773abf
Comment 12 User image Ryan VanderMeulen [:RyanVM] 2017-01-16 15:44:25 PST
Please request Aurora approval on this when you get a chance.
Comment 13 User image Alexandre Poirot [:ochameau] 2017-01-17 07:14:27 PST
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.
Comment 14 User image Nazir Ahmed Sabbir [:NaSb] 2017-01-20 03:27:01 PST
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)
Comment 15 User image Brindusa Tot[:brindusat] 2017-01-20 08:00:38 PST
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?
Comment 16 User image Sylvestre Ledru [:sylvestre] 2017-01-25 05:51:25 PST
Alex, please fill the uplift request when you are ok!
Merci
Comment 17 User image Alexandre Poirot [:ochameau] 2017-02-02 06:33:03 PST
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.
Comment 18 User image Brindusa Tot[:brindusat] 2017-02-03 02:23:16 PST
Based on previous comments, mark this verified on Fx 53.
Comment 19 User image Julien Cristau [:jcristau] 2017-02-06 07:06:42 PST
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+
Comment 20 User image Carsten Book [:Tomcat] 2017-02-06 07:22:42 PST
https://hg.mozilla.org/releases/mozilla-beta/rev/9a1b79873db7

Note You need to log in before you can comment on or make changes to this bug.