Closed Bug 1244074 Opened 9 years ago Closed 9 years ago

support multiple style sheet backends

Categories

(Core :: CSS Parsing and Computation, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla47
Tracking Flags:
Tracking Status
firefox47 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 4 obsolete files)

Part 1: Move SheetParsingMode to a separate file.
4.87 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
Part 3: Add skeleton ServoStyleSheet and a StyleSheetHandle smart pointer.
16.85 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
Part 2: Add HandleRefPtr for refcounting StyleSheetHandles. (v2) r=dholbert
2.91 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
Part 4: Use StyleSheetHandle instead of concrete style sheet class in most places. (v2)
259.45 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
Similar refactoring to bug 1244068, but for CSSStyleSheet. In these patches we introduce a HandleRefPtr smart pointer class to use with StyleSheetHandle, since RefPtr<StyleSheetHandle> won't work.
See bug 1244068 comment 0 for the try run.
Comment on attachment 8713574 [details] [diff] [review] Part 1: Move SheetParsingMode to a separate file. Review of attachment 8713574 [details] [diff] [review]: ----------------------------------------------------------------- r=me, just one nit: ::: layout/style/SheetParsingMode.h @@ +1,2 @@ > +#ifndef mozilla_css_SheetParsingMode_h > +#define mozilla_css_SheetParsingMode_h This file needs a modeline & license boilerplate.
Attachment #8713574 - Flags: review?(dholbert) → review+
Comment on attachment 8713575 [details] [diff] [review] Part 2: Add HandleRefPtr for refcounting StyleSheetHandles. Review of attachment 8713575 [details] [diff] [review]: ----------------------------------------------------------------- I'm a bit hesitant about adding a new RefPtr-like class from scratch, but maybe that really is what we need here... I'll dig in more tomorrow & think more about this. For now, just one nit: ::: layout/style/HandleRefPtr.h @@ +27,5 @@ > +{ > +public: > + HandleRefPtr() {} > + > + MOZ_IMPLICIT HandleRefPtr(HandleRefPtr<T>& aRhs) I don't think you need MOZ_IMPLICIT on this constructor or the next one. The MOZ_IMPLICIT documentation says it "...must be used for constructors which intend to provide implicit *conversions*" (emphasis added). These constructors are not conversions, because the parameter is the same type as the thing we're constructing (with an ampersand or two) -- they're copy/move-constructors, not conversion constructors. Backing me up, RefPtr.h doesn't use a MOZ_IMPLICIT annotation on its RefPtr-accepting constructors: http://mxr.mozilla.org/mozilla-central/source/mfbt/RefPtr.h?rev=de1fd96512f7&mark=68-68,77-77#68 ...though it does on its constructors which take "T" (since those are indeed conversions.) @@ +41,5 @@ > + mHandle = aRhs; > + aRhs.mHandle = nullptr; > + } > + > + MOZ_IMPLICIT HandleRefPtr(T aRhs) (The MOZ_IMPLICIT is correct here, though, since T is a different type from HandleRefPtr<T>.)
So my concern/hesitancy about HandleRefPtr is: - Refcounting is tricky to get right (and to make automagic) in all edge cases. - Mistakes can lead to leaks or use-after-free security bugs. - Therefore, if at all possible, we should share code with our existing RefPtr infrastructure to the extent that makes sense, so that we can benefit from its bulletproofing, and so that bugfixes to one pointer-type will Just Work for the other pointer type. With that in mind, here's a strawman counter-proposal, as an alternative to creating this new custom "HandleRefPtr". * Refactor our existing RefPtr class, such that it inherits from a AbstractRefPtr<P> superclass, where type "P" is assumed to be either a pointer or something "pointer-like". In particular, we'd require that P satisfies the following things: - P must implement operator->. - We can call ->AddRef() and ->Release() on instances of P. - We can assign instances of P to nullptr. - P must support boolean conversion, and this boolean conversion must return false IFF it's been assigned to nullptr. (I think all of these things are true of "real" pointers to refcounted objects, and they're also true of your StyleSheetHandle objects.) * Define the class AbstractRefPtr<P> { ... } such that it owns an instance of "P mPtrLike", and it would call mPtrLike->AddRef and mPtrLike->Release at appropriate times (assignment, destruction, etc), in the same way that RefPtr does (and your custom-made HandleRefPtr class does). * Redefine the normal RefPtr<T> class as deriving from AbstractRefPtr<T*>, with any additional pointer-specific functionality that RefPtr needs. * For your patches here, you'd use AbstractRefPtr<StyleSheetHandle>. Thoughts? (Maybe you even already tried something like this and ran into trouble? And, do you agree with my concerns & see any other way that we might mitigate them?)
Comment on attachment 8713577 [details] [diff] [review] Part 3: Add skeleton ServoStyleSheet and a StyleSheetHandle smart pointer. Review of attachment 8713577 [details] [diff] [review]: ----------------------------------------------------------------- r=me on part 3, with nits below addressed. ::: layout/style/ServoStyleSheet.cpp @@ +9,5 @@ > +using namespace mozilla; > +using namespace mozilla::dom; > + > +nsIURI* > +ServoStyleSheet::GetSheetURI() const The function-implementations here should be inside of "namespace mozilla { ... }", and you can probably drop 'using namespace mozilla;' as a result. At least, the coding style guide says: Prefer to wrap code in namespace ... { ... }; instead [of writing 'using namespace ...',] if possible. https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style ::: layout/style/ServoStyleSheet.h @@ +6,5 @@ > + > +#ifndef mozilla_ServoStyleSheet_h > +#define mozilla_ServoStyleSheet_h > + > +#include "mozilla/StyleSheetHandle.h" Probably worth adding the following, to provide the "nsAString" type (which is used further down in this file): #include "nsStringFwd.h" (That's the only thing I see that's not directly provided by our existing StyleSheetHandle.h include.) ::: layout/style/StyleSheetHandle.h @@ +10,5 @@ > +#include "mozilla/css/SheetParsingMode.h" > +#include "mozilla/net/ReferrerPolicy.h" > +#include "mozilla/CORSMode.h" > +#include "mozilla/HandleRefPtr.h" > +#include "nsIURI.h" Can we just forward-declare nsIURI here (maybe 10 lines further down, just after "class nsIDocument")? I only see nsIURI being mentioned as a pointer type in this file (and I don't see any methods being called on these pointers), which I think means a forward-decl is OK. @@ +19,5 @@ > +} // namespace dom > +class CSSStyleSheet; > +class ServoStyleSheet; > +} // namespace mozilla > +class nsIDocument; We probably need a forward-decl for nsIPrincipal, too. (At least, there are several "nsIPrincipal*" usages in this file.) @@ +28,5 @@ > +#ifdef MOZ_STYLO > +#define SERVO_BIT_IF_ENABLED SERVO_BIT > +#else > +#define SERVO_BIT_IF_ENABLED 0x0 > +#endif My question in bug 1244068 comment 11 about SERVO_BIT_IF_ENABLED applies here, too. @@ +40,5 @@ > +public: > + typedef HandleRefPtr<StyleSheetHandle> RefPtr; > + > + class Ptr > + { As in bug 1244068, please include a comment here explaining the separation of concerns between "Ptr" & "StyleSheetHandle". @@ +82,5 @@ > + const ServoStyleSheet* GetAsServo() const { return IsServo() ? AsServo() : nullptr; } > + > + // These inline methods are defined in StyleSheetHandleInlines.h. > + inline MozExternalRefCountType AddRef(); > + inline MozExternalRefCountType Release(); Add the #include for this type at the top of this file, to define this "MozExternalRefCountType" return value: #include "mozilla/RefCountType.h" @@ +149,5 @@ > + > + // Make StyleSheetHandle usable in boolean contexts. > +#ifdef MOZ_HAVE_REF_QUALIFIERS > + operator void*() const & { return reinterpret_cast<void*>(mPtr.mValue); } > + operator void*() const && = delete; (My confusion/question about the void*/MOZ_HAVE_REF_QUALIFIERS stuff in bug 1244068 applies here too; if there's any changes/cleanup that you want to make to this chunk in the other bug, you'll want to do it here as well of course.)
Attachment #8713577 - Flags: review?(dholbert) → review+
Update: I ran my strawman AbstractRefPtr proposal by Waldo, to get a second opinion (and in particular a MFBT-expert opinion). He shares the concerns I expressed in comment 9, but he thinks your HandleRefPtr solution is a better way forward than my strawman AbstractRefPtr alternative, since RefPtr.h is already pretty complicated and splitting it up would make it even messier. SO: with that in mind, I'm going to proceed with reviewing "part 2", and I'm going to punt to Waldo for a secondary review, since I don't trust myself to be the only set of eyes to sanity-check it.
Comment on attachment 8713575 [details] [diff] [review] Part 2: Add HandleRefPtr for refcounting StyleSheetHandles. Review of attachment 8713575 [details] [diff] [review]: ----------------------------------------------------------------- I think this is r=me with review comments below addressed. When you've addressed my review feedback, please have Waldo do a secondary review on the result, so that I'm off the hook if I miss something important. :) ::: layout/style/HandleRefPtr.h @@ +4,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. > + */ > + > +/* smart pointer for strong references to objects through handles */ "through handles" is perhaps a little too abstract/vague here. (I don't really know what "handle" means in this context. In particular: a pointer is a type of handle, but this class isn't for use with pointers.) Maybe: s/through handles/through pointer-like 'handle' objects/ @@ +36,5 @@ > + MOZ_IMPLICIT HandleRefPtr(HandleRefPtr<T>&& aRhs) > + { > + if (mHandle) { > + mHandle->Release(); > + } I don't think we need this mHandle->Release() business here -- this is a constructor, so our mHandle has only just now been constructed. So it should be falsey, and it shouldn't have anything that we could release. (assuming a reasonable definition of T) @@ +37,5 @@ > + { > + if (mHandle) { > + mHandle->Release(); > + } > + mHandle = aRhs; This assignment is a little confusing -- mHandle has type T, whereas aRhs has type HandleRefPtr<T>&&, so I'm not intuitively sure what assigning one to the other does. Could you make this more explicit with s/aRhs/aRhs.mHandle/ here? (I think it'll do the same thing but just be a bit clearer.) @@ +73,5 @@ > + operator T() const { return mHandle; } > + T operator->() const { return mHandle; } > + > + void forget(T* aRhs) > + { This "forget" method scares me a little, since it feels like it'd be easy to misuse accidentally and leak. It seems like it's trying to be analogous to RefPtr::forget(T** aOut), which is also a bit of a footgun, but which is mostly safe because we pretty much use it exclusively with getter_AddRefs. But here with HandleRefPtr, there's no getter_AddRefs idiom to make things clear & make the ownership transfer as clear. IIUC, you only need this "forget" method for this one spot in your last patch here, where we call "sheet.forget(&handle); *aSheet = handle->AsGecko();" to produce an already-addref'ed nsIDOMStyleSheet outparam. https://bugzilla.mozilla.org/attachment.cgi?id=8713940&action=diff#a/layout/base/nsStyleSheetService.cpp_sec6 I'd prefer that we get rid of this "forget" method, and instead do the outparam-setting with the help of a dedicated "traditional" RefPtr of the correct concrete type, like so: StyleSheetHandle::RefPtr sheet; [... code which sets up sheet ...] RefPtr<CSSStyleSheet> geckoSheet = sheet.AsGecko(); geckoSheet.forget(aSheet); I haven't tested this, but I'm assuming this or something like it works. This means we'll do one extra AddRef/Release here, which is a small cost. But since this seems to be pretty rare, that doesn't seem too bad. Worth it, IMO, to make the forget()ing as concrete as possible, to avoid the chance of leaking or other mischief. @@ +92,5 @@ > + mHandle->Release(); > + } > + mHandle = aPtr; > + if (mHandle) { > + mHandle->AddRef(); Waldo pointed out when I was discussing with him -- you should be doing the AddRef on the new value *before* you do the release on the old value. Otherwise, self-assignment will likely trigger a use-after-free. (we'll release & delete, and then assign, and then addref the already-deleted value). To the extent possible, you should probably be mimicking code-patterns from RefPtr here (including the order in which things are performed), to avoid pitfalls like this.
Attachment #8713575 - Flags: review?(dholbert) → review+
Comment on attachment 8713575 [details] [diff] [review] Part 2: Add HandleRefPtr for refcounting StyleSheetHandles. Review of attachment 8713575 [details] [diff] [review]: ----------------------------------------------------------------- One more thing I forgot to mention in part 2: ::: layout/style/HandleRefPtr.h @@ +62,5 @@ > + ~HandleRefPtr() { assign(nullptr); } > + > +#ifdef MOZ_HAVE_REF_QUALIFIERS > + operator void*() const & { return mHandle; } > + operator void*() const && = delete; As mentioned in a similar chunk of code on the StyleSet bug, I'm confused about what these void* operators and MOZ_HAVE_REF_QUALIFIERS are trying to achieve (and why our bool-conversion operator is behind this ifdef). Perhaps this is so we can have comparisons to "nullptr"? (If that's all this is for, perhaps we can just do away with any such comparisons so that this whole chunk can be condensed down to the boolean conversion operators? I can see expectations of nullptr-comparisons working for RefPtr, but I think I'm fine with them not working for this type, since this is several steps removed from a true pointer anyway, and since bool-conversion does the same thing as nullptr-comparison more concisely.)
Comment on attachment 8713940 [details] [diff] [review] Part 4: Use StyleSheetHandle instead of concrete style sheet class in most places. (v2) Review of attachment 8713940 [details] [diff] [review]: ----------------------------------------------------------------- Review notes for "Part 4", wave 1: ::: dom/base/nsDocument.cpp @@ +1330,3 @@ > NS_ASSERTION(sheet, "Null sheet in sheet list!"); > + // XXXheycam ServoStyleSheets don't expose their title yet. > + sheet->AsGecko()->GetTitle(title); Unjustified AsGecko() dependency. In general, these need either: - a MOZ_ASSERT(sheet->IsGecko(), ...) with a message explaining why sheet must be gecko-flavored here. - or, a stub servo codepath - or, no explicit servo/gecko dependency at all (e.g. with a stub GetTitle impl on the servo backend which warns/fails/does nothing for now). For this specific spot: I think you want to add "if (sheet->IsServo()) { continue; }" before the call to GetTitle. At least, that's what you do before GetTitle calls elsewhere in this file. @@ +9849,5 @@ > } > > nsresult > nsDocument::LoadChromeSheetSync(nsIURI* uri, bool isAgentSheet, > + mozilla::StyleSheetHandle::RefPtr& aSheet) tl;dr: This should probably be mozilla::StyleSheetHandle::RefPtr*, not RefPtr&. Right now, this patch is inconsistent on how to represent "StyleSheetHandle::RefPtr" as an outparam, in conversions from getter_AddRefs/CSSStyleSheet** in the old code. Here, the patch converts it to a C++ reference, i.e. StyleSheetHandle::RefPtr&. But in most other cases, the patch converts to a pointer, i.e. a StyleSheetHandle::RefPtr*. See for example the part of this patch for Loader.h's LoadSheetSync, LoadSheet, & CreateSheet functions. Probably best to make that consistent, for the conversions that you're performing in this patch. I tend to lean in favor of using a pointer (rather than a C++ reference), to make the outparam-ness clearer from the callsite. Bonus points if there's a way to make getter_AddRefs still Just Work and produce a StyleSheetHandle::RefPtr* here (though maybe that'd be overkill). (Side note: nsLayoutStylesheetCache has some outparams that were _already_ using RefPtr<>& as an outparam, which you're converting to StyleSheetHandle::RefPtr& in this patch. I'm less concerned with whether those should use &/* in this patch, since those are straight string-replaces. But we might eventually want to bring those into alignment with whatever you decide for the other conversions here.) @@ +10188,2 @@ > RefPtr<CSSStyleSheet> clonedSheet = > + sheet->AsGecko()->Clone(nullptr, nullptr, clonedDoc, nullptr); Unjustified AsGecko() dependency. @@ +10203,2 @@ > RefPtr<CSSStyleSheet> clonedSheet = > + sheet->AsGecko()->Clone(nullptr, nullptr, clonedDoc, nullptr); Unjustified AsGecko() dependency. ::: dom/base/nsIDocument.h @@ +1008,5 @@ > mozilla::css::Loader* CSSLoader() const { > return mCSSLoader; > } > > + mozilla::StyleImplementation GetStyleImplementation() const; (Per bug 1244068 comment 14 & comments leading up to it, this function-name needs s/Implementation/BackendType/) ::: layout/style/CSSStyleSheet.h @@ +210,5 @@ > nsIDocument* GetDocument() const { return mDocument; } > > void SetTitle(const nsAString& aTitle) { mTitle = aTitle; } > void SetMedia(nsMediaList* aMedia); > + nsINode* GetOwningNode() const { return mOwningNode; } It looks like you're aiming to rename an old function, "GetOwnerNode", to use this new name, "GetOwningNode". But you left behind the old function (GetOwnerNode). Please drop that old version, if you're adding this new version, so we're not left with two subtly-differently-named functions that do the same thing. It would also probably be better to split these function-rename changes (here & the callsites for GetOwnerNode/GetOwningNode) into a separate patch, because this patch is already huge (multiple hundreds of KB), and this function-rename seems orthogonal to the other changes here.
Comment on attachment 8713940 [details] [diff] [review] Part 4: Use StyleSheetHandle instead of concrete style sheet class in most places. (v2) Review of attachment 8713940 [details] [diff] [review]: ----------------------------------------------------------------- Review notes for "Part 4", wave 2 (not quite done yet): ::: dom/base/nsStyleLinkElement.cpp @@ +490,5 @@ > nsIDocument* document = thisContent->GetOwnerDocument(); > > if (thisContent->IsInShadowTree()) { > ShadowRoot* containingShadow = thisContent->GetContainingShadow(); > + containingShadow->RemoveSheet(sheet); Can this line just stay unchanged? (The old code passes mStyleSheet to RemoveSheet here; you're changing it to pass "sheet" instead.) This patch is changing the type of mStyleSheet, but it's also changing the type that ShadowRoot::RemoveSheet expects. So I'd expect that this should Just Work, without this patch needing to touch this line of code. (and I'd expect that it might *not* work with |sheet| as you've got it here, though maybe there's some automagic casting that makes it work). This also goes for 3 more calls just after this (InsertSheet, a second RemoveStyleSheet call, and a call to document->AddStyleSheet(sheet) -- all of these will now expect StyleSheetHandle, which means we should probably still be passing mStyleSheet instead of |sheet|.) ::: dom/base/nsStyleLinkElement.h @@ +40,5 @@ > > + mozilla::CSSStyleSheet* GetSheet() const > + { > + return mStyleSheet && mStyleSheet->IsGecko() ? mStyleSheet->AsGecko() : > + nullptr; This probably needs a brief comment explaining why this method is Gecko-specific. (or an XXX comment saying we should broaden it to include servo, if this is just a stopgap) ::: dom/ipc/ContentChild.cpp @@ +2562,5 @@ > static void > PreloadSlowThings() > { > // This fetches and creates all the built-in stylesheets. > + nsLayoutStylesheetCache::For(StyleImplementation::Gecko).UserContentSheet(); Why is this gecko-specific? (Explicitly using ::Gecko) The comment should probably hint at this -- or, if we're planning to broaden this somehow, there should be an XXX comment mentioning this. Otherwise it's just mysterious. ::: dom/xbl/nsXBLPrototypeResources.cpp @@ +145,5 @@ > nsXBLPrototypeResources::GatherRuleProcessor() > { > + nsTArray<RefPtr<CSSStyleSheet>> sheets(mStyleSheetList.Length()); > + for (StyleSheetHandle sheet : mStyleSheetList) { > + sheets.AppendElement(sheet->AsGecko()); Unjustified AsGecko() usage. (It's not clear to me where mStyleSheets comes from or how we'd know they're all gecko-flavored stylesheets) @@ +147,5 @@ > + nsTArray<RefPtr<CSSStyleSheet>> sheets(mStyleSheetList.Length()); > + for (StyleSheetHandle sheet : mStyleSheetList) { > + sheets.AppendElement(sheet->AsGecko()); > + } > + mRuleProcessor = new nsCSSRuleProcessor(sheets, So after this patch, |sheets| will get its contents copied here (in the nsCSSRuleProcessor constructor) -- and then |sheets| dies. This means every entry in the array gets AddRef'ed (in the copy operation) and then Release'd (when |sheets| dies), which is a lot of churn. If this code is going to stay like this (with a local nsTArray that gets destructed at the end of this function), it'd probably be worth making nsCSSRuleProcessor use C++ Move to steal the contents of the passed-in array, instead of copying it. (Not sure if that's appropriate for everywhere that instantiates a nsCSSRuleProcessor, but it'd be very much appropriate for this callsite at least, as long as we're working with this temporary nsTArray.) ::: editor/nsIEditorStyleSheets.idl @@ +4,5 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > #include "nsISupports.idl" > > +[scriptable, uuid(d8c05150-f9f3-49e5-88f0-1e03af3d936f)] You shouldn't bother updating the UUID, according to bug 1170718 & this dev.platform post from a few days ago: https://groups.google.com/d/msg/mozilla.dev.platform/HE1_qZhPj1I/rElS7KjXAQAJ ::: layout/base/RestyleTracker.cpp @@ +19,5 @@ > #include "RestyleTrackerInlines.h" > #include "nsTransitionManager.h" > #include "mozilla/RestyleTimelineMarker.h" > +#include "mozilla/StyleSheetHandle.h" > +#include "mozilla/StyleSheetHandleInlines.h" Do we really need these includes here in RestyleTracker.cpp? These are literally the only instances of the word "sheet" in this file, so I'd bet we don't use anything from these headers here... ::: layout/base/nsPresShell.cpp @@ +1452,5 @@ > > // Document specific "additional" Author sheets should be stronger than the ones > // added with the StyleSheetService. > + CSSStyleSheet* firstAuthorSheet = > + mDocument->FirstAdditionalAuthorSheet()->AsGecko(); Unjustified AsGecko() dependency. @@ +4428,5 @@ > > if (mStylesHaveChanged) > return; > > + if (aStyleSheet->IsGecko()) { Before this patch, we wrap the arg (aStyleSheet) in a local RefPtr variable here. (I don't immediately see why we do that, but we do.) Looks like you're removing that here. Please make sure this is OK -- i.e. that we're not depending on this local RefPtr to keep us alive for the duration of some operation here or something. (The RefPtr was added by you back in 2013, here: http://hg.mozilla.org/mozilla-central/rev/a526743bbd75#l2.104 -- so I'll trust your judgement on whether it's needed. Maybe it never was?) ::: layout/base/nsStyleSheetService.cpp @@ +156,5 @@ > // mSheets[aSheetType] > + > + // XXXheycam Once the nsStyleSheetService can hold ServoStyleSheets too, > + // we'll need to include them in the notification. > + CSSStyleSheet* sheet = mSheets[aSheetType].LastElement()->AsGecko(); Unjustified AsGecko() dependency. @@ +268,5 @@ > + > + StyleSheetHandle handle; > + sheet.forget(&handle); > + > + *aSheet = handle->AsGecko(); Unjustified AsGecko() dependency. @@ +303,5 @@ > nsCOMPtr<nsIObserverService> serv = services::GetObserverService(); > if (serv) { > + // XXXheycam Once the nsStyleSheetService can hold ServoStyleSheets too, > + // we'll need to include them in the notification. > + CSSStyleSheet* cssSheet = sheet->AsGecko(); Unjustified AsGecko() dependency. ::: layout/inspector/inDOMUtils.cpp @@ +109,5 @@ > // Get the document sheets. > for (int32_t i = 0; i < document->GetNumberOfStyleSheets(); i++) { > + // XXXheycam ServoStyleSets don't have the ability to expose their > + // sheets in a script-accessible way yet. > + sheets.AppendElement(document->GetStyleSheetAt(i)->AsGecko()); Unjustified AsGecko() dependency. ::: layout/style/CSSStyleSheet.cpp @@ +2205,4 @@ > bool aWasAlternate, > nsresult aStatus) > { > + CSSStyleSheet* sheet = aSheet->AsGecko(); Unjustified AsGecko() dependency. (This one seems like it's pretty legit, because we're inside of CSSStyleSheet itself, and I'd guess (?) that CSSStyleSheet::StyleSheetLoaded may only be called for CSSStyleSheet instances -- not for ServoStyleSheet instances [though, I'm not sure about that]. Still: it'd be worth an IsGecko() assertion with a brief explanation.) ::: layout/style/Loader.cpp @@ +1127,5 @@ > nsXULPrototypeCache* cache = nsXULPrototypeCache::GetInstance(); > if (cache) { > if (cache->IsEnabled()) { > sheet = cache->GetStyleSheet(aURI); > + LOG((" From XUL cache: %p", static_cast<void*>(sheet))); Hmm, this must be why you've got the void* operator -- for all the LOG() statements in this file. (optional): I'd prefer that we had a real method, called something like StyleSheetHandle::AsVoidPtr() (name like "AsGecko()" / "AsServo()"). And I think that function should clear the SERVO_BIT before it returns (which the current void*-conversion operator does not do IIRC), unless you plan on watching for & caring about the SERVO_BIT in the logged output. And it can be documented as being *only* for convenient logging, and not to be used otherwise. What do you think? (The void* cast feels too automagic to me -- it feels like we could trigger it accidentally somewhere. Maybe I'm being overly concerned, though...) @@ +1149,5 @@ > > if (sheet) { > // This sheet came from the XUL cache or our per-document hashtable; it > // better be a complete sheet. > + NS_ASSERTION(sheet->AsGecko()->IsComplete(), Unjustified AsGecko() dependency, here & throughout the rest of this function (lots of AsGecko() calls). (I think the justification is your "XXXheycam Cached sheets currently must be CSSStyleSheets" comment a ways up from this; but if you're depending on that here, we probably should be asserting it as well.) @@ +1263,3 @@ > } > > + NS_ASSERTION(aSheet, "We should have a sheet by now!"); This line's change (dropping a "*") needs to be reverted -- you really want to be asserting about "*aSheet", not "aSheet". aSheet is a pointer to a RefPtr. You want to be asserting that we have populated the passed-in RefPtr (i.e. that *aSheet is truthy). But right now, you're just asserting that some non-null pointer-to-RefPtr was passed in (that aSheet is truthy). So: bring back the * here, to leave this line as it was before this patch. @@ +1416,5 @@ > > + // XXXheycam The InsertChildSheet API doesn't work with ServoStyleSheets, > + // since they won't have Gecko ImportRules in them. > + if (aSheet->IsServo()) { > + return NS_OK; (Is NS_OK the right thing to return here? Maybe a failure code would be better?) @@ +1774,5 @@ > > aCompleted = false; > > + // XXXheycam ServoStyleSheets don't support parsing their contents yet. > + nsCSSParser parser(this, aLoadData->mSheet->AsGecko()); Unjustified AsGecko() dependency. (need an assertion w/ hint at why we know servo can't get here, or a fallback codepath for servo -- whichever is appropriate) @@ +1938,5 @@ > // one of the sheets that will be kept alive by a document or > // parent sheet anyway, so that if someone then accesses it via > // CSSOM we won't have extra clones of the inner lying around. > data = aLoadData; > + CSSStyleSheet* sheet = aLoadData->mSheet->AsGecko(); Unjustified AsGecko() dependency. (here as well as a few lines below, "data->mSheet->AsGecko()") @@ +2205,5 @@ > + if (!parent) { > + break; > + } > + topSheet = parent; > + } If you like, I believe the 7-line for(;;;) loop here could be condensed to these 3 lines: while (StyleSheetHandle parent = topSheet->GetParentSheet()) { topSheet = parent; } @@ +2242,5 @@ > LOG((" No parent load; must be CSSOM")); > // No parent load data, so the sheet will need to be notified when > // we finish, if it can be, if we do the load asynchronously. > + // XXXheycam ServoStyleSheet doesn't implement nsICSSLoaderObserver yet. > + observer = aParentSheet->AsGecko(); Unjustified AsGecko() usage. (I'm not clear on whether the XXX comment is intending (a) to declare that we won't get here with a servo stylesheet ...or: (b) to say we can't do anything useful if we *do* get here with a servo stylesheet. If the former, this probably wants an assert with a version of that XXX comment as its message. If the latter, it probably wants some stub fallback code for the IsServo case.) @@ +2643,5 @@ > iter.Next()) { > // If aSheet has a parent, then its parent will report it so we don't > // have to worry about it here. Likewise, if aSheet has an owning node, > // then the document that node is in will report it. > + StyleSheetHandle sheet = iter.UserData(); Maybe declare this as a "const StyleSheetHandle", if you can. (It's const in the old code that you're replacing; and it looks like we're just calling getters, so I think const should be OK.) (It's nice to have some foolproofing in our memory-measurement functions, to be sure they don't actually mess with the stuff they're measuring. :) ) ::: layout/style/StyleSetHandleInlines.h @@ +145,4 @@ > { > + if (IsGecko()) { > + // How safe would it be to reinterpret_cast aNewSheets to > + // |const nsTArray<RefPtr<CSSStyleSheet>>|? (answering the question in this comment): I wouldn't be surprised if this worked, but I would be very hesitant to actually take that code. :) Seems way too fragile/dangerous. Maybe drop this comment, unless you're actually seriously considering doing this. Seems like a better solution (to avoid copying) would be to make CSSStyleSheet::ReplaceSheets accept the more general parameter -- nsTArray<StyleSheetHandle::RefPtr> -- and assert that each entry in that array IsGecko before using it. Maybe file a followup on this, to avoid introducing a perf regression from the temporary-array & copying here?
I'll let Cameron have the authoritative voice here, but just to clarify - the goal of this patch is to get some of our infrastructure in-tree in order to make it easier for us to continue development and collaboration and minimize bitrot. The Servo implementation isn't done yet, and the goal here is just to avoid regressing the Gecko behavior/performance when MOZ_STYLO is not set. So any code that safely crashes in the Servo case is totally fine, and I don't want to block these patches until we have answers for all of the places where Cameron needed to do AsGecko. Does that seem reasonable?
Flags: needinfo?(dholbert)
(In reply to Bobby Holley (busy) from comment #16) > So any code that safely crashes in the Servo case is totally fine I assume this is in reference to my "Unjustified AsGecko() dependency" comments. (1) In the servo case, these will not be safe crashes -- the AsGecko() invocation will return a CSSStyleSheet* which actually points to 1 byte beyond the start of a ServoStyleSheet object. Calling functions on this pointer will produce strange behavior. (2) I am totally OK with having safe fallback or safe crashes for the servo case. For example, if each of my "Unjustified AsGecko() dependency" review-comments were addressed with... if (foo->IsServo()) { MOZ_CRASH("servo codepath needs triage/followup here"); } that would be fine with me, because any crashes will be safe, and we can easily go back and triage these spots, and fill them in with "real" Servo codepaths as-needed. My concern with the current patch's AsGecko() calls is that some of them are justified, some of them are maybe-justified (not sure), and some of them seem definitely-not-justified (which means unsafe crashes). And we have no easy way after-the-fact to search for the ones that need fixing. > and I > don't want to block these patches until we have answers for all of the > places where Cameron needed to do AsGecko. Agreed. But for the places where we *do* have a "This AsGecko usage is definitely fine" answer, we should annotate (and sanity-check) the reason with a MOZ_ASSERT. And for the places we're not sure about (or that we know are not-fine), we need to (a) make sure we crash or fail safely, and (b) ideally use a consistent string in a comment or assertion/crash-message, so that we can go back and triage these after the fact.
Flags: needinfo?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #17) > (In reply to Bobby Holley (busy) from comment #16) > > So any code that safely crashes in the Servo case is totally fine > > I assume this is in reference to my "Unjustified AsGecko() dependency" > comments. > > (1) In the servo case, these will not be safe crashes -- the AsGecko() > invocation will return a CSSStyleSheet* which actually points to 1 byte > beyond the start of a ServoStyleSheet object. Calling functions on this > pointer will produce strange behavior. > > (2) I am totally OK with having safe fallback or safe crashes for the servo > case. For example, if each of my "Unjustified AsGecko() dependency" > review-comments were addressed with... > if (foo->IsServo()) { > MOZ_CRASH("servo codepath needs triage/followup here"); > } > that would be fine with me, because any crashes will be safe, and we can > easily go back and triage these spots, and fill them in with "real" Servo > codepaths as-needed. > > My concern with the current patch's AsGecko() calls is that some of them are > justified, some of them are maybe-justified (not sure), and some of them > seem definitely-not-justified (which means unsafe crashes). And we have no > easy way after-the-fact to search for the ones that need fixing. > > > and I > > don't want to block these patches until we have answers for all of the > > places where Cameron needed to do AsGecko. > > Agreed. But for the places where we *do* have a "This AsGecko usage is > definitely fine" answer, we should annotate (and sanity-check) the reason > with a MOZ_ASSERT. And for the places we're not sure about (or that we know > are not-fine), we need to (a) make sure we crash or fail safely, and (b) > ideally use a consistent string in a comment or assertion/crash-message, so > that we can go back and triage these after the fact. Makes sense! A MOZ_ASSERT reason-string convention should be good enough to grep for untriaged situations down the line.
Cool. (Though, MOZ_ASSERT probably isn't sufficient for untriaged situations, because those would still let opt builds crash unsafely. Either MOZ_CRASH or assert + early-return would be better for those cases, IMO.)
(In reply to Daniel Holbert [:dholbert] from comment #19) > Cool. (Though, MOZ_ASSERT probably isn't sufficient for untriaged > situations, because those would still let opt builds crash unsafely. Either > MOZ_CRASH or assert + early-return would be better for those cases, IMO.) Depending on how hot the functions are, sure. Easiest is to release-assert everwhere, but if we're concerned about overhead, we can also just make triaging these things a blocker before shipping stylo to actual users.
I'm not too concerned about code-hotness as long as MOZ_STYLO is default-disabled, because hopefully compilers should be able to optimize away all our IsServo() checks and any code that they guard. (since IsServo() always returns false, when MOZ_STYLO is disabled) When MOZ_STYLO is approaching being ready to be default-enabled, code-hotness will be more of something to worry about, but hopefully at that point we'll have all of these hypothetical MOZ_CRASH statements triaged & replaced with correct code (or some sort of safe fallback code, or an assertion explaining why we're sure we don't have to bother checking for servo).
Comment on attachment 8713940 [details] [diff] [review] Part 4: Use StyleSheetHandle instead of concrete style sheet class in most places. (v2) Review of attachment 8713940 [details] [diff] [review]: ----------------------------------------------------------------- Review notes for "Part 4", wave 3 (final wave): ::: layout/style/nsLayoutStylesheetCache.cpp @@ +756,5 @@ > } > > /* static */ void > +nsLayoutStylesheetCache::InvalidateSheet(StyleSheetHandle::RefPtr* aGeckoSheet, > + StyleSheetHandle::RefPtr* aServoSheet) This function is pretty confusing. (It takes a gecko and a servo stylesheet, either (or both) of which could be null), and it doesn't actually do anything gecko/servo-specific with them -- so we don't really know (or care) that they're gecko/servo sheets. Could you add the following sanity-check assertions: // Make sure sheets have the expected types: MOZ_ASSERT(!aGeckoSheet || (*aGeckoSheet)->IsGecko()); MOZ_ASSERT(!aServoSheet || (*aServoSheet)->IsServo()); // Make sure the URIs match: MOZ_ASSERT(!aGeckoSheet || !aServoSheet || (*aGeckoSheet)->GetSheetURI() == (*aServoSheet)->GetSheetURI(), "Sheets passed should have the same URI"); These things don't matter too much for the operation of the function (it won't crash or anything if the assertions fail), but they're important to ensuring that it's being called correctly. ::: layout/style/nsLayoutStylesheetCache.h @@ +30,5 @@ > NS_DECL_ISUPPORTS > NS_DECL_NSIOBSERVER > NS_DECL_NSIMEMORYREPORTER > > + static nsLayoutStylesheetCache& For(mozilla::StyleImplementation aImpl); Three notes here: (1) (optional) Consider calling this "ForBackend" (or "ForStyleBackend"), instead of the plain name "For". Reason: at first glance, an invocation called "For" seems like it might be related to the keyword "for" (as in "for loops"), particularly because we do often add our own polyfills for C++ features with capitalized mozilla-namespaced versions, e.g. unique_ptr/UniquePtr, move/Move. So a function called "For" looks like it could be some sort of polyfilled next-gen C++17 extension of the "for" keyword. (as some sort of magic foreach/iterator/etc. thing perhaps) (2) (optional) Consider changing this to return a pointer instead of a reference, because the type (nsLayoutStylesheetCache) is refcounted. We're pretty consistent about dealing with refcounted objects *exclusively* using raw-pointers & refcounted-smart-pointers -- never C++ references. See bug 1230056 comment 6 for some thoughts on why this consistency is important/useful. (On the other hand, it doesn't matter a lot here, since these objects are kept alive via long-lasting StaticRefPtr instances, and callers generally don't need to care that they're refcounted. So, while I'd be comfortable with returning a pointer here, I'm OK with leaving this as a C++ reference too.) (3) Please add some documentation for this function, and/or for this class, explaining that we have two caches and hinting at how they're allowed to be used. In particular, the first questions that pop into my head when I see this class are: - are the two backends allowed to be used simultaneously? (I think so?) - Is a given sheet expected to only be stored in a single backend's cache, or could it be stored in both caches? @@ +86,2 @@ > static mozilla::css::Loader* gCSSLoader; > + mozilla::StyleImplementation mImplementation; s/Implementation/BackendType/ in the typename & variable-name here (I think that's the rename you're going with for this enum). ::: layout/style/nsStyleSet.cpp @@ +748,5 @@ > if (authorSheets.IndexOf(sheet) != authorSheets.NoIndex) { > break; > } > } > + if (sheet == aDocument->FirstAdditionalAuthorSheet()->AsGecko()) { Unjustified AsGecko() dependency. @@ +767,5 @@ > +void > +nsStyleSet::AppendAllXBLStyleSheets(nsTArray<mozilla::CSSStyleSheet*>& aArray) const > +{ > + nsTArray<StyleSheetHandle> sheets; > + if (mBindingManager) { Move the |sheets| decl inside of the mBindingManager check, since that's the only place it's used. Also, |sheets| probably wants to be an AutoTArray<StyleSheetHandle, 32> instead of a nsTArray, for consistency with the next chunk of this patch (in EnsureUniqueInnerOnCSSSheets) @@ +770,5 @@ > + nsTArray<StyleSheetHandle> sheets; > + if (mBindingManager) { > + mBindingManager->AppendAllSheets(sheets); > + for (StyleSheetHandle handle : sheets) { > + aArray.AppendElement(handle->AsGecko()); Unjustified AsGecko() dependency. @@ +2494,5 @@ > } > > if (mBindingManager) { > + nsAutoTArray<StyleSheetHandle, 32> sheets; > + mBindingManager->AppendAllSheets(sheets); nsAutoTArray was very-recently renamed to AutoTArray, so you'll need to drop the "ns" here. (you may have already discovered) @@ +2496,5 @@ > if (mBindingManager) { > + nsAutoTArray<StyleSheetHandle, 32> sheets; > + mBindingManager->AppendAllSheets(sheets); > + for (StyleSheetHandle sheet : sheets) { > + queue.AppendElement(sheet->AsGecko()); Unjustified AsGecko() dependency.
Attachment #8713940 - Flags: review?(dholbert) → review-
Depends on: 1244068
(In reply to Daniel Holbert [:dholbert] from comment #12) > @@ +92,5 @@ > > + mHandle->Release(); > > + } > > + mHandle = aPtr; > > + if (mHandle) { > > + mHandle->AddRef(); > > Waldo pointed out when I was discussing with him -- you should be doing the > AddRef on the new value *before* you do the release on the old value. > > Otherwise, self-assignment will likely trigger a use-after-free. (we'll > release & delete, and then assign, and then addref the already-deleted > value). > > To the extent possible, you should probably be mimicking code-patterns from > RefPtr here (including the order in which things are performed), to avoid > pitfalls like this. Indeed, I should have noticed that!
Blocks: 1247150
(In reply to Daniel Holbert [:dholbert] from comment #14) > @@ +9849,5 @@ > > } > > > > nsresult > > nsDocument::LoadChromeSheetSync(nsIURI* uri, bool isAgentSheet, > > + mozilla::StyleSheetHandle::RefPtr& aSheet) > > tl;dr: > This should probably be mozilla::StyleSheetHandle::RefPtr*, not RefPtr&. ... > Bonus points if there's a way to make getter_AddRefs still Just Work and > produce a StyleSheetHandle::RefPtr* here (though maybe that'd be overkill). That feels like too much work compared to the number of places we'd use it. I'll just stick with the RefPtr* outparam. > (Side note: nsLayoutStylesheetCache has some outparams that were _already_ > using RefPtr<>& as an outparam, which you're converting to > StyleSheetHandle::RefPtr& in this patch. I'm less concerned with whether > those should use &/* in this patch, since those are straight > string-replaces. But we might eventually want to bring those into alignment > with whatever you decide for the other conversions here.) Filed bug 1247150. > @@ +10188,2 @@ > > RefPtr<CSSStyleSheet> clonedSheet = > > + sheet->AsGecko()->Clone(nullptr, nullptr, clonedDoc, nullptr); > > Unjustified AsGecko() dependency. > > @@ +10203,2 @@ > > RefPtr<CSSStyleSheet> clonedSheet = > > + sheet->AsGecko()->Clone(nullptr, nullptr, clonedDoc, nullptr); > > Unjustified AsGecko() dependency. Again these should only affect printing. I'll add an assertion with a message. > ::: layout/style/CSSStyleSheet.h > @@ +210,5 @@ > > nsIDocument* GetDocument() const { return mDocument; } > > > > void SetTitle(const nsAString& aTitle) { mTitle = aTitle; } > > void SetMedia(nsMediaList* aMedia); > > + nsINode* GetOwningNode() const { return mOwningNode; } > > It looks like you're aiming to rename an old function, "GetOwnerNode", to > use this new name, "GetOwningNode". > > But you left behind the old function (GetOwnerNode). Please drop that old > version, if you're adding this new version, so we're not left with two > subtly-differently-named functions that do the same thing. > > It would also probably be better to split these function-rename changes > (here & the callsites for GetOwnerNode/GetOwningNode) into a separate patch, > because this patch is already huge (multiple hundreds of KB), and this > function-rename seems orthogonal to the other changes here. Indeed, sorry I didn't split that out. I'll do that.
Blocks: 1247159
(In reply to Daniel Holbert [:dholbert] from comment #15) > ::: dom/base/nsStyleLinkElement.cpp > @@ +490,5 @@ > > nsIDocument* document = thisContent->GetOwnerDocument(); > > > > if (thisContent->IsInShadowTree()) { > > ShadowRoot* containingShadow = thisContent->GetContainingShadow(); > > + containingShadow->RemoveSheet(sheet); > > Can this line just stay unchanged? (The old code passes mStyleSheet to > RemoveSheet here; you're changing it to pass "sheet" instead.) OK, I can leave these as mStyleSheet. (Using sheet does skip a runtime check but it's not hot code here.) > ::: dom/base/nsStyleLinkElement.h > @@ +40,5 @@ > > > > + mozilla::CSSStyleSheet* GetSheet() const > > + { > > + return mStyleSheet && mStyleSheet->IsGecko() ? mStyleSheet->AsGecko() : > > + nullptr; > > This probably needs a brief comment explaining why this method is > Gecko-specific. (or an XXX comment saying we should broaden it to include > servo, if this is just a stopgap) I'll add an XXX comment saying that we return null until ServoStyleSheets can be exposed to the DOM. > ::: dom/ipc/ContentChild.cpp > @@ +2562,5 @@ > > static void > > PreloadSlowThings() > > { > > // This fetches and creates all the built-in stylesheets. > > + nsLayoutStylesheetCache::For(StyleImplementation::Gecko).UserContentSheet(); > > Why is this gecko-specific? (Explicitly using ::Gecko) > > The comment should probably hint at this -- or, if we're planning to broaden > this somehow, there should be an XXX comment mentioning this. Otherwise it's > just mysterious. Here I'm just preserving the existing behaviour. I didn't want to force preloading all the UA sheets as Servo-flavoured ones until we're at the point where we're sure we going to use them. I'll add a comment. > ::: dom/xbl/nsXBLPrototypeResources.cpp > @@ +145,5 @@ > > nsXBLPrototypeResources::GatherRuleProcessor() > > { > > + nsTArray<RefPtr<CSSStyleSheet>> sheets(mStyleSheetList.Length()); > > + for (StyleSheetHandle sheet : mStyleSheetList) { > > + sheets.AppendElement(sheet->AsGecko()); > > Unjustified AsGecko() usage. (It's not clear to me where mStyleSheets comes > from or how we'd know they're all gecko-flavored stylesheets) This is because GatherRuleProcessor will only ever be called from nsStyleSet and not ServoStyleSet (since rule processors are a Gecko-specific thing). But I haven't yet worked out how to deal with XBL in the Servo-backed style system. I'll add a check in here. > @@ +147,5 @@ > > + nsTArray<RefPtr<CSSStyleSheet>> sheets(mStyleSheetList.Length()); > > + for (StyleSheetHandle sheet : mStyleSheetList) { > > + sheets.AppendElement(sheet->AsGecko()); > > + } > > + mRuleProcessor = new nsCSSRuleProcessor(sheets, > > So after this patch, |sheets| will get its contents copied here (in the > nsCSSRuleProcessor constructor) -- and then |sheets| dies. This means every > entry in the array gets AddRef'ed (in the copy operation) and then Release'd > (when |sheets| dies), which is a lot of churn. > > If this code is going to stay like this (with a local nsTArray that gets > destructed at the end of this function), it'd probably be worth making > nsCSSRuleProcessor use C++ Move to steal the contents of the passed-in > array, instead of copying it. (Not sure if that's appropriate for everywhere > that instantiates a nsCSSRuleProcessor, but it'd be very much appropriate > for this callsite at least, as long as we're working with this temporary > nsTArray.) I'll give that a short. (I wasn't that happy with the refcount churn there either.) > ::: layout/base/nsPresShell.cpp > @@ +1452,5 @@ > > > > // Document specific "additional" Author sheets should be stronger than the ones > > // added with the StyleSheetService. > > + CSSStyleSheet* firstAuthorSheet = > > + mDocument->FirstAdditionalAuthorSheet()->AsGecko(); > > Unjustified AsGecko() dependency. We should return early at the top of the function if aSheet is a ServoStyleSheet. Hopefully the comment there is sufficient? > @@ +4428,5 @@ > > > > if (mStylesHaveChanged) > > return; > > > > + if (aStyleSheet->IsGecko()) { > > Before this patch, we wrap the arg (aStyleSheet) in a local RefPtr variable > here. (I don't immediately see why we do that, but we do.) Looks like > you're removing that here. > > Please make sure this is OK -- i.e. that we're not depending on this local > RefPtr to keep us alive for the duration of some operation here or something. > > (The RefPtr was added by you back in 2013, here: > http://hg.mozilla.org/mozilla-central/rev/a526743bbd75#l2.104 -- so I'll > trust your judgement on whether it's needed. Maybe it never was?) I suspect it's just to make the do_QueryObject work. I think we'd have existing issues with the various style sheet related document observer notifications and the methods that dispatch them (such as nsDocument::SetStyleSheetApplicableState, InsertStyleSheetAt, etc.) if we weren't strongly holding on to the sheet. > ::: layout/base/nsStyleSheetService.cpp > @@ +156,5 @@ > > // mSheets[aSheetType] > > + > > + // XXXheycam Once the nsStyleSheetService can hold ServoStyleSheets too, > > + // we'll need to include them in the notification. > > + CSSStyleSheet* sheet = mSheets[aSheetType].LastElement()->AsGecko(); > > Unjustified AsGecko() dependency. I'll skip the NotifyObservers call if IsServo(), and assert-unreached if we find it is. (Currently only ever store CSSStyleSheets in the nsStyleSheetService.) > @@ +268,5 @@ > > + > > + StyleSheetHandle handle; > > + sheet.forget(&handle); > > + > > + *aSheet = handle->AsGecko(); > > Unjustified AsGecko() dependency. I'll assert explicitly that Loader gives us a CSSStyleSheet. (This function will need changing once Loader does support handing out ServoStyleSheets, anyway.) > @@ +303,5 @@ > > nsCOMPtr<nsIObserverService> serv = services::GetObserverService(); > > if (serv) { > > + // XXXheycam Once the nsStyleSheetService can hold ServoStyleSheets too, > > + // we'll need to include them in the notification. > > + CSSStyleSheet* cssSheet = sheet->AsGecko(); > > Unjustified AsGecko() dependency. As above. > ::: layout/inspector/inDOMUtils.cpp > @@ +109,5 @@ > > // Get the document sheets. > > for (int32_t i = 0; i < document->GetNumberOfStyleSheets(); i++) { > > + // XXXheycam ServoStyleSets don't have the ability to expose their > > + // sheets in a script-accessible way yet. > > + sheets.AppendElement(document->GetStyleSheetAt(i)->AsGecko()); > > Unjustified AsGecko() dependency. I'll skip and assert-unreached instead. > ::: layout/style/CSSStyleSheet.cpp > @@ +2205,4 @@ > > bool aWasAlternate, > > nsresult aStatus) > > { > > + CSSStyleSheet* sheet = aSheet->AsGecko(); > > Unjustified AsGecko() dependency. > > (This one seems like it's pretty legit, because we're inside of > CSSStyleSheet itself, and I'd guess (?) that CSSStyleSheet::StyleSheetLoaded > may only be called for CSSStyleSheet instances -- not for ServoStyleSheet > instances [though, I'm not sure about that]. Still: it'd be worth an > IsGecko() assertion with a brief explanation.) That's right. > ::: layout/style/Loader.cpp > @@ +1127,5 @@ > > nsXULPrototypeCache* cache = nsXULPrototypeCache::GetInstance(); > > if (cache) { > > if (cache->IsEnabled()) { > > sheet = cache->GetStyleSheet(aURI); > > + LOG((" From XUL cache: %p", static_cast<void*>(sheet))); > > Hmm, this must be why you've got the void* operator -- for all the LOG() > statements in this file. Oh, that's right. :( > (optional): I'd prefer that we had a real method, called something like > StyleSheetHandle::AsVoidPtr() (name like "AsGecko()" / "AsServo()"). And I > think that function should clear the SERVO_BIT before it returns (which the > current void*-conversion operator does not do IIRC), unless you plan on > watching for & caring about the SERVO_BIT in the logged output. And it can > be documented as being *only* for convenient logging, and not to be used > otherwise. What do you think? Sounds good! > @@ +1149,5 @@ > > > > if (sheet) { > > // This sheet came from the XUL cache or our per-document hashtable; it > > // better be a complete sheet. > > + NS_ASSERTION(sheet->AsGecko()->IsComplete(), > > Unjustified AsGecko() dependency, here & throughout the rest of this > function (lots of AsGecko() calls). > > (I think the justification is your "XXXheycam Cached sheets currently must > be CSSStyleSheets" comment a ways up from this; but if you're depending on > that here, we probably should be asserting it as well.) OK, I'll assert inside the if statement here, repeating that justification. > @@ +1263,3 @@ > > } > > > > + NS_ASSERTION(aSheet, "We should have a sheet by now!"); > > This line's change (dropping a "*") needs to be reverted -- you really want > to be asserting about "*aSheet", not "aSheet". Yes, thanks for spotting this! > @@ +1416,5 @@ > > > > + // XXXheycam The InsertChildSheet API doesn't work with ServoStyleSheets, > > + // since they won't have Gecko ImportRules in them. > > + if (aSheet->IsServo()) { > > + return NS_OK; > > (Is NS_OK the right thing to return here? Maybe a failure code would be > better?) Actually I think I'll just change this to assert. We can't get in here with a ServoStyleSheet since that ultimately comes from nsCSSParser parsing an @import rule. > @@ +1774,5 @@ > > > > aCompleted = false; > > > > + // XXXheycam ServoStyleSheets don't support parsing their contents yet. > > + nsCSSParser parser(this, aLoadData->mSheet->AsGecko()); > > Unjustified AsGecko() dependency. (need an assertion w/ hint at why we know > servo can't get here, or a fallback codepath for servo -- whichever is > appropriate) This is because we only ever create a new CSSStyleSheet currently, up in CreateSheet. I'll assert with a message saying that. > @@ +1938,5 @@ > > // one of the sheets that will be kept alive by a document or > > // parent sheet anyway, so that if someone then accesses it via > > // CSSOM we won't have extra clones of the inner lying around. > > data = aLoadData; > > + CSSStyleSheet* sheet = aLoadData->mSheet->AsGecko(); > > Unjustified AsGecko() dependency. (here as well as a few lines below, > "data->mSheet->AsGecko()") This one looks like a bug. We shouldn't be trying to cache ServoStyleSheets yet. > @@ +2205,5 @@ > > + if (!parent) { > > + break; > > + } > > + topSheet = parent; > > + } > > If you like, I believe the 7-line for(;;;) loop here could be condensed to > these 3 lines: > > while (StyleSheetHandle parent = topSheet->GetParentSheet()) { > topSheet = parent; > } Looks good. (I don't think I realised you could declare variables in a while loop, despite knowing you could in an if.) > @@ +2242,5 @@ > > LOG((" No parent load; must be CSSOM")); > > // No parent load data, so the sheet will need to be notified when > > // we finish, if it can be, if we do the load asynchronously. > > + // XXXheycam ServoStyleSheet doesn't implement nsICSSLoaderObserver yet. > > + observer = aParentSheet->AsGecko(); > > Unjustified AsGecko() usage. > > (I'm not clear on whether the XXX comment is intending > (a) to declare that we won't get here with a servo stylesheet > ...or: > (b) to say we can't do anything useful if we *do* get here with a servo > stylesheet. > If the former, this probably wants an assert with a version of that XXX > comment as its message. If the latter, it probably wants some stub fallback > code for the IsServo case.) We shouldn't be able to get in here with ServoStyleSheets, until we work out how to handle @import rules. I'll assert that. > ::: layout/style/StyleSetHandleInlines.h > @@ +145,4 @@ > > { > > + if (IsGecko()) { > > + // How safe would it be to reinterpret_cast aNewSheets to > > + // |const nsTArray<RefPtr<CSSStyleSheet>>|? > > (answering the question in this comment): I wouldn't be surprised if this > worked, but I would be very hesitant to actually take that code. :) Seems > way too fragile/dangerous. > > Maybe drop this comment, unless you're actually seriously considering doing > this. > > Seems like a better solution (to avoid copying) would be to make > CSSStyleSheet::ReplaceSheets accept the more general parameter -- > nsTArray<StyleSheetHandle::RefPtr> -- and assert that each entry in that > array IsGecko before using it. Maybe file a followup on this, to avoid > introducing a perf regression from the temporary-array & copying here? Filed bug 1247159.
(In reply to Daniel Holbert [:dholbert] from comment #22) > ::: layout/style/nsLayoutStylesheetCache.h > @@ +30,5 @@ > > NS_DECL_ISUPPORTS > > NS_DECL_NSIOBSERVER > > NS_DECL_NSIMEMORYREPORTER > > > > + static nsLayoutStylesheetCache& For(mozilla::StyleImplementation aImpl); > > Three notes here: > (1) (optional) Consider calling this "ForBackend" (or "ForStyleBackend"), > instead of the plain name "For". Reason: at first glance, an invocation > called "For" seems like it might be related to the keyword "for" (as in "for > loops"), particularly because we do often add our own polyfills for C++ > features with capitalized mozilla-namespaced versions, e.g. > unique_ptr/UniquePtr, move/Move. So a function called "For" looks like it > could be some sort of polyfilled next-gen C++17 extension of the "for" > keyword. (as some sort of magic foreach/iterator/etc. thing perhaps) Fair comment, but I think For by itself reads well in the places it's called, and those lines are already quite wordy: nsLayoutStylesheetCache::For(StyleBackendType::Gecko)... vs nsLayoutStylesheetCache::ForBackend(StyleBackendType::Gecko)... > ::: layout/style/nsStyleSet.cpp > @@ +748,5 @@ > > if (authorSheets.IndexOf(sheet) != authorSheets.NoIndex) { > > break; > > } > > } > > + if (sheet == aDocument->FirstAdditionalAuthorSheet()->AsGecko()) { > > Unjustified AsGecko() dependency. I'll add a justification that we're in nsStyleSet (and so deal purely with CSSStyleSheets). > @@ +770,5 @@ > > + nsTArray<StyleSheetHandle> sheets; > > + if (mBindingManager) { > > + mBindingManager->AppendAllSheets(sheets); > > + for (StyleSheetHandle handle : sheets) { > > + aArray.AppendElement(handle->AsGecko()); > > Unjustified AsGecko() dependency. As above. > @@ +2494,5 @@ > > } > > > > if (mBindingManager) { > > + nsAutoTArray<StyleSheetHandle, 32> sheets; > > + mBindingManager->AppendAllSheets(sheets); > > nsAutoTArray was very-recently renamed to AutoTArray, so you'll need to drop > the "ns" here. (you may have already discovered) I hadn't. :-) > @@ +2496,5 @@ > > if (mBindingManager) { > > + nsAutoTArray<StyleSheetHandle, 32> sheets; > > + mBindingManager->AppendAllSheets(sheets); > > + for (StyleSheetHandle sheet : sheets) { > > + queue.AppendElement(sheet->AsGecko()); > > Unjustified AsGecko() dependency. As above. Thanks again for the detailed review, Daniel. All the rest of the comments I agree with.
Sounds good! Thanks for the responses.
(In reply to Daniel Holbert [:dholbert] from comment #14) > ::: layout/style/CSSStyleSheet.h > @@ +210,5 @@ > > nsIDocument* GetDocument() const { return mDocument; } > > > > void SetTitle(const nsAString& aTitle) { mTitle = aTitle; } > > void SetMedia(nsMediaList* aMedia); > > + nsINode* GetOwningNode() const { return mOwningNode; } > > It looks like you're aiming to rename an old function, "GetOwnerNode", to > use this new name, "GetOwningNode". > > But you left behind the old function (GetOwnerNode). Please drop that old > version, if you're adding this new version, so we're not left with two > subtly-differently-named functions that do the same thing. Looking at this again, I remember: GetOwnerNode needs to stay, as it implements the ownerNode IDL attribute on StyleSheet. I'll just change all of my GetOwningNode to GetOwnerNode and remove my added GetOwningNode method.
Blocks: 1247182
(In reply to Daniel Holbert [:dholbert] from comment #15) > @@ +147,5 @@ > > + nsTArray<RefPtr<CSSStyleSheet>> sheets(mStyleSheetList.Length()); > > + for (StyleSheetHandle sheet : mStyleSheetList) { > > + sheets.AppendElement(sheet->AsGecko()); > > + } > > + mRuleProcessor = new nsCSSRuleProcessor(sheets, > > So after this patch, |sheets| will get its contents copied here (in the > nsCSSRuleProcessor constructor) -- and then |sheets| dies. This means every > entry in the array gets AddRef'ed (in the copy operation) and then Release'd > (when |sheets| dies), which is a lot of churn. > > If this code is going to stay like this (with a local nsTArray that gets > destructed at the end of this function), it'd probably be worth making > nsCSSRuleProcessor use C++ Move to steal the contents of the passed-in > array, instead of copying it. (Not sure if that's appropriate for everywhere > that instantiates a nsCSSRuleProcessor, but it'd be very much appropriate > for this callsite at least, as long as we're working with this temporary > nsTArray.) Filed followup bug 1247182 for that.
FWIW, I am going to follow this convention for unexpected Servo things, so they can be grepped for easily later: * NS_ASSERTION(false, "stylo: ...") for things we don't implement yet for the Servo backed, when it is easy to just ignore the problem (and thus don't need a fatal assert) * MOZ_CRASH("stylo: ...") for things we don't handle yet and which can't be easily ignored
Jeff, can you take a look at my RefPtr-like class (as dholbert requests in comment 12) to check I'm not doing anything crazy?
Attachment #8713575 - Attachment is obsolete: true
Attachment #8717831 - Flags: review?(jwalden+bmo)
Attachment #8713940 - Attachment is obsolete: true
Attachment #8717832 - Flags: review?(dholbert)
(In reply to Cameron McCormack (:heycam) from comment #30) > * NS_ASSERTION(false, "stylo: ...") for things we don't implement yet for > the Servo backed, when it is easy to just ignore the problem (and thus don't > need a fatal assert) Please don't use NS_ASSERTION(false) -- better (shorter & more declarative) would be NS_ERROR("...") or NS_NOTYETIMPLEMENTED("..."). Those end up doing the exact same thing that NS_ASSERTION(false) would, under the hood, without the need to "assert false". (If you've already sprinkled this throughout your patches, though, feel free to postpone the NS_ASSERTION(false)-removal in a followup patch, if you prefer.) > * MOZ_CRASH("stylo: ...") for things we don't handle yet and which can't be easily ignored
Comment on attachment 8717832 [details] [diff] [review] Part 4: Use StyleSetHandle instead of concrete style set class in most places. Looks like you attached the wrong file for re-review here. This new 'part 4' is for styleset, i.e. bug 1244068, as opposed to stylsheet/this bug here.)
Flags: needinfo?(cam)
Attachment #8717832 - Flags: review?(dholbert)
Attachment #8717832 - Attachment is obsolete: true
Flags: needinfo?(cam)
Attachment #8718118 - Flags: review?(dholbert)
Comment on attachment 8718118 [details] [diff] [review] Part 4: Use StyleSheetHandle instead of concrete style sheet class in most places. (v2) Review of attachment 8718118 [details] [diff] [review]: ----------------------------------------------------------------- r=me, with nits below addressed as you see fit, and with the NS_ASSERTION(false, ...) replaced per comment 33 (in a followup patch if you like). ::: editor/libeditor/nsHTMLEditor.cpp @@ +2939,5 @@ > sheet->SetOwningDocument(doc); > > + if (sheet->IsServo()) { > + // XXXheycam ServoStyleSheets don't support being enabled/disabled yet. > + NS_ASSERTION(false, "ServoStyleSheets can't be disabled yet"); (The assertion-message here is missing the "stylo:" prefix -- you might want to add that for greppability.) @@ +2959,5 @@ > sheet->SetOwningDocument(doc); > > + if (sheet->IsServo()) { > + // XXXheycam ServoStyleSheets don't support being enabled/disabled yet. > + NS_ASSERTION(false, "ServoStyleSheets can't be disabled yet"); (As above, this might want to have "stylo:" in the message.) ::: layout/base/nsPresShell.cpp @@ +1450,3 @@ > RefPtr<CSSStyleSheet> sheet = do_QueryObject(aSheet); > if (!sheet) { > + NS_ASSERTION(false, "stylo: AddAgentSheet needs to support " copypaste typo -- s/AddAgentSheet/AddAuthorSheet/ here. @@ +1470,5 @@ > PresShell::RemoveSheet(SheetType aType, nsISupports* aSheet) > { > RefPtr<CSSStyleSheet> sheet = do_QueryObject(aSheet); > if (!sheet) { > + NS_ASSERTION(false, "stylo: AddAgentSheet needs to support " copypaste typo -- s/AddAgentSheet/RemoveSheet/ here. ::: layout/inspector/inDOMUtils.cpp @@ +81,5 @@ > > // Get the agent, then user and finally xbl sheets in the style set. > nsIPresShell* presShell = document->GetShell(); > > + if (presShell && presShell->StyleSet()->IsServo()) { I think this is in the wrong patch... This chunk seems to be replacing this call... presShell->IsServo() ...with: presShell->StyleSet()->IsServo() Looks like this is a style-set related change, not a stylesheet related change -- so this probably belongs in one of bug 1244068's patches, instead of this patch. (I'm not actually sure where the "presShell->IsServo()" call that you're removing here was introduced -- and I'm not sure presShell *has* its own "IsServo" API. Maybe this line was added in one of your earlier patches, and it doesn't compile, and you accidentally qref'ed this bustage-fix into the wrong patch?) ::: layout/style/Loader.cpp @@ +2207,5 @@ > // we finish, if it can be, if we do the load asynchronously. > + // XXXheycam ServoStyleSheet doesn't implement nsICSSLoaderObserver yet. > + MOZ_ASSERT_UNREACHABLE("stylo: ServoStyleSheets don't support child sheet " > + "loading yet"); > + observer = aParentSheet->AsGecko(); I think your MOZ_ASSERT_UNREACHABLE here is a mistake -- I think it really wants to be MOZ_ASSERT(aParentSheet->IsGecko(), ...) -- right? (Right now, the UNREACHABLE assertion implies we've got a servo-flavored sheet; but the code right after says we've got a gecko-flavored sheet.) ::: layout/style/nsStyleSet.cpp @@ +771,5 @@ > +void > +nsStyleSet::AppendAllXBLStyleSheets(nsTArray<mozilla::CSSStyleSheet*>& aArray) const > +{ > + nsTArray<StyleSheetHandle> sheets; > + if (mBindingManager) { I think you missed these review notes from comment 22: (1) Move the |sheets| decl inside of the mBindingManager check, since that's the only place it's used. (2) Also, |sheets| probably wants to be an AutoTArray<StyleSheetHandle, 32> instead of a nsTArray, for consistency with the next chunk of this patch (in EnsureUniqueInnerOnCSSSheets) @@ +778,5 @@ > + // CSSStyleSheets). > + mBindingManager->AppendAllSheets(sheets); > + for (StyleSheetHandle handle : sheets) { > + MOZ_ASSERT(handle->IsGecko(), "only callers interested in CSSStyleSheets " > + "should be calling us!"); I don't fully understand this assertion-message. Seems like we're asserting about the contents of |sheets| here, and those contents are determined by our call to "mBindingManager.AppendAllSheets()" -- they're not determined by our callers. SO: I don't see why the message here is talking about our callers. Maybe this wants to use the same message that we use in a similar loop in the next chunk of this patch: MOZ_ASSERT(sheet->IsGecko(), "stylo: AppendAllSheets shouldn't give us " "ServoStyleSheets yet"); (This assertion message makes more sense to me, at least.) Or, maybe your message makes sense and I'm just not understanding it. :)
Attachment #8718118 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #33) > Please don't use NS_ASSERTION(false) -- better (shorter & more declarative) > would be NS_ERROR("...") or NS_NOTYETIMPLEMENTED("..."). Those end up doing > the exact same thing that NS_ASSERTION(false) would, under the hood, without > the need to "assert false". I've changed to NS_ERROR locally.
Comment on attachment 8717831 [details] [diff] [review] Part 2: Add HandleRefPtr for refcounting StyleSheetHandles. (v2) r=dholbert Review of attachment 8717831 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/HandleRefPtr.h @@ +20,5 @@ > + * A class for holding strong references to handle-managed objects. > + * > + * This is intended for use with objects like StyleSheetHandle, where > + * the handle type is not a pointer but which can still have ->AddRef() > + * and ->Release() called on it. Also mention that T must be nullable using |nullptr|, since you require that here. @@ +36,5 @@ > + > + HandleRefPtr(HandleRefPtr<T>&& aRhs) > + { > + mHandle = aRhs.mHandle; > + aRhs.mHandle = nullptr; Just swap the two handles and let |aRhs|'s destruction handle the nulling, rather than inlining the effect of |assign(nullptr)| here. @@ +62,5 @@ > + explicit operator bool() const { return !!mHandle; } > + bool operator!() const { return !mHandle; } > + > + operator T() const { return mHandle; } > + T operator->() const { return mHandle; } If we're starting from scratch, don't add (implicit!!!) casting overloading -- if implicit conversions didn't exist in nsCOMPtr now we'd be in a far better place. Instead make this T& get() const { return mHandle; } T& operator->() const { return get(); } @@ +67,5 @@ > + > + bool operator==(const HandleRefPtr<T>& aOther) const > + { > + return mHandle == aOther.mHandle; > + } Might as well add bool operator!=(const HandleRefPtr<T>& aOther) const { return !(*this == aOther); } @@ +83,5 @@ > + } > + if (mHandle) { > + mHandle->Release(); > + } > + mHandle = aPtr; This is nitpicky and possibly won't ever be relevant, but I'm not sure about releasing the handle before overwriting it with the new pointer. Suppose the owner of |this| was owned by mHandle, then we could be freeing |this| underneath us. How about we have the method in this order, with comments like these: // AddRef early so |aPtr| can't disappear underneath us. if (aPtr) aPtr->AddRef(); // Don't release |mHandle| yet: if |mHandle| indirectly owns |this|, // releasing would invalidate |this|. Swap |aPtr| for |mHandle| so that // |aPtr| lives as long as |this|, then release the new |aPtr| (really the // original |mHandle|) so that if |this| is invalidated, we're not using it // any more. std::swap(mHandle, aPtr); aPtr->Release();
Attachment #8717831 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #38) > If we're starting from scratch, don't add (implicit!!!) casting overloading > -- if implicit conversions didn't exist in nsCOMPtr now we'd be in a far > better place. Instead make this > > T& get() const { return mHandle; } > T& operator->() const { return get(); } This is making it a bit awkward to replace most existing RefPtr<CSSStyleSheet>s with HandleRefPtr<StyleSheetHandle>s (as I do in a the part 4 patch), as * there are ~80 call sites where I need to add a .get() (mostly on lines I didn't have to touch before) * it means I can't just convert the nsBaseHashtable<..., RefPtr<CSSStyleSheet>, CSSStyleSheet*> in Loader.h to nsBaseHashtable<..., HandleRefPtr<StyleSheetHandle>, StyleSheetHandle> * I can't call nsTArray<StyleSheetHandle>::AppendElements(nsTArray<HandleRefPtr<StyleSheetHandle>>&) and instead have to expand that out into a loop So I'm wondering if it's really worth removing the implicit conversion?
Flags: needinfo?(jwalden+bmo)
(In reply to Cameron McCormack (:heycam) (away Feb 27 – Mar 13) from comment #39) > (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #38) > > If we're starting from scratch To emphasize here, we're totally not starting from scratch. The goal here is to build a shim with the properties we need to get the Servo backend in-tree and streamline the engineering for our prototype. I really don't want to add overhead here.
Also, we want to make this easy to back out, which means minimizing code churn.
(In reply to Bobby Holley (busy) from comment #40) > To emphasize here, we're totally not starting from scratch. Oh, you aren't? Okay, what you have is fine, then. But let's start a ball rolling on improving this in followups. (In reply to Cameron McCormack (:heycam) (away Feb 27 – Mar 13) from comment #39) > * there are ~80 call sites where I need to add a .get() (mostly on lines I > didn't have to touch before) Please file the followup for this. > * it means I can't just convert the nsBaseHashtable<..., > RefPtr<CSSStyleSheet>, CSSStyleSheet*> in Loader.h to nsBaseHashtable<..., > HandleRefPtr<StyleSheetHandle>, StyleSheetHandle> And (separate followup -- this one should be fast and easy, possibly unlike fixing 80 callsites above) I think you could if you defined MOZ_IMPLICIT StyleSheetHandle(const HandleRefPtr<StyleSheetHandle>&); maybe with/without consts or something. As nsBaseHashtable.h says, "If UserDataType is not the same, DataType must implicitly cast to UserDataType". Not that hard to make that work, it would seem. > * I can't call > nsTArray<StyleSheetHandle>:: > AppendElements(nsTArray<HandleRefPtr<StyleSheetHandle>>&) and instead have > to expand that out into a loop At a skim, I think the implicit conversion would solve this.
Flags: needinfo?(jwalden+bmo)
Blocks: 1250711
Filed bug 1250711 for removing the implicit conversion operator.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: