1319928 - csscoverage report tool doesn't seem to work anymore

Status: VERIFIED FIXED

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect, P1)

Description

[Jeff Muizelaar [:jrmuizel]]

It doesn't seem to show anything anymore.

Comment 1

[Markus Stange [:mstange]]

Specifically, the "csscoverage report" command does not open the report panel.

Steps to reproduce:
 1. On any website, open the developer command line using fn+Shift+F2.
 2. In the command line, type "csscoverage start" and hit Enter.
 3. In the command line, type "csscoverage stop" and hit Enter.
 4. Wait for the devtools to open up and display some CSS.
 5. Refocus the developer command line, type "csscoverage report", and hit Enter.

Now the devtools should open a pane with a pie chart and a textbox with reduced CSS.

Regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d26ac63f1b81c3fce35448a7c502e95e0b5c56c0&tochange=54f7bdea89765bb113513f00000160f4dd25c317

I'm guessing at bug 1311178, because that's the only devtools-related change I can find in there.

Comment 2

[Markus Stange [:mstange]]

(In reply to Markus Stange [:mstange] from comment #1)
> because that's the only devtools-related change I can find in there.

Well that wasn't true at all. There's also bug 1311605 and bug 1266134 in there.

Comment 3

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

In local testing, the browser console shows an error with toolbox tool selection:

Error: Can't select tool, wait for toolbox 'ready' 

so, I'll suspect bug 1266134 for the moment.

:ochameau, can you take a look?

Comment 4

[Hani Yacoub, Desktop QA]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20160801030227

I tested this issue on Windows 10 64 bit, using Firefox Nightly 49.0a1, 50.0a1 and Firefox 50 Release.

Comment 5

[Alexandre Poirot [:ochameau]]

Against a website I get two toolboxes, one being empty. Yes, that may be related to bug 1266134.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1319928 - Prevent gDevTools.showToolbox from racing when called multiple times in a row.

Review commit: https://reviewboard.mozilla.org/r/95450/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95450/

Comment 7

[Alexandre Poirot [:ochameau]]

Typical promise/Task race... It could be fixed in a more elegant way if _toolboxes Map was storing only promises but it would force gDevTools.getToolbox to also return promises :/

It wasn't catched by tests as devtools/client/commandline/test/browser_cmd_csscoverage_startstop.js isn't calling gcli commands but instead fronts method directly.
Maintaining a real test may be painful as I'm expecting it to be quite slow and possibly intermittent as it involves loading gcli and a toolbox.

Comment 8

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

[Tracking Requested - why for this release]: Regression in 52, make sure to uplift.

Comment 9

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

Comment on attachment 8814191 [details]
Bug 1319928 - Prevent gDevTools.showToolbox from racing when called multiple times in a row.

https://reviewboard.mozilla.org/r/95450/#review96144

Looks good, I confirmed this fixes the issue.  Thanks for the quick response!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fc1af61ec6de
Prevent gDevTools.showToolbox from racing when called multiple times in a row. r=jryans

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fc1af61ec6de

Comment 12

[Julien Cristau [:jcristau]]

Tracking as new regression in 52.  I guess I'll see an uplift request once this is verified fixed in nightly.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Jeff, can you confirm that this is fixed now?

Comment 14

[Petruta Horea [:phorea]]

53.0a1 2016-12-02 Nightly generates the csscoverage report after following the steps from comment 1.
OSs: Win 10 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.11.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Thanks for the verification! Alexandre, please request Aurora approval on this when you get a chance.

Comment 16

[Alexandre Poirot [:ochameau]]

Comment on attachment 8814191 [details]
Bug 1319928 - Prevent gDevTools.showToolbox from racing when called multiple times in a row.

Approval Request Comment
[Feature/Bug causing the regression]:

  Regressed by bug 1266134.
 
[User impact if declined]:

  In the "Developer toolbar", "csscoverage" command doesn't work.

[Is this code covered by automated tests?]:

  Yes "gDevTools.showToolbox" function is the main codepath for opening devtools. But this "csscoverage" command is tested but with a unit test that missed that regression. It misses a more high level, functional test which is pretty challenging to write.

[Has the fix been verified in Nightly?]:

  Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 

  I think it has already been verified?

[List of other uplifts needed for the feature/fix]:

  None

[Is the change risky?]:

  I feel better with this patch in rather than out as it clearly fix a possible race. The patch itself is quite naive.

[Why is the change risky/not risky?]:

  That's a common codepath when opening devtools, so I think we have a decent coverage with tests. Almost all tests are going to go through this code.

[String changes made/needed]:

  No.

Comment 17

[Julien Cristau [:jcristau]]

Comment on attachment 8814191 [details]
Bug 1319928 - Prevent gDevTools.showToolbox from racing when called multiple times in a row.

fix race condition in devtools

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c64dab018c90