stylo: support ServoStyleSheets in nsIStyleSheetService::loadAndRegisterSheet

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: xidorn, Assigned: heycam)

Tracking

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment)

This is needed by layout/style/test_additional_sheets.html

Otherwise, this test crashes with the following stack:
Assertion failure: IsServo(), at c:\mozilla-source\stylo\obj-firefox-stylo\dist\include\mozilla/StyleSheetInlines.h:16
#01: mozilla::PresShell::AddAgentSheet (c:\mozilla-source\stylo\layout\base\presshell.cpp:1554)
#02: mozilla::PresShell::Observe (c:\mozilla-source\stylo\layout\base\presshell.cpp:9586)
#03: nsObserverList::NotifyObservers (c:\mozilla-source\stylo\xpcom\ds\nsobserverlist.cpp:112)
#04: nsObserverService::NotifyObservers (c:\mozilla-source\stylo\xpcom\ds\nsobserverservice.cpp:285)
#05: nsStyleSheetService::LoadAndRegisterSheet (c:\mozilla-source\stylo\layout\base\nsstylesheetservice.cpp:188)
#06: XPTC__InvokebyIndex (c:\mozilla-source\stylo\xpcom\reflect\xptcall\md\win32\xptcinvoke_asm_x86_64.asm:99)
#07: CallMethodHelper::Call (c:\mozilla-source\stylo\js\xpconnect\src\xpcwrappednative.cpp:1331)
#08: XPCWrappedNative::CallMethod (c:\mozilla-source\stylo\js\xpconnect\src\xpcwrappednative.cpp:1296)
#09: XPC_WN_CallMethod (c:\mozilla-source\stylo\js\xpconnect\src\xpcwrappednativejsops.cpp:983)
#10: js::CallJSNative (c:\mozilla-source\stylo\js\src\jscntxtinlines.h:239)
heycam, could you have a look?
Flags: needinfo?(cam)
I think the way we should solve this is to store both Gecko and Servo style sheets in the nsStyleSheetService, and perhaps to detect if we've never created a document with a particular style backend, and defer the actual creation of the sheet of that type until it's needed.

One complication is that the {agent,user,author}-sheet-added observer service notifications use the CSSStyleSheet pointer, and nsDocuments observe this to add the sheet to their style sets, so that would probably need to change to pass both (or some kind of ID?).  Luckily there is exactly one addon in DXR that observes these notifications, and it doesn't do anything when it receives them.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Depends on: 1340090
I maybe could have split this patch up some, but I didn't, sorry.  This patch turns nsStyleSheetService::mSheets into two parallel arrays, mGeckoSheets and mServoSheets, and whenever we call loadAndRegisterSheet and unregisterSheet, we work on both arrays at once.  In non-MOZ_STYLO builds, we just stick nullptrs in mServoSheets, but in MOZ_STYLO builds, we parse and load both a CSSStyleSheet and a ServoStyleSheet.  (If one fails, then we consider the load overall to have failed.)  The main accessor methods AgentStyleSheets, UserStyleSheets and AuthorStyleSheets are given a StyleBackendType argument, and all the call sites updated to pass the right one in.

This makes test_additional_sheets.html pass in Stylo builds except for one sub-test, which I haven't looked into yet, but which I guess is some dynamic restyling issue.
Flags: needinfo?(cam)
Comment on attachment 8839087 [details]
Bug 1337258 - Support ServoStyleSheets in nsStyleSheetService.

https://reviewboard.mozilla.org/r/113846/#review115494

r=me with the copy issue below fixed.

::: dom/base/nsDocument.cpp:2290
(Diff revision 1)
>      RemoveStyleSheetsFromStyleSets(mOnDemandBuiltInUASheets, SheetType::Agent);
>      RemoveStyleSheetsFromStyleSets(mAdditionalSheets[eAgentSheet], SheetType::Agent);
>      RemoveStyleSheetsFromStyleSets(mAdditionalSheets[eUserSheet], SheetType::User);
>      RemoveStyleSheetsFromStyleSets(mAdditionalSheets[eAuthorSheet], SheetType::Doc);
>  
> -    if (GetStyleBackendType() == StyleBackendType::Gecko) {
> +    if (nsStyleSheetService* sheetService = nsStyleSheetService::GetInstance()) {

I'm thinking that we can probably rename `GetInstance` to `Instance` and remove all the null-check of this... It doesn't seem to me `nsStyleSheetService::GetInstance()` can ever return a nullptr. It is unrelated to this bug, though.

::: layout/base/nsStyleSheetService.cpp:405
(Diff revision 1)
>  
>  size_t
>  nsStyleSheetService::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const
>  {
>    size_t n = aMallocSizeOf(this);
> -  for (auto& sheetArray : mSheets) {
> +  for (auto& sheetArrays : { mGeckoSheets, mServoSheets }) {

This is probably not a good idea. Per cppreference [1]
> The underlying array is a temporary array of type const T[N], in which each element is copy-initialized ... from the corresponding element of the original initializer list.

So it seems you are basically copying the two arrays here, which could be expensive.

C++ doesn't seem to have concept of array of references. So probably use pointers here? e.g.
> for (auto* sheetArrays : { &mGeckoSheets, &mServoSheets })

This should avoid the copy.

[1] http://en.cppreference.com/w/cpp/utility/initializer_list
Attachment #8839087 - Flags: review?(xidorn+moz) → review+
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/840ecb1c7d65
Support ServoStyleSheets in nsStyleSheetService. r=xidorn
https://hg.mozilla.org/mozilla-central/rev/840ecb1c7d65
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.