1278823 - " Error: csscoverage.sheetToUrl is not a function" error in jsconsole when focusing a stylesheet in the StyleEditor

Status: VERIFIED FIXED

(DevTools :: Style Editor, defect, P1)

Description

[Nicolas Chevobbe [:nchevobbe]]

STR: 
1. Open `data:text/html,<p>hello` url
2. Open the devtools
3. Go to the style editor tab
4. Click on the "New" button in the left panel

Expected Result:
No error in the jsconsole

Actual Result:
There is an error in the jsconsole

The bug was introduced by https://hg.mozilla.org/mozilla-central/rev/a325b8ebc115 , in which the csscoverage actor and front were decoupled. The `sheetToUrl` lives in the actor file, but we try to call it from the front in StyleEditorUI.jsm (http://searchfox.org/mozilla-central/source/devtools/client/styleeditor/StyleEditorUI.jsm#614).
The function is also called within the actor (http://searchfox.org/mozilla-central/source/devtools/server/actors/csscoverage.js#563), so it should probably be moved to shared.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1278823 - styleeditor: fix csscoverage report creation;

The sheetToUrl function in csscoverage is only used to create ids for the
csscoverage map of knownRules. Instead of asking the UI to format stylesheet
URLs using the same logic as the server, StyleEditor.jsm now sends the
stylesheet actor to create the report. The csscoverage actor can then compute
the stylesheet URL on the server.

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

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1278823 - fix eslint issues in StyleEditorUI.jsm;

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

Comment 3

[Julian Descottes [:jdescottes]]

Pushed to try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=06cea45c8257 (seems green except for unrelated intermittents)

Comment 4

[Tom Tromey :tromey]

Comment on attachment 8763860 [details]
Bug 1278823 - styleeditor: fix csscoverage report creation;

https://reviewboard.mozilla.org/r/59954/#review57084

It looks reasonable but I think there's a backward compatibility issue.

It's worth noting that I am not 100% sure that we care about this.  You may need to find someone else to ask.

::: devtools/client/styleeditor/StyleEditorUI.jsm:619
(Diff revision 1)
>            if (usage == null) {
>              return;
>            }
>  
> -          let href = csscoverage.sheetToUrl(showEditor.styleSheet);
> -          let reportData = yield usage.createEditorReport(href);
> +          let sheet = showEditor.styleSheet;
> +          let {reports} = yield usage.createEditorReportForSheet(sheet);

It seems like this needs a fallback in case it is talking to an older actor.

One option might be to put the fallback code into the front.

Comment 5

[Tom Tromey :tromey]

Comment on attachment 8763861 [details]
Bug 1278823 - fix eslint issues in StyleEditorUI.jsm;

https://reviewboard.mozilla.org/r/59956/#review57086

Thank you.  This is ok.

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8763861 [details]
Bug 1278823 - fix eslint issues in StyleEditorUI.jsm;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59956/diff/1-2/

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8763860 [details]
Bug 1278823 - styleeditor: fix csscoverage report creation;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59954/diff/1-2/

Comment 8

[Julian Descottes [:jdescottes]]

https://reviewboard.mozilla.org/r/59954/#review57084

> It seems like this needs a fallback in case it is talking to an older actor.
> 
> One option might be to put the fallback code into the front.

Good catch! 

I think in this case it would be fair to try catch around the call for createEditorReportForSheet (as it is also done in the same file when calling yield hUtils.getHighlighterByType). 

I would like to avoid having to move the "sheetToUrl" method to a new shared utils for backward compatibility purpose only. From my point of view, we should simply make sure we don't crash here, so it boils down to adding a wrapper in the front or doing the try/catch in the caller. I will try to get a second opinion before landing.

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8763861 [details]
Bug 1278823 - fix eslint issues in StyleEditorUI.jsm;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59956/diff/2-3/

Comment 10

[Julian Descottes [:jdescottes]]

Comment on attachment 8763860 [details]
Bug 1278823 - styleeditor: fix csscoverage report creation;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59954/diff/2-3/

Comment 11

[Julian Descottes [:jdescottes]]

jryans: as Tom pointed out in his review, my change introduces a backward compatibility issue because I added a new method to an actor (createEditorReportForSheet in csscoverage.js). I could use some advice on how to handle this properly.

I assumed that it would be enough to try/catch around calls the new method (which I did in the last submitted patch). 
But maybe we should preserve backward compatibility here, in which case I'll change my approach for the patch and extract the sheetToUrl method to a "shared" file (which I wanted to avoid because this code can really live only on the server).

Comment 12

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

Comment on attachment 8763860 [details]
Bug 1278823 - styleeditor: fix csscoverage report creation;

https://reviewboard.mozilla.org/r/59954/#review57286

Hmm, I may not be following this code correctly...  but it seems like we don't actually need to run `sheetToUrl` on the server at all?  Or am I misunderstanding?  It seems like it had a code path for handling the stylesheet being remote (that is removed in the current patch).

It seems simpler to move `sheetToUrl` to a shared module instead of adding a new actor method.  Is that what you mean as your alternate approach?

Comment 13

[Julian Descottes [:jdescottes]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #12)
> Comment on attachment 8763860 [details]
> Bug 1278823 - styleeditor: fix csscoverage report creation;
> 
> https://reviewboard.mozilla.org/r/59954/#review57286
> 
> Hmm, I may not be following this code correctly...  but it seems like we
> don't actually need to run `sheetToUrl` on the server at all?  Or am I
> misunderstanding?  It seems like it had a code path for handling the
> stylesheet being remote (that is removed in the current patch).
> 
> It seems simpler to move `sheetToUrl` to a shared module instead of adding a
> new actor method.  Is that what you mean as your alternate approach?

Maybe I am missing something, but I understand the opposite. The css-coverage actor computes a map of the used stylesheet rules.
To do so it needs to generate an id for each rule, which uses the spritesheet url as a prefix.

When the UI needs to retrieve the coverage report for a stylesheet, it used to pass the spritesheet URL. The server would match this URL against its map of rules. So client and server had to both generate the same spritesheet URL for a given spritesheet.

The way I see it, the server still needs to build keys to generate its map, but the client doesn't need to know about this logic.

Comment 14

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

Attachment: sheetToUrl comparison

(In reply to Julian Descottes [:jdescottes] from comment #13)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #12)
> > Comment on attachment 8763860 [details]
> > Bug 1278823 - styleeditor: fix csscoverage report creation;
> > 
> > https://reviewboard.mozilla.org/r/59954/#review57286
> > 
> > Hmm, I may not be following this code correctly...  but it seems like we
> > don't actually need to run `sheetToUrl` on the server at all?  Or am I
> > misunderstanding?  It seems like it had a code path for handling the
> > stylesheet being remote (that is removed in the current patch).
> > 
> > It seems simpler to move `sheetToUrl` to a shared module instead of adding a
> > new actor method.  Is that what you mean as your alternate approach?
> 
> Maybe I am missing something, but I understand the opposite. The
> css-coverage actor computes a map of the used stylesheet rules.
> To do so it needs to generate an id for each rule, which uses the
> spritesheet url as a prefix.
> 
> When the UI needs to retrieve the coverage report for a stylesheet, it used
> to pass the spritesheet URL. The server would match this URL against its map
> of rules. So client and server had to both generate the same spritesheet URL
> for a given spritesheet.
> 
> The way I see it, the server still needs to build keys to generate its map,
> but the client doesn't need to know about this logic.

It seems like client and server should reach the same result, because the stylesheet front has enough of the getters on it that a real stylesheet would have.  So, sheetToUrl should be able to go down the same paths and return the same things on both sides.

As a simple check, I pasted sheetToUrl into the style editor and had the client and server log the URLs they found.  They appear to match in my (limited) checks.

(Of course to do this for real the code should be moved to a shared module.)

Comment 15

[Julian Descottes [:jdescottes]]

Thanks for having a look!

First of all, there is a mistake in my patch, sorry about that!

> createEditorReportForSheet: function (stylesheet) {
>   let url = sheetToUrl(stylesheet);
>   return this.createEditorReport(url);
> }

I should pass stylesheet.rawSheet to sheetToUrl, so that sheetToUrl() only ever has to deal with real stylesheet objects.

> createEditorReportForSheet: function (stylesheet) {
>   let url = sheetToUrl(stylesheet.rawSheet);
>   return this.createEditorReport(url);
> }

By any chance, does this change your opinion on my initial approach :) ? 

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #14)
> Created attachment 8765075 [details] [diff] [review]
> sheetToUrl comparison
> 
> It seems like client and server should reach the same result, because the
> stylesheet front has enough of the getters on it that a real stylesheet
> would have.  So, sheetToUrl should be able to go down the same paths and
> return the same things on both sides.
> 
> As a simple check, I pasted sheetToUrl into the style editor and had the
> client and server log the URLs they found.  They appear to match in my
> (limited) checks.
> 

Yes, it works. After all that is what happened before the decoupling, when the code was accessible both to the actor and the front. And that's why this method can handle both a stylesheet actor and a raw stylesheet.

My point is that the client does not need to do it. The only reason to generate the key on the client, is to send it to the server so that the server can find the spritesheet. IMO this introduces a tighter coupling between client and server. This also makes the sheetToUrl method harder to understand, with a first argument that can be a raw stylesheet or a stylesheet actor.

That being said, I really don't want to waste your time with this! I will do as you said and move it to a shared module. Would you have some time for chat/vidyo to explain to me why the shared module approach is cleaner here? I just feel like I'm missing something :(

Comment 16

[Julian Descottes [:jdescottes]]

Comment on attachment 8763860 [details]
Bug 1278823 - styleeditor: fix csscoverage report creation;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59954/diff/3-4/

Comment 17

[Julian Descottes [:jdescottes]]

Regarding my patch, for now I moved the code to the csscoverage front. I found some other actors that were requiring their front files (webaudio, canvas for instance). In this case I think it would be a good fit for the methods, because they are only relevant for the csscoverage front/actor couple. I looked at the other existing files in the shared folder but could not find a good file where I could move the methods. I'm fine with moving them somewhere else though, let me know if there is a better spot.

While actually moving the code to a shared module I found out that we already had two coupling issues between the server and client implementations in sheetToUrl, that prevent the CSS coverage to be displayed for <style> elements in most cases.

Issue #1 : index for style tags:
- server side : uses the index of the stylesheet in document.querySelectorAll("style")
- client side : uses stylesheetActor.styleSheetIndex which is the index of the stylesheet in document.styleSheets

Those indexes only match if you only have stylesheets as style tags, as soon as you have a link stylesheet, it doesn't work. CSS coverage does not work in the StyleEditor in this case.

Issue #2 : document URL
- server side : uses a simplified version of the URL without hash or query parameter
- client side : uses stylesheetActor.nodeHref which is the full href

Those URLs don't match as soon as you are on a page with a hash or query parameters. Again, CSS coverage was not working in the StyleEditor in this case.

I fixed it in my patch, but this is exactly the kind of issues that motivated my initial implementation. As we can see here, the current approach introduces a heavy coupling between client and server implementations.

Comment 18

[Julian Descottes [:jdescottes]]

Comment on attachment 8763860 [details]
Bug 1278823 - styleeditor: fix csscoverage report creation;

Switching to f? to discuss my previous comment.

Comment 19

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

Comment on attachment 8763860 [details]
Bug 1278823 - styleeditor: fix csscoverage report creation;

(In reply to Julian Descottes [:jdescottes] from comment #15)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #14)
> > Created attachment 8765075 [details] [diff] [review]
> > sheetToUrl comparison
> > 
> > It seems like client and server should reach the same result, because the
> > stylesheet front has enough of the getters on it that a real stylesheet
> > would have.  So, sheetToUrl should be able to go down the same paths and
> > return the same things on both sides.
> > 
> > As a simple check, I pasted sheetToUrl into the style editor and had the
> > client and server log the URLs they found.  They appear to match in my
> > (limited) checks.
> > 
> 
> Yes, it works. After all that is what happened before the decoupling, when
> the code was accessible both to the actor and the front. And that's why this
> method can handle both a stylesheet actor and a raw stylesheet.
> 
> My point is that the client does not need to do it. The only reason to
> generate the key on the client, is to send it to the server so that the
> server can find the spritesheet. IMO this introduces a tighter coupling
> between client and server. This also makes the sheetToUrl method harder to
> understand, with a first argument that can be a raw stylesheet or a
> stylesheet actor.
> 
> That being said, I really don't want to waste your time with this! I will do
> as you said and move it to a shared module. Would you have some time for
> chat/vidyo to explain to me why the shared module approach is cleaner here?
> I just feel like I'm missing something :(

In my previous review, I assumed the existing code worked correctly.  If that were actually true, then I would prefer a path that does not add a new RDP message to avoid compat problems.

However, you have presented new information in comment 17 about cases where the existing code is actually broken.

(In reply to Julian Descottes [:jdescottes] from comment #17)
> Regarding my patch, for now I moved the code to the csscoverage front. I
> found some other actors that were requiring their front files (webaudio,
> canvas for instance). In this case I think it would be a good fit for the
> methods, because they are only relevant for the csscoverage front/actor
> couple. I looked at the other existing files in the shared folder but could
> not find a good file where I could move the methods. I'm fine with moving
> them somewhere else though, let me know if there is a better spot.

It's not ideal for the actors to require their fronts.  We ideally want ship the client without the server and the server without the client, so they shouldn't require things from the other side.  See also bug 1278324 about this.  If we were going forward with the shared module path, a new module for this purpose that both the front and actor each require is a better design.

This means the other cases you found of actors requiring fronts are bugs to be fixed.  Feel free to file them!

> While actually moving the code to a shared module I found out that we
> already had two coupling issues between the server and client
> implementations in sheetToUrl, that prevent the CSS coverage to be displayed
> for <style> elements in most cases.
> 
> Issue #1 : index for style tags:
> - server side : uses the index of the stylesheet in
> document.querySelectorAll("style")
> - client side : uses stylesheetActor.styleSheetIndex which is the index of
> the stylesheet in document.styleSheets
> 
> Those indexes only match if you only have stylesheets as style tags, as soon
> as you have a link stylesheet, it doesn't work. CSS coverage does not work
> in the StyleEditor in this case.
> 
> Issue #2 : document URL
> - server side : uses a simplified version of the URL without hash or query
> parameter
> - client side : uses stylesheetActor.nodeHref which is the full href
> 
> Those URLs don't match as soon as you are on a page with a hash or query
> parameters. Again, CSS coverage was not working in the StyleEditor in this
> case.

Thanks, it's good to have these cases, it makes the discussion much easier.  In light of this new info, I agree your previous approach with a new request is better.  Fixing broken behavior outweighs working around compat issues.

Let's rollback to the previous patch you had requested for review, and I'll take another look.  (I don't think I can review old versions in MozReview...?)

Comment 20

[Julian Descottes [:jdescottes]]

Comment on attachment 8763861 [details]
Bug 1278823 - fix eslint issues in StyleEditorUI.jsm;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59956/diff/3-4/

Comment 21

[Julian Descottes [:jdescottes]]

Comment on attachment 8763860 [details]
Bug 1278823 - styleeditor: fix csscoverage report creation;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59954/diff/4-5/

Comment 22

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

Comment on attachment 8763860 [details]
Bug 1278823 - styleeditor: fix csscoverage report creation;

https://reviewboard.mozilla.org/r/59954/#review57842

Thanks for helping me see why this approach was better. :)

Normally we would need to take special care to handle compat for old servers with this approach like :tromey mentioned.  However, for the case of csscoverage (which is controlled through GCLI), I don't think the remote case is relevant because we don't expose GCLI for remote devices, so this should be fine as is.

Thanks for working on this!

Comment 23

[Julian Descottes [:jdescottes]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #22)
> Comment on attachment 8763860 [details]
> Bug 1278823 - styleeditor: fix csscoverage report creation;
> 
> https://reviewboard.mozilla.org/r/59954/#review57842
> 
> Thanks for helping me see why this approach was better. :)
> 
> Normally we would need to take special care to handle compat for old servers
> with this approach like :tromey mentioned.  However, for the case of
> csscoverage (which is controlled through GCLI), I don't think the remote
> case is relevant because we don't expose GCLI for remote devices, so this
> should be fine as is.
> 
> Thanks for working on this!

Thanks for your patience and for hearing me out :)

Rebased and pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b388dd685e56

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/1d60f33e7a44
fix eslint issues in StyleEditorUI.jsm;r=tromey
https://hg.mozilla.org/integration/fx-team/rev/5f550071f889
styleeditor: fix csscoverage report creation;r=jryans

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1d60f33e7a44
https://hg.mozilla.org/mozilla-central/rev/5f550071f889

Comment 26

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 50.0a1 (2016-06-08) on Windows 7, 64 bit !

This bug's fix is verified with latest Developer Edition (Aurora)

Build ID   20160907004009
User Agent Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0
[bugday-20160907]