Closed Bug 1276468 Opened 5 years ago Closed 5 years ago

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

Categories

(DevTools :: Style Editor, defect, P2)

49 Branch
defect

Tracking

(firefox49 affected, firefox50 fixed)

RESOLVED FIXED
Firefox 50
Tracking Status
firefox49 --- affected
firefox50 --- fixed

People

(Reporter: nchevobbe, Assigned: nchevobbe)

Details

Attachments

(1 file)

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
Assignee: nobody → chevobbe.nicolas
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
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/
Attachment #8760415 - Flags: review?(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!
Attachment #8760415 - Flags: review?(jwalker) → review+
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/
Attachment #8760415 - Attachment description: Bug 1276468 - Fix function for inline style. → Bug 1276468 - Fix `sheetToUrl` function for inline style.
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.
Attachment #8760415 - Flags: review+ → review?(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.
Attachment #8760415 - Flags: review?(jwalker) → review+
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.
Keywords: checkin-needed
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!
Flags: needinfo?(chevobbe.nicolas)
Keywords: checkin-needed
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/
(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.
Flags: needinfo?(chevobbe.nicolas)
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/0aab68e30c0e
Fix `sheetToUrl` function for inline style. r=jwalker
https://hg.mozilla.org/mozilla-central/rev/0aab68e30c0e
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.