Multiple tests cause ASSERTIONS when system add-ons import AUTHOR_SHEET style sheets (ASSERTION: Style set already has document sheets?: 'aStyleSet->SheetCount(SheetType::Doc) == 0')

RESOLVED FIXED in Firefox 45

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: standard8, Assigned: gkrizsanits)

Tracking

33 Branch
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

4 years ago
In bug 1223573 we're trying to move Hello to a system add-on. As part of our code we have:

  let sheets = ["chrome://loop/content/addon/css/loop.css",
                "chrome://loop/skin/platform.css"];
  for (let sheet of sheets) {
    let styleSheetURI = Services.io.newURI(sheet, null, null);
    if (styleSheetService.sheetRegistered(styleSheetURI,
                                          styleSheetService.AUTHOR_SHEET)) {
      styleSheetService.unregisterSheet(styleSheetURI,
                                        styleSheetService.AUTHOR_SHEET);
    }
  }

However, when we run it through try, there is a debug assertion raised on lots of different tests:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2be8d2c8c797&selectedJob=14172398
https://treeherder.mozilla.org/logviewer.html#?job_id=14172398&repo=try
 14:48:52     INFO -  [1191] ###!!! ASSERTION: Style set already has document sheets?: 'aStyleSet->SheetCount(SheetType::Doc) == 0', file /builds/slave/try-m64-d-00000000000000000000/build/src/dom/base/nsDocument.cpp, line 2365
 14:49:49     INFO -  #01: nsDocument::ResetStylesheetsToURI(nsIURI*) [xpcom/glue/nsCOMPtr.h:403]
 14:49:49     INFO -  #02: nsDocument::ResetToURI(nsIURI*, nsILoadGroup*, nsIPrincipal*) [mfbt/RefPtr.h:268]
 14:49:49     INFO -  #03: nsHTMLDocument::ResetToURI(nsIURI*, nsILoadGroup*, nsIPrincipal*) [mfbt/RefPtr.h:40]
 14:49:49     INFO -  #04: nsDocument::Reset(nsIChannel*, nsILoadGroup*) [mfbt/RefPtr.h:40]
 14:49:49     INFO -  #05: nsHTMLDocument::Reset(nsIChannel*, nsILoadGroup*) [dom/html/nsHTMLDocument.cpp:234]
 14:49:49     INFO -  #06: nsHTMLDocument::Open(JSContext*, nsAString_internal const&, nsAString_internal const&, mozilla::ErrorResult&) [xpcom/glue/nsCOMPtr.h:721]
 14:49:49     INFO -  #07: mozilla::dom::HTMLDocumentBinding::open [mfbt/AlreadyAddRefed.h:138]
 14:49:49     INFO -  #08: mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) [dom/bindings/BindingUtils.cpp:2671]
 14:49:49     INFO -  #09: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) [js/src/jscntxtinlines.h:236]

If we change AUTHOR_SHEET to either USER_SHEET or AGENT_SHEET, then the tests no longer fail, however due to the CSS cascade doing this means we have to add a lot of !important annotations to the majority of our rules to get them to apply correctly.
Reporter

Comment 1

4 years ago
For now, I've worked around this in attachment 8692901 [details] [diff] [review], but as you can see, its really ugly, and I won't be surprised if it causes us issues at some stage.
Looks like nsDocument::FillStyleSet adds style sheets from the stylesheet service to the style set, but nsDocument::ResetStylesheetsToURI never removes them, right?

Looks like a bug in the initial patch for bug 676054 which no one ever noticed, perhaps because no one ever tried to call document.open() in a debug build with any actual author sheets in the stlylesheet service...

Gabor, do you have bandwidth to look at this?
Blocks: 676054
Component: DOM → CSS Parsing and Computation
Flags: needinfo?(gkrizsanits)
Assignee

Updated

4 years ago
Flags: needinfo?(gkrizsanits)
QA Contact: gkrizsanits
Assignee

Comment 3

4 years ago
Sorry hit the wrong "assigned to".
Assignee: nobody → gkrizsanits
QA Contact: gkrizsanits
Reporter

Updated

4 years ago
Blocks: 1228999
Assignee

Comment 4

4 years ago
Thanks for spotting out the problem. I totally overlooked it. The test is not pretty but I didn't have any better idea at the moment... I'm not even sure I want to land that test, just wanted to see if it fixes the problem.
Attachment #8693554 - Flags: review?(bzbarsky)
Comment on attachment 8693554 [details] [diff] [review]
resetting AuthorStyleSheets. v1

Nice test!  r=me
Attachment #8693554 - Flags: review?(bzbarsky) → review+

Comment 7

4 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d9a1efa7d1ab
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Reporter

Comment 8

4 years ago
I've just tried removing our workaround, and try shows it as working fine now.

Thank you for fixing this quickly.
Reporter

Updated

4 years ago
No longer blocks: 1226706
You need to log in before you can comment on or make changes to this bug.