1337258 - stylo: support ServoStyleSheets in nsIStyleSheetService::loadAndRegisterSheet

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

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)

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

heycam, could you have a look?

Comment 2

[Cameron McCormack (:heycam)]

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.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Bug 1337258 - Support ServoStyleSheets in nsStyleSheetService.

Review commit: https://reviewboard.mozilla.org/r/113846/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/113846/

Comment 4

[Cameron McCormack (:heycam)]

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.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8839087 [details]
Bug 1337258 - Support ServoStyleSheets in nsStyleSheetService.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113846/diff/1-2/

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8839087 [details]
Bug 1337258 - Support ServoStyleSheets in nsStyleSheetService.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113846/diff/2-3/

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8839087 [details]
Bug 1337258 - Support ServoStyleSheets in nsStyleSheetService.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113846/diff/3-4/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/840ecb1c7d65
Support ServoStyleSheets in nsStyleSheetService. r=xidorn

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/840ecb1c7d65