1276468 - " Error: Unknown sheet source" error in jsconsole when creating a new stylesheet in the StyleEditor

Status: RESOLVED FIXED

(DevTools :: Style Editor, defect, P2)

Description

[Nicolas Chevobbe [:nchevobbe]]

There is no impact on the tool itself, everything works fine, but the jsconsole is cluttered with error messages.

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

Full Error
Error: Unknown sheet source
Stack trace:
exports.sheetToUrl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/csscoverage.js:733:9
StyleEditorUI.prototype._sourceLoaded/<.onShow</<@resource://devtools/client/styleeditor/StyleEditorUI.jsm:614:22
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:310:39
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:816:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:11
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:779:7
this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:714:7
Editor.prototype.appendTo/onLoad@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/sourceeditor/editor.js:456:7
EventListener.handleEvent*Editor.prototype.appendTo@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/sourceeditor/editor.js:459:5
StyleSheetEditor.prototype.load@resource://devtools/client/styleeditor/StyleSheetEditor.jsm:446:12
StyleEditorUI.prototype._sourceLoaded/<.onShow</<@resource://devtools/client/styleeditor/StyleEditorUI.jsm:601:19
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:310:39
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:272:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:246:14
this.Task.spawn@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:160:12
StyleEditorUI.prototype._sourceLoaded/<.onShow<@resource://devtools/client/styleeditor/StyleEditorUI.jsm:596:9
set activeSummary@resource://devtools/client/shared/SplitView.jsm:147:7
StyleEditorUI.prototype._selectEditor/summaryPromise<@resource://devtools/client/styleeditor/StyleEditorUI.jsm:702:7
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:816:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:11
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:779:7
this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:714:7
Front<.onPacket/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:1281:9
DevTools RDP*Front<.request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:1223:7
generateRequestMethods/</frontProto[name]@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:1382:14
StyleSheetEditor.prototype._getSourceTextAndPrettify@resource://devtools/client/styleeditor/StyleSheetEditor.jsm:270:12
StyleSheetEditor.prototype.fetchSource@resource://devtools/client/styleeditor/StyleSheetEditor.jsm:290:12
StyleEditorUI.prototype._addStyleSheetEditor<@resource://devtools/client/styleeditor/StyleEditorUI.jsm:348:11
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:310:39
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:272:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:246:14
StyleEditorUI.prototype._onStyleSheetCreated@resource://devtools/client/styleeditor/StyleEditorUI.jsm:398:5
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:816:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:11
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:779:7
this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:714:7
Front<.onPacket/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:1281:9
DevTools RDP*Front<.request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:1223:7
generateRequestMethods/</frontProto[name]@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:1382:14
StyleEditorUI.prototype.createUI/<@resource://devtools/client/styleeditor/StyleEditorUI.jsm:161:7
EventListener.handleEvent*wire/<@resource://devtools/client/styleeditor/StyleEditorUtil.jsm:161:7
forEach@resource://devtools/client/styleeditor/StyleEditorUtil.jsm:106:7

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

This is not limited to added new rule, but seems to appear also on page with inline <style> tag.
Another STR : 

1. Open : `data:text/html,<style>body{color: blue;}</style><p>hello`
2. Open the devtools and go to the style editor
3. Normally, the stylesheet is automatically selected, if not, select it

You should see the error mentioned in Comment 0

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1276468 - Fix `sheetToUrl` function for inline style.

The function was trying to access StyleSheetActor's ownerNode property, which is undefined.
Switch to nodeHref and styleSheetIndex properties to build the so-called URL.
The built string is only used later in the same file for comparison with another string,
built with the same function, so the syntax of the string does not really matter as long
it is unique.

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

Comment 3

[Joe Walker [:jwalker]]

Comment on attachment 8760415 [details]
Bug 1276468 - Fix `sheetToUrl` function for inline style.

https://reviewboard.mozilla.org/r/57972/#review55004

Looks good to me, thanks!

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8760415 [details]
Bug 1276468 - Fix `sheetToUrl` function for inline style.

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

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

TRY run raised some failures https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9fe131ebd8d , mostly in the gcli tool.
It turns out that the function can be called either with a plain StyleSheet or with a StyleSheetActor.
For now, I just added a simple case for when we have an actor and retrieve similar properties to when we have a StyleSheet. 
Although, I don't know if it's a good behavior to have a function that can be called with 2 different types of parameter, but this is a relative concise function and the types are related. 
We could avoir this by always passing in a StyleSheet parameter, but this would either (1) require extra work, i.e. go through the CSSRule to have access to the parentStyleSheet property (which is what's done in the function call done by the gcli), or (2) adding a `stylesheet`property to the StyleSheetActor, which I think is not worthy just to get around this.

Please, feel free to tell me if you think I'm wrong and we should handle this more gracefully.

Comment 6

[Joe Walker [:jwalker]]

Comment on attachment 8760415 [details]
Bug 1276468 - Fix `sheetToUrl` function for inline style.

https://reviewboard.mozilla.org/r/57972/#review55148

My take on the question of parameter overloading is that it's OK if the 2 different set of possible arguments are disjoint sets with no possibility of overlap.

So it's OK to have a parameter which is either a Person object or their social security ID (for example).

But java.util.List.remove(x) when x can be either an index or the instance to be removed, is dangerous because list.remove(3) is confusing.

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

TRY run is over https://treeherder.mozilla.org/#/jobs?repo=try&revision=7eca1c1f9709&selectedJob=22122677
There is 2 dt* failures, but they seem unrelated and they've been successfully retriggered.

Comment 8

[Carsten Book [:Tomcat]]

has problems to apply: 

applying 95d72f6eed1f
patching file devtools/server/actors/csscoverage.js
Hunk #1 FAILED at 709
1 out of 1 hunks FAILED -- saving rejects to file devtools/server/actors/csscoverage.js.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue

could you take a look, thanks!

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8760415 [details]
Bug 1276468 - Fix `sheetToUrl` function for inline style.

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

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Carsten Book [:Tomcat] from comment #8)
> has problems to apply: 
> 
> applying 95d72f6eed1f
> patching file devtools/server/actors/csscoverage.js
> Hunk #1 FAILED at 709
> 1 out of 1 hunks FAILED -- saving rejects to file
> devtools/server/actors/csscoverage.js.rej
> patch failed to apply
> abort: fix up the working directory and run hg transplant --continue
> 
> could you take a look, thanks!

I rebased (see Comment 9) and pushed to patch to review again. Can you tell me if it's okay for you ?


Also, unrelated to you, but the STR in Comment 0 now shows a new error, introduced by https://hg.mozilla.org/mozilla-central/diff/a325b8ebc115/devtools/client/styleeditor/StyleEditorUI.jsm (the function is called on the front but only exists in the actor). I'll file a new bug for this.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/0aab68e30c0e
Fix `sheetToUrl` function for inline style. r=jwalker

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0aab68e30c0e