Closed Bug 1201420 Opened 9 years ago Closed 9 years ago

Bug 1035091 (limit @-moz-document to UA and user sheets) breaks "Classic Theme Restorer" add-on.

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Version:
43 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox43 --- fixed

People

(Reporter: alice0775, Unassigned)

References

Details

(Whiteboard: [fixed by backed out bug 1035091]) 

Steps to reproduce:
1. Install latest version of "Classic Theme Restorer" add-on
   https://addons.mozilla.org/ja/firefox/addon/classicthemerestorer/versions/

Actual Results:
Nav Toolbar layout is broken
I guess we could allow it for chrome style sheets as well.  I have a very weak preference not to (just because I'd like what we do for Chrome not to diverge unnecessarily from the Web), but it is *very* weak (given how far we've already lost there).

Anybody know the addon author's bugzilla account, so perhaps they could explain why it's being used and whether it's really needed?
Summary: Bug 1035091 breaks "Classic Theme Restorer" add-on. → Bug 1035091 (limit @-moz-document to UA and user sheets) breaks "Classic Theme Restorer" add-on.
The add-on is using @-moz-document rules in sheets loaded from a constructed data: URL.
And those sheets are being loaded through a call like this, where sheet is the nsIURI object for the data: URL:

  sss.loadAndRegisterSheet(sheet, sss.AGENT_SHEET);
I would have thought that passing AGENT_SHEET here to loadAndRegisterSheet is enough to cause the nsCSSParser to have set to eAgentSheetFeatures, regardless of the URL passed in.  Zack, do you have time to check this?
Flags: needinfo?(zackw)
*have mParsingMode set to eAgentSheetFeatures
Also breaks the add-on Omnisidebar and some of my Stylish css styles aren't working as expected.  I'm sure I could find more 'stuff' that is broken.
Patch also breaks the add-on Stylish, and the add-on FindBar Tweak.
I can't speak for CTR's dev, but as far as OmniSidebar goes (and all my other add-ons as well, FindBar Tweak being one of them), I load my stylesheets as AUTHOR because of the cascading order. Those sheets take precedence over AGENT and USER, which allows me to more easily override any defined properties in Firefox's, and sometimes other add-on's, own sheets.

I tried loading as AGENT (which is what I mostly used before) and USER sheets but those required a hell of a lot of !important tags everywhere, as no CSS selector specificity could override many properties already defined. Not only did that make it hard for me to create dynamic changes and effects (according to states, actions, etc.), that also caused hell for themes and user's customs stylesheets, often making it impossible to override my overrides and style my add-ons properly.

As :heycam explained, sometimes it's not possible to load dynamic sheets into a specific window as an xml-stylesheet node, so I need to use @-moz-document to make sure that sheet only affects the window I actually want to style with it. I do pass AUTHOR_SHEET in these cases, and not AGENT_SHEET though.

BTW wild guess here, but I think Stylish requires this as well because AFAIK web sheets are AUTHOR sheets by definition (are they not?) So it's only logical that any stylesheet meant to style webpages should be loaded as such as well.
Maybe we can enable @-moz-document rules for all style sheets loaded through the style sheet service.
(In reply to Cameron McCormack (:heycam) from comment #9)
> Maybe we can enable @-moz-document rules for all style sheets loaded through
> the style sheet service.

I understand there may be some technical difficulties in achieving this, but FWIW it does seem a bit strange to have a "-moz" prefixed something not work when used through actual "moz" stuff. :)
I agree with Luís Miguel. We are loosing the ability to override sheets, that probably already try to override something for a specific browser page.

It would be great, if at least browsers/add-ons own xul/about pages (e.g browser.xul, about:preferences) could be excluded from this change.

Most of the time CTR uses
@-moz-document url(chrome://browser/content/browser.xul) {
//code
}
to make sure css does not affect anything outside the ui. The recent change forces me to remove everything related to "@-moz-document" for my code to continue to work with negative side effects on web content, if it uses same ids/classes Firefox or CTR use.
I won't be able to look into this in detail until Saturday.  Here are some initial thoughts:

1) Intuitively, loadAndRegisterSheet(..., AGENT_SHEET) does seem like it ought to entail a sheet with agent privileges.  The code _is_ a bit of a tangle; this might just be an oversight, or a merge error.

2) There may be a problem with data: URLs not having a meaningful security principal or something like that.

3) I remember there being something where @import rules sometimes didn't have a loader context and therefore didn't inherit parser mode and so on from their parent.  Is anything like that going on?

4) Logically, sheets injected into documents by add-ons like Stylish _ought to be_ user sheets in terms of the cascade.  If that doesn't work out in practice, maybe that is what needs fixed. (Add a "user override" cascade slot, perhaps?)  I am not familiar with CTR, OmniSidebar, FindBar Tweak, or, honestly, add-on development in general; in particular I have no idea why "sometimes it's not possible to load dynamic sheets into a specific window as an xml-stylesheet node, so I need to use @-moz-document to make sure that sheet only affects the window I actually want to style with it"...  I assume that the "documents" in question are the XUL definitions of chrome, not webpages?

4a) "...override any defined properties in Firefox's, and sometimes other add-on's, own sheets" scares me.  How do we resolve conflicts when two add-ons try to poke the same styles?

5) It was suggested that add-ons' ability to use UA-only style extensions should be removed entirely, because misuse of some of them can violate layout invariants; see bug 1177533.  I'm personally reluctant to do that if there's no evidence that it's contributing to crashes, though.  Note that @-moz-document is *not* one of those.
Flags: needinfo?(zackw)
(In reply to Aris from comment #11)
> Most of the time CTR uses
> @-moz-document url(chrome://browser/content/browser.xul) {
> //code
> }
> to make sure css does not affect anything outside the ui.

Again, I know almost nothing about add-on programming, but it really seems like there ought to be a better way to get this effect.

(Please note: I don't use it myself, but I _will_ make sure CTR keeps working one way or another.  You are providing an essential service for 400,000 people and we are not trying to undermine you.)
Could you at least back off the 'bad' patch then decide what you are going to do?
(In reply to Zack Weinberg (:zwol) from comment #12)
> I won't be able to look into this in detail until Saturday.  Here are some
> initial thoughts:
> 
> 1) Intuitively, loadAndRegisterSheet(..., AGENT_SHEET) does seem like it
> ought to entail a sheet with agent privileges.  The code _is_ a bit of a
> tangle; this might just be an oversight, or a merge error.

Agreed.

> 4) Logically, sheets injected into documents by add-ons like Stylish _ought
> to be_ user sheets in terms of the cascade.  If that doesn't work out in

I thought they were.

> 5) It was suggested that add-ons' ability to use UA-only style extensions
> should be removed entirely, because misuse of some of them can violate
> layout invariants; see bug 1177533.  I'm personally reluctant to do that if
> there's no evidence that it's contributing to crashes, though.  Note that
> @-moz-document is *not* one of those.

I think the security-related switches should perhaps be separate from what we consider here.



I think it may make sense to back out for now.
(In reply to Aris from comment #11)
> I agree with Luís Miguel. We are loosing the ability to override sheets,
> that probably already try to override something for a specific browser page.
> 
> It would be great, if at least browsers/add-ons own xul/about pages (e.g
> browser.xul, about:preferences) could be excluded from this change.
> 
> Most of the time CTR uses
> @-moz-document url(chrome://browser/content/browser.xul) {
> //code
> }
> to make sure css does not affect anything outside the ui. The recent change
> forces me to remove everything related to "@-moz-document" for my code to
> continue to work with negative side effects on web content, if it uses same
> ids/classes Firefox or CTR use.

For what it's worth, I thought extensions had could style sheets to specific XUL documents, via XUL overlays.
I do not object to a backout but I won't be able to do it myself (for the same reason I won't be looking at the code till Saturday).
Does that mean the 'bad' patch will make it to Nightly?  If so, you will have some very unhappy campers.  Yes, I realize it's Nightly but you recognize the problem and it should be backed out by someone.
(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #16)
> (In reply to Aris from comment #11)
> > I agree with Luís Miguel. We are loosing the ability to override sheets,
> > that probably already try to override something for a specific browser page.
> > 
> > It would be great, if at least browsers/add-ons own xul/about pages (e.g
> > browser.xul, about:preferences) could be excluded from this change.
> > 
> > Most of the time CTR uses
> > @-moz-document url(chrome://browser/content/browser.xul) {
> > //code
> > }
> > to make sure css does not affect anything outside the ui. The recent change
> > forces me to remove everything related to "@-moz-document" for my code to
> > continue to work with negative side effects on web content, if it uses same
> > ids/classes Firefox or CTR use.
> 
> For what it's worth, I thought extensions had could style sheets to specific
> XUL documents, via XUL overlays.

They still can, but it is more comfortable use on/off switches, that register/unregister styles for specific documents like this "@-moz-document url(about:preferences) {...}" instead of adding overlays to chrome.manifest file for every xul document a style wants to modify.

Example
Assuming we want to style/change three browser pages at once with one preference "style_abc", that manages style sheets.
a.xul
b.xul
c.xul

Either we create one css file, that gets registered/unregistered globally (but only affects these documents), containing code for these three xul files

@-moz-document url(path://to/a.xul) { code for page a }
@-moz-document url(path://to/b.xul) { code for page b }
@-moz-document url(path://to/c.xul) { code for page c }

or we add three overlay entries to chrome.manifest and create three xul overlays and three js files called by that xul overlays, which register/unregister three css files using own preference listeners. It is not impossible, but would be unnecessarily more complicated compared to what is possible now and was in the past.
Even about:about displays differently.  It is as wide as my screen.
I backed out 1035091 on m-c. Backout will be in tonight's nightly.
(In reply to Zack Weinberg (:zwol) from comment #12)
> 4) Logically, sheets injected into documents by add-ons like Stylish _ought
> to be_ user sheets in terms of the cascade.

Technically true, but that would mean that the webpages (author) stylesheets take precedence over stylish (user) stylesheets, making them much harder to override. So I think practicality dictates they're loaded as AUTHOR as well. I'd love to hear from Stylish's devs if anyone knows their handles here.

(Tangent: reading through the cascade order [1] was weird... author goes later than user, but !important's in those are reversed. I guess that's meant to help with the case for user stylesheets specific to webpages, but I don't think it does much for add-on specific stylesheets. For instance, I avoid the usage of !important tags as much as possible, precisely so those properties are always easily override-able.)

[1] https://developer.mozilla.org/en-US/docs/Web/CSS/Cascade#Cascading_order

> "sometimes it's not possible to load dynamic sheets into a specific window
> as an xml-stylesheet node, so I need to use @-moz-document to make sure that
> sheet only affects the window I actually want to style with it"...  I assume
> that the "documents" in question are the XUL definitions of chrome, not
> webpages?

If I understood you correctly, yes. Stylesheets loaded directly in a xml-stylesheet node inserted into the DOM tree should work fine either way (I think?), but those stylsheets loaded through the nsIStyleSheetService are loaded globally so they need the @-moz-document to restrict them, so they don't style things unwittingly and break them.

> 4a) "...override any defined properties in Firefox's, and sometimes other
> add-on's, own sheets" scares me.  How do we resolve conflicts when two
> add-ons try to poke the same styles?

Isn't that up to the add-ons? I don't follow...

(In reply to Zack Weinberg (:zwol) from comment #13)
> (In reply to Aris from comment #11)
> > Most of the time CTR uses
> > @-moz-document url(chrome://browser/content/browser.xul) {
> > //code
> > }
> > to make sure css does not affect anything outside the ui.
> 
> Again, I know almost nothing about add-on programming, but it really seems
> like there ought to be a better way to get this effect.

AFAIK there's only two ways to load stylesheets: through an xml-stylesheet node or through the nsIStyleSheetService.

I don't think it's possible to load dynamically created sheets (data: strings built in JS) in an xml-stylesheet node, although I haven't tested. Even if it is, if every add-on started doing it, the main window's DOM tree would quickly become chaotic with possibly hundreds, if not thousands, of those (I dare Aris count how many of these sheets CTR has :) ). Plus I'm sure the overhead of having to create/modify/remove nodes like that would take a much larger toll than using the stylesheet service.

(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #16)
> For what it's worth, I thought extensions had could style sheets to specific
> XUL documents, via XUL overlays.

To summarize on Aris' explanation, yes that's probably the easiest method _only_ for XUL-based add-ons (non-restartless) _and_ for actual hardcoded stylesheets; not for other cases where things need to be promptly (un)loaded on command.

BTW Aris' alternate method isn't even valid for restartless add-ons, their chrome.manifest files don't support the overlay entry.
(In reply to Luís Miguel [:Quicksaver] from comment #22)
> (In reply to Zack Weinberg (:zwol) from comment #12)
> > 4) Logically, sheets injected into documents by add-ons like Stylish _ought
> > to be_ user sheets in terms of the cascade.
> 
> Technically true, but that would mean that the webpages (author) stylesheets
> take precedence over stylish (user) stylesheets, making them much harder to
> override. So I think practicality dictates they're loaded as AUTHOR as well.
> I'd love to hear from Stylish's devs if anyone knows their handles here.

Stylish dev here. Stylish used to apply things as user sheets, as it does sound like that what it *should* be. It now by default applies things as author sheets, for two main reasons:

1. Stylish works in other browsers (i.e. Chrome) which have no facility to apply stylesheets as anything but author sheets. For maximum compatibility of styles across browsers, I decided to make stylesheets in Firefox apply as author sheets to match.
2. Some style authors said it made their lives easier. Stylish authors are generally advised to make everything !important, but some don't want to do this, I guess.

Stylish additionally allows style authors to have their styles applied as agent sheets. This is typically used to style things like scrollbars which is not possible in any other mode. Style authors are advised that doing so may cause bugs, crashes, their toast to burn, etc.

> > "sometimes it's not possible to load dynamic sheets into a specific window
> > as an xml-stylesheet node, so I need to use @-moz-document to make sure that
> > sheet only affects the window I actually want to style with it"...  I assume
> > that the "documents" in question are the XUL definitions of chrome, not
> > webpages?
> 
> If I understood you correctly, yes. Stylesheets loaded directly in a
> xml-stylesheet node inserted into the DOM tree should work fine either way
> (I think?), but those stylsheets loaded through the nsIStyleSheetService are
> loaded globally so they need the @-moz-document to restrict them, so they
> don't style things unwittingly and break them.

Sounds right to me. In Stylish's case, these stylesheets come from the user (or the user installed one made by someone else). When Firefox starts up, it grabs all enabled styles, crams each into a data URI, and registers them with nsIStyleSheetService. @-moz-document is the way style authors specify where their style gets applied.

> > 4a) "...override any defined properties in Firefox's, and sometimes other
> > add-on's, own sheets" scares me.  How do we resolve conflicts when two
> > add-ons try to poke the same styles?
> 
> Isn't that up to the add-ons? I don't follow...

The relative ordering of two stylesheets registered with the same mode in nsIStyleSheetService is undefined. I haven't heard of any problem with this.
 
> (In reply to David Baron [:dbaron] ⌚UTC-7 from comment #16)
> > For what it's worth, I thought extensions had could style sheets to specific
> > XUL documents, via XUL overlays.
> 
> To summarize on Aris' explanation, yes that's probably the easiest method
> _only_ for XUL-based add-ons (non-restartless) _and_ for actual hardcoded
> stylesheets; not for other cases where things need to be promptly (un)loaded
> on command.

Yes, Stylish styles can be turned on and off at will. New ones can be added, existing ones can be modified or deleted. Styles can apply to every document ever, or only to a specific URL. Using overlays is not feasible for Stylish.
Hopefully it is OK to disable @-moz-document in style sheets loaded through means that content pages have, like <?xml-stylesheet?>, <link>, and <style>, even if those nodes are inserted into the document by an add-on, since there's not going to be a way to tell that these nodes were inserted by an add-on.  Then, for all style sheets loaded through the style sheet service (which I think captures those loaded through the catalog manager too?), @-moz-document can be enabled, regardless of whether AGENT_SHEET, USER_SHEET or AUTHOR_SHEET is used.

This should still be sufficient to prevent random content from using @-moz-document to perform data exfiltration, which was the original reason for disabling it.
(In reply to Cameron McCormack (:heycam) from comment #24)
> Hopefully it is OK to disable @-moz-document in style sheets loaded through
> means that content pages have, like <?xml-stylesheet?>, <link>, and <style>,
> even if those nodes are inserted into the document by an add-on, since
> there's not going to be a way to tell that these nodes were inserted by an
> add-on.

Those already restrict their stylesheets to the document they're in (which is even more restrictive than @-moz-document, which matches all documents for whatever rules used in it), I can't imagine @-moz-document rules in those would be very useful anyway.

> Then, for all style sheets loaded through the style sheet service
> @-moz-document can be enabled, regardless of whether AGENT_SHEET, USER_SHEET
> or AUTHOR_SHEET is used.

As far as I'm concerned, that should be fine.

> This should still be sufficient to prevent random content from using
> @-moz-document to perform data exfiltration, which was the original reason
> for disabling it.

I wonder if the stylesheet service can somehow be exposed to content, at least in a way that can be taken advantage of like that, through a perhaps poorly conceived add-on... Maybe this is something the AMO review team should keep in mind, that is if they don't already.
fixed by backed out bug 1035091
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
Whiteboard: [fixed by backed out bug 1035091]
Mentioning it here in case I missed adding anyone to bug 1035091 last month.

I have a candidate patch that should allow us to re-land the @-moz-document change for web content without breaking any addons, and I need add-on authors that were affected by the broken version to test their add-ons with a trial build.  Please get the trial build from https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/zackw@panix.com-e63594b7a815/ and report status to bug 1035901.  The trial builds get deleted after a relatively short time so please do this soon.
Argh.  Report status to bug 1035091, not 1035901.
You need to log in before you can comment on or make changes to this bug.