"Add new rule" doesn't work on http://vosteran.com/

NEW
Unassigned

Status

()

Firefox
Developer Tools: CSS Rules Inspector
P2
normal
2 years ago
a year ago

People

(Reporter: ntim, Unassigned)

Tracking

unspecified
Points:
---

Firefox Tracking Flags

(firefox42 affected, firefox45 affected)

Details

(Whiteboard: [btpp-fix-later], URL)

(Reporter)

Description

2 years ago
13:30:09.798 TypeError: sheet is null
Stack trace:
PageStyleActor<.addNewRule<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/styles.js:981:9
TaskImpl_run@resource://gre/modules/Task.jsm:314:40
TaskImpl@resource://gre/modules/Task.jsm:275:3
createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:249:14
actorProto/</handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1013:19
DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1643:15
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:569:11
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/DevToolsUtils.js:87:14
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/DevToolsUtils.js:87:14
1 protocol.js:907
(Reporter)

Comment 1

2 years ago
Doesn't work in 42 or 45, haven't tested on other version, but I'm pretty sure it doesn't work on 43 or 44.
status-firefox42: --- → affected
status-firefox45: --- → affected
(Reporter)

Comment 2

2 years ago
So it looks like one of the stylesheets doesn't exist on the page.

:gl, :tromey, any idea why this might be failing ?
Flags: needinfo?(ttromey)
Flags: needinfo?(gl)
This still doesn't work in FF47.
It actually comes from a CSP policy on this site (style-src).
https://developer.mozilla.org/fr/docs/Web/Security/CSP/CSP_policy_directives#style-src

The headers on this page define this CSP and this blocks the creation of inline <style> sheet.
However, the "add rule" feature depends on this.

I think we've had this problem already in the past and I seem to remember that CSP policies were not supposed to apply to devtools code. But I can't remember the exact bug where this was discussed.

I'm tempted to triage this as a P1, but I don't know if we have the bandwidth at the moment to investigate and solve this potentially complex problem, and this has been here for a while and hasn't had really bad consequences from what I can see.

I'll try to ping a few people, hopefully I get a lead on how to start fixing this.
Flags: needinfo?(ttromey)
Flags: needinfo?(gl)
Priority: -- → P2
Whiteboard: [btpp-fix-later]
See Also: → bug 1185351
This is similar to bug 1185351 which was about the highlighter. The highlighter sets style attributes on dynamically injected native anonymous content in the page, and this used to be blocked by CSP. Bug 1185351 basically made CSP ignore native anon content.

Here, we're not using native anon content, devtools is creating a (normal) <style> DOM node to inject new rules that the user is creating in the inspector.

According to https://en.wikipedia.org/wiki/Content_Security_Policy#Browser_add-ons_and_extensions_exemption browser code should be exempt from this.

Christoph: how should we proceed here? It looks like a CSP bug.
Flags: needinfo?(ckerschb)
Component: Developer Tools: Inspector → Developer Tools: CSS Rules Inspector
(In reply to Patrick Brosset [:pbro] from comment #4)
> Here, we're not using native anon content, devtools is creating a (normal)
> <style> DOM node to inject new rules that the user is creating in the
> inspector.

Right, so applying the same fix as within Bug 1185351 where we just used | IsInNativeAnonymousSubtree() | doesn't apply here. 

> Christoph: how should we proceed here? It looks like a CSP bug.

I don't think it's a CSP bug though, but you are right, we should not apply CSP when adding a rule. Ehsan probably knows how we can exempt such loads from CSP. Ehsan, any ideas?

Just for the sake of completeness, here is the CSP error I receive in the console:
> Content Security Policy: The page's settings blocked the loading of a resource at self ("style-src http://vosteran.com").

However, I get a slightly different stacktrace, see below:

console.error: 
  Error writing response to: addStyleSheet
*************************
A coding exception was thrown in a Promise resolution callback.
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Full message: TypeError: this.rawSheet is null
Full stack: StyleSheetActor<.ownerNode@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/stylesheets.js:441:5
StyleSheetActor<.form@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/stylesheets.js:581:1
types.addActorType/type<.write@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:294:44
RetVal<.write@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:529:12
Response<.write/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:682:16
Response<.write@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:680:23
actorProto/</handler/sendReturn@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1023:24
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
Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:454:5
actorProto/</handler/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1043:18
Actor<._queueResponse@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:920:20
actorProto/</handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1042:9
DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1643:15
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:569:11
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14

*************************
console.error: 
  Message: TypeError: this.rawSheet is null
  Stack:
    StyleSheetActor<.ownerNode@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/stylesheets.js:441:5
StyleSheetActor<.form@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/stylesheets.js:581:1
types.addActorType/type<.write@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:294:44
RetVal<.write@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:529:12
Response<.write/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:682:16
Response<.write@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:680:23
actorProto/</handler/sendReturn@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1023:24
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
Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:454:5
actorProto/</handler/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1043:18
Actor<._queueResponse@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:920:20
actorProto/</handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1042:9
DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1643:15
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:569:11
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14

StyleSheetActor<.ownerNode@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/stylesheets.js:441:5
StyleSheetActor<.form@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/stylesheets.js:581:1
types.addActorType/type<.write@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:294:44
RetVal<.write@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:529:12
Response<.write/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:682:16
Response<.write@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:680:23
actorProto/</handler/sendReturn@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1023:24
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
Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:454:5
actorProto/</handler/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1043:18
Actor<._queueResponse@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:920:20
actorProto/</handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1042:9
DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1643:15
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:569:11
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
Flags: needinfo?(ckerschb) → needinfo?(ehsan)

Comment 6

2 years ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #5)
> > Christoph: how should we proceed here? It looks like a CSP bug.
> 
> I don't think it's a CSP bug though, but you are right, we should not apply
> CSP when adding a rule. Ehsan probably knows how we can exempt such loads
> from CSP. Ehsan, any ideas?

Comment 0 contains 0 information about what this bug is about, and how the stacktrace there (or the ones in comment 5) are captured.  Unfortunately I don't understand this bug at all, so I can't really answer this question in any useful way.

Can you please express the question in a self-contained paragraph?  :)
Flags: needinfo?(ehsan) → needinfo?(ckerschb)
(Reporter)

Comment 7

2 years ago
Ehsan, sorry for the lack of precision, here are some STR:
- Go to http://vosteran.com
- Open inspector
- Click the '+' icon next to 'Filter Styles'

See how nothing happens (no rule is created). The stack trace can be captured simply by opening the Browser Toolbox.
(In reply to Tim Nguyen [:ntim] from comment #7)
> Ehsan, sorry for the lack of precision, here are some STR:
> - Go to http://vosteran.com
> - Open inspector
> - Click the '+' icon next to 'Filter Styles'
> 
> See how nothing happens (no rule is created). The stack trace can be
> captured simply by opening the Browser Toolbox.

Thanks Tim for the STR.

Ehsan, basically the problem is that the site's CSP applies to devtools code, but devtools code should be exempt from the site's CSP. We had a similar problem within Bug 1185351, where using |IsInNativeAnonymousSubtree()| fixed the problem and the site's CSP did not apply to devtools code anymore.

The problem here is slightly different though, because we are not using native anon code. Devtools code is rather creating a regular <style> DOM Node which allows injecting new user rules created through the inspector. I added a stacktrace within Comment 5 (see also original stacktrace in Comment 0).

Any idea how we can exempt such DOM Node creations from being blocked by CSP?
Flags: needinfo?(ckerschb) → needinfo?(ehsan)

Comment 9

2 years ago
As far as I know, there is no real difference between a style element added by devtools code versus one added by the web page itself, so as things stand, there is no way to differentiate on the source of the style element, and therefore it's impossible to make it not be subject to the site's CSP.  The case of a node in a native anonymous subtree is different since native anonymous subtrees are not exposed to the web page script, so there's no way for such an element to come from the web page.

It seems like in this case, devtools needs to do something different.  Injecting nodes into the DOM is not really OK since those nodes will be visible to the web page script, they can change the meaning of CSS selectors, etc.
Flags: needinfo?(ehsan)
Worth noting: clicking on the "new" button in the style-editor to create a new stylesheet also fails on this site for the same reason.
Here are the interesting parts of the errors displayed in the console when this happens:

> Error writing response to: addStyleSheet protocol.js:1025
> 
> Content Security Policy: The page's settings blocked the loading of a resource at self ("style-src http://vosteran.com")./:1
> 
> Full Message: Protocol error (unknownError): this.rawSheet is null

The style-editor tries to create an inline stylesheet in the page just like when using the "Add rule" button in the inspector.

Interestingly, this works fine in Chrome even if they also insert a <style> element in the DOM.

As Ehsan said, we either need a way to differentiate DOM elements/attributes added/modified by devtools so we can avoid them being subject to the site's CSP, or find another way to inject stylesheets.

The only other way I can think of now is attaching an author stylesheet using the sdk:
https://developer.mozilla.org/en-US/Add-ons/SDK/Low-Level_APIs/stylesheet_style
Something like this:

let {Style} = require("sdk/stylesheet/style");
let {attach} = require("sdk/content/mod");
let style = Style({
  source: "body{background:blue;}",
  type: "author"
});
attach(style, win);

This works even with the site's CSP. The "author" type makes it show nicely in the rule-view, however the new sheet doesn't appear in the style-editor somehow. Also, the SDK Style constructor doesn't seem to return an object we can later use to modify the sheet, so we'd have to detach and re-attach a new sheet every time a property is modified/added/deleted.

Comment 11

2 years ago
Yeah, doing something along those lines is a good idea.  I'm not sure how the add-on SDK stylesheet API works but the presshell supports a number of APIs for loading various types of stylesheets without injecting style nodes into the DOM: <https://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.h#582>  Some of these (IIRC the agent stylesheets for example) are exposed from nsIPresShell, and we can expose more of them if needed.
You need to log in before you can comment on or make changes to this bug.