I think post-bug 1221436 this becomes easier, and we don't need to do bug 898764. In particular, I think instead of bug 898764, we can now use css::Rule as the class that the WebIDL uses for CSS rules rather than making a new one. It already has virtual functions, so there's no change in size. Then what bug 1221436 allows is that we can merge css::DOMCSSStyleRule into css::StyleRule. It currently has a vtable pointer and a nested DOMCSSDeclarationImpl object. We can separate the DOMCSSDeclarationImpl object from it (but perhaps still make it owned, and with reference counts aggregated). This keeps the size of DOMCSSDeclarationImpl the same (it already has an mRule pointing to the css::StyleRule) and increases the size of css::StyleRule only by one word (the vtable pointer for nsICSSStyleRuleDOMWrapper (which is mostly nsIDOMCSSStyleRule), which I think new bindings will allow us to eliminate in a later step; there's no increase for the pointer to the lazily-allocated DOMCSSDeclarationImpl since we reuse the space that's currently occupied by the lazily-allocated mDOMRule (and probably put it inside the DOMCSSStyleRule subobject as long as it's a subobject... which might not need to be the case once we remove support for the old bindings). Do you agree that new bindings will let us get rid of the nsIDOMCSSStyleRule/nsICSSStyleRuleDOMWrapper vtable pointer? (I'm hoping it will let us shrink DOMCSSDeclarationImpl even more... but that's far less important since they'd still be lazily created on DOM access.) And does this plan otherwise make sense?

We could probably use css::Rule now, yes. Unfortunately, that _will_ involve changing its size, since we'll need to make it inherit from nsWrapperCache, which is two words (or at least a word followed by a uint32_t). > Do you agree that new bindings will let us get rid of the nsIDOMCSSStyleRule/nsICSSStyleRuleDOMWrapper vtable pointer? Yes. nsICSSStyleRuleDOMWrapper will become completely unnecessary. nsIDOMCSSStyleRule will take a bit more work, because we have browser and addon code doing QI to that interface (for totally unnecessary reasons, but such is the power of cargo-culting). I assume that we will want to handle it as follows: 1) Make css::Rule inherit from nsIDOMCSSRule and adjust its QI impl accordingly, so QI of any rule to nsIDOMCSSRule works. 2) Make nsIDOMCSS*Rule _not_ inherit from nsIDOMCSSRule, so we don't end up with multiple inheritance from nsIDOMCSSRule. This might need to happen in the same changeset as #1. 3) Make nsIDOMCSSStyleRule an empty interface, change any consumers to use StyleRule instead as needed. 4) Have css::StyleRule QI to it without inheriting from it (returning itself as nsISupports* instead); given that it's empty this is not unreasonable because the vtable is just that of nsISupports. If we're not ok with #3 because of the creepy-crawley feeling it induces (and it does, I know), we could instead do: 4) Change mozilla::dom::QueryInterface to special-case nsIDOMCSSStyleRule and when its IID is passed just QI the object to mozilla::css::StyleRule instead. Have nothing to do with nsIDOMCSSStyleRule in StyleRule at all.

OK, before I start posting patches, a summary of the new setup: * Steps 1 and 2 from comment 2 are done. nsIDOMCSSRule still has a GetCSSRule() method to get the css::StyleRule; this just returns "this". * Step 3 from comment 2 is complicated by ServoStyleRule. I will see about doing something like it in a followup. * I am introducing a BindingStyleRule class that's a common base class of ServoStyleRule and StyleRule. We may want to give it a better name (mozilla::StyleRule seemed like a bad idea!) and make it QI-able or something; at that point it could be the thing we do for step 3. * For now I'm leaving all the nsIDOMCSS* bits around, but we may want to investigate removing them. Need to figure out how that should work with Stylo.

Oh, and I looked at addons dxr, and looks like I need to keep scripted QueryInterface on CSSStyleRule and CSSMozDocumentStyleRule...

Er, I meant CSSMozDocumentRule.

Note that this increases the size of css::Rule by three words, unfortunately.

Peter, could you review he UnwrapArg bits and the dom/bindings and dom/webidl changes, for this changeset and the following ones?

Andrew, can you please review the weakmap test change?

Andrew, can you please review the other weakmap test change?

The PutForwards bit is a new feature, but easy enough to implement here.

Hi and happy new year! Happy to see this happening :-) A quick question: does this migration to webidl change the behavior for devs in anyway, or, in another way, does this need any developer documentation?

There are all sorts of behavior changes, yes. Properties will now appear on the correct prototypes (e.g. the CSSRule ones will only be on CSSRule.prototype and not shadowed on most-derived prototypes as they are now; CSSGroupingRule.prototype will actually have properties hanging off it, etc). The various CSS*Rule interface objects will now look like Function objects. The toString of the various CSS*Rule.prototype objects will be different. We're adding wrapper caching, so we will no longer do weird stuff like lose expandos on CSS rules when GC happens. We'll be able to use CSS rules as weakmap keys (which we can't do right now). Plus the bits described in https://groups.google.com/d/msg/mozilla.dev.platform/baiZcqFrF50/eFAX84n6EwAJ of course. And there may be some other pieces I can't think of off the top of my head. In terms of documenting this, I'm not really sure what the right approach is. Mostly this makes CSS rules act like all other DOM objects.

Comment on attachment 8822845 [details] [diff] [review] part 11. Convert CSSImportRule to WebIDL Review of attachment 8822845 [details] [diff] [review]: ----------------------------------------------------------------- r=me for the test change. Thanks for maintaining these weak map tests over the years as various things were swapped to WebIDL.

Comment on attachment 8822846 [details] [diff] [review] part 12. Convert CSSStyleRule to WebIDL Review of attachment 8822846 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the weakmap test changes

Comment on attachment 8822836 [details] [diff] [review] part 2. Remove the now-unused GetExistingDOMRule method Review of attachment 8822836 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/StyleRule.cpp @@ +1433,5 @@ > NS_IMPL_RELEASE_INHERITED(StyleRule, Rule) > > +NS_IMPL_CYCLE_COLLECTION_CLASS(StyleRule) > +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(StyleRule, Rule) > + tmp->DropReferences(); Was not calling DropReference / SetOwningRule during unlinking a pre-existing problem, or did this only surface for some reason due to the changes in part 1? (This change seems unrelated to the commit message so maybe should be in a separate patch.)

> This change seems unrelated to the commit message so maybe should be in a separate patch. It should be in part 1. Good catch!

Comment on attachment 8823521 [details] [diff] [review] Part 5 merged on top of bug 1326507 Review of attachment 8823521 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/StyleRule.cpp @@ -1140,5 @@ > -inline css::DOMCSSStyleRule* DOMCSSDeclarationImpl::DomRule() > -{ > - return reinterpret_cast<css::DOMCSSStyleRule*> > - (reinterpret_cast<char*>(this) - > - offsetof(css::DOMCSSStyleRule, mDOMDeclaration)); Much prefer having a separate UniquePtr owned object than the fused allocation we had before! ::: layout/style/StyleRule.h @@ +313,3 @@ > > +class StyleRule final : public Rule, > + public nsICSSStyleRuleDOMWrapper Nit: comma on this line.

Comment on attachment 8823518 [details] [diff] [review] Part 4 merged on top of bug 1326507 Review of attachment 8823518 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsWrapperCache.h @@ +286,5 @@ > } > } > > private: > + // Firend declarations for things that need to be able to call Friend ::: layout/style/Rule.h @@ +30,5 @@ > DECL_STYLE_RULE_INHERIT_NO_DOMRULE \ > virtual nsIDOMCSSRule* GetDOMRule() override; > > +class Rule : public nsISupports, > + public nsWrapperCache Nit: comma on this line.

Comment on attachment 8822840 [details] [diff] [review] part 6. Make css::Rule inherit from nsIDOMCSSRule Review of attachment 8822840 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/Rule.h @@ +114,5 @@ > // Note that this returns null for inline style rules since they aren't > // supposed to have a DOM rule representation (and our code wouldn't work). > virtual nsIDOMCSSRule* GetDOMRule() = 0; > > // to implement methods on nsIDOMCSSRule Maybe just change this comment to "// nsIDOMCSSRule" since they are the actual implementations now.

(In reply to Cameron McCormack (:heycam) from comment #38) > Maybe just change this comment to "// nsIDOMCSSRule" since they are the > actual implementations now. That changes in the next patch, so don't worry.

Comment on attachment 8822841 [details] [diff] [review] part 7. Push the nsIDOMCSSRule implementation up to css::Rule Review of attachment 8822841 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/NameSpaceRule.h @@ +51,5 @@ > > void GetURLSpec(nsString& aURLSpec) const { aURLSpec = mURLSpec; } > > + // WebIDL interface > + uint16_t Type() const override; Nit: trailing space. ::: layout/style/nsCSSRules.cpp @@ +109,5 @@ > +NS_IMETHODIMP > +Rule::SetCssText(const nsAString& aCssText) > +{ > + // We used to throw for some rule types, but not all. Specifically, we did > + // not throw for StyleRule. Let's just always not throw. Luckily that's what the spec says to do. :)

> Luckily that's what the spec says to do. :) Ick spec. Ah, well.

Comment on attachment 8822843 [details] [diff] [review] part 9. Add a CSSRule Web IDL interface Review of attachment 8822843 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the bits outside dom/ and not UnwrapArg-related. ::: layout/style/ServoStyleSheet.cpp @@ +132,3 @@ > ServoStyleSheet::GetDOMOwnerRule() const > { > + NS_ERROR("Don't know how to get DOM owner rule for ServoStyleSheet"); Nit: I've been using a prefix of "stylo: " in messages like these, so that later they'll be easy to grep for and fix.

Comment on attachment 8822844 [details] [diff] [review] part 10. Convert CSSNamespaceRule to WebIDL Review of attachment 8822844 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the layout/ bits.

Comment on attachment 8822845 [details] [diff] [review] part 11. Convert CSSImportRule to WebIDL Review of attachment 8822845 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the layout/ bits.

Comment on attachment 8822846 [details] [diff] [review] part 12. Convert CSSStyleRule to WebIDL Review of attachment 8822846 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the layout/ bits. ::: dom/webidl/LegacyQueryInterface.webidl @@ +87,5 @@ > XMLHttpRequest implements LegacyQueryInterface; > XMLHttpRequestUpload implements LegacyQueryInterface; > XMLSerializer implements LegacyQueryInterface; > XPathEvaluator implements LegacyQueryInterface; > +CSSStyleRule implements LegacyQueryInterface; Nit: This file looks pretty well sorted, so we may as well keep it that way.

Comment on attachment 8822847 [details] [diff] [review] part 13. Convert media, supports, and moz-document rules to WebIDL Review of attachment 8822847 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the layout/ bits. ::: layout/style/nsCSSRules.cpp @@ +771,5 @@ > +nsMediaList* > +MediaRule::Media() const > +{ > + // In practice, if we end up being parsed at all, we have non-null mMedia. So > + // it's OK to claim we don't return null here. Is it worth then changing all of the existing null checks in MediaRule's other methods into assertions?

Comment on attachment 8822848 [details] [diff] [review] part 14. Convert CSSPageRule to WebIDL Review of attachment 8822848 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the layout/ bits. ::: dom/bindings/Bindings.conf @@ +233,5 @@ > + 'nativeType': 'nsCSSPageRule', > + 'headerFile': 'nsCSSRules.h', > +}, > + > + Nit: two blank lines probably not needed.

Comment on attachment 8822849 [details] [diff] [review] part 15. Convert CSSFontFaceRule to WebIDL Review of attachment 8822849 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the layout/ bits. ::: dom/webidl/CSSFontFaceRule.webidl @@ +6,5 @@ > + * The origin of this IDL file is > + * https://drafts.csswg.org/css-fonts/#om-fontface > + */ > + > +// https://drafts.csswg.org/cssom/#the-csspagerule-interface Wrong link.

Comment on attachment 8822850 [details] [diff] [review] part 16. Convert CSSFontFeatureValuesRule to WebIDL Review of attachment 8822850 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the layout/ bits. ::: dom/webidl/CSSFontFeatureValuesRule.webidl @@ +11,5 @@ > +// but we don't implement anything remotely resembling the spec. > +interface CSSFontFeatureValuesRule : CSSRule { > + [SetterThrows] > + attribute DOMString fontFamily; > + // Gecko addition? I guess so. Maybe split out into a |partial interface|?

Comment on attachment 8822851 [details] [diff] [review] part 17. Convert CSSKeyframeRule to WebIDL Review of attachment 8822851 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the layout/ bits.

Comment on attachment 8822852 [details] [diff] [review] part 18. Convert CSSKeyframesRule to WebIDL Review of attachment 8822852 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the layout/ bits.

Comment on attachment 8822853 [details] [diff] [review] part 19. Convert CSSCounterStyleRule to WebIDL Review of attachment 8822853 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the layout/ bits.

> Is it worth then changing all of the existing null checks in MediaRule's other methods into assertions? Yeah, probably a good idea. Not in the dtor, because we may have been cycle-collected. Probably best done as a followup; filed bug 1328808 but stylo may make it irrelevant anyway.

Addressed all the other comments. Thank you for the quick reviews!

Comment on attachment 8822843 [details] [diff] [review] part 9. Add a CSSRule Web IDL interface Review of attachment 8822843 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/Rule.h @@ +138,5 @@ > +// Specialization of the bindings UnwrapArg setup for css::Rule, so we can avoid > +// adding an IID to css::Rule. This can go away once all css::Rule subclasses > +// are on WebIDL bindings. > + > +#include "js/TypeDecls.h" Shouldn't this include be outside the mozilla namespace? ::: layout/style/nsCSSRules.cpp @@ +46,5 @@ > namespace mozilla { > + > +// Temporary code that can go away once all css::Rules are on WebIDL bindings. > +namespace dom { > +#include "xpcpublic.h" Shouldn't this include be outside of the mozilla::dom namespace?

> Shouldn't this include be outside the mozilla namespace? > Shouldn't this include be outside of the mozilla::dom namespace? Yes to both. Good catch. Fixed.

Comment on attachment 8822844 [details] [diff] [review] part 10. Convert CSSNamespaceRule to WebIDL Review of attachment 8822844 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/tests/mochitest/general/test_interfaces.html @@ +248,5 @@ > "CSSMediaRule", > // IMPORTANT: Do not change this list without review from a DOM peer! > "CSSMozDocumentRule", > // IMPORTANT: Do not change this list without review from a DOM peer! > + "CSSNamespaceRule", Ugh!

Comment on attachment 8822846 [details] [diff] [review] part 12. Convert CSSStyleRule to WebIDL Review of attachment 8822846 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/ServoStyleRule.cpp @@ +155,5 @@ > /* virtual */ JSObject* > ServoStyleRule::WrapObject(JSContext* aCx, > JS::Handle<JSObject*> aGivenProto) > { > + return dom::CSSStyleRuleBinding::Wrap(aCx, this, aGivenProto); I guess an alternative would be to put the WrapObject in BindingStyleRule, up to you.

> I guess an alternative would be to put the WrapObject in BindingStyleRule Oh, good point. Done.

Comment on attachment 8822847 [details] [diff] [review] part 13. Convert media, supports, and moz-document rules to WebIDL Review of attachment 8822847 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/CSSMediaRule.webidl @@ +10,5 @@ > + > +// https://drafts.csswg.org/cssom/#the-cssmediarule-interface and > +// https://drafts.csswg.org/css-conditional/#the-cssmediarule-interface > +// except they disagree with each other. We're taking the inheritance from > +// css-conditional and the PutForwards behavior from cssom. Spec bug filed? ::: dom/webidl/LegacyQueryInterface.webidl @@ +88,5 @@ > XMLHttpRequestUpload implements LegacyQueryInterface; > XMLSerializer implements LegacyQueryInterface; > XPathEvaluator implements LegacyQueryInterface; > CSSStyleRule implements LegacyQueryInterface; > +CSSMozDocumentRule implements LegacyQueryInterface; Alphabetical order? ::: layout/style/nsCSSRules.cpp @@ +586,1 @@ > Want to remove the trailing whitespace while you're here? @@ +830,5 @@ > nsresult rv = media->SetMediaText(aConditionText); > if (NS_SUCCEEDED(rv)) { > mMedia = media; > + } else { > + aRv.Throw(rv); Hmm, why is it ok to not return here? Won't we crash below when accessing mMedia?

Comment on attachment 8822849 [details] [diff] [review] part 15. Convert CSSFontFaceRule to WebIDL Review of attachment 8822849 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/CSSFontFaceRule.webidl @@ +7,5 @@ > + * https://drafts.csswg.org/css-fonts/#om-fontface > + */ > + > +// https://drafts.csswg.org/cssom/#the-csspagerule-interface > +// But we implement a very old draft, apparently.... Maybe file a bug?

> Spec bug filed? Yes, back then. https://github.com/w3c/csswg-drafts/issues/845 > Alphabetical order? Done. > Want to remove the trailing whitespace while you're here? Done. > Hmm, why is it ok to not return here? Won't we crash below when accessing mMedia? It's not, good catch. We want to return whether rv succeeded or not, actually, because if it succeeded we've already done SetMediaText. In practice, of course, mMedia is never null so this code is unreachable, but that's tracked in bug 1328808. > Maybe file a bug? Bug 1058408 tracks already. Will add that to the comment.

Would it make sense to file spec bugs about adding SameObject to the style attribute of CSSKeyframeRule and PutForwards to the style attribute of CSSFontFaceRule and CSSKeyframeRule?

Probably not a bad idea, good catch. CSSFontFaceRule no longer has a .style that's a CSSStyleDeclaration in the spec, so that part is not relevant. But you're right that CSSKeyframeRule should act like the others. Filed https://github.com/w3c/csswg-drafts/issues/906

Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/47fff3ed2a6c part 1. Make all CSS rules cycle-collected. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/4be9dd0ee8cc part 2. Remove the now-unused GetExistingDOMRule method. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/681ca39f5411 part 3. Give CSS rules a PreCreate hook so we can safely wrappercache them. r=peterv https://hg.mozilla.org/integration/mozilla-inbound/rev/efbe11a48de7 part 4. Make css::Rule wrappercached. r=heycam,peterv https://hg.mozilla.org/integration/mozilla-inbound/rev/1a603d30e5e1 part 5. Get rid of DOMCSSStyleRule. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/174bb89ffd6e part 6. Make css::Rule inherit from nsIDOMCSSRule. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/4135091f5f31 part 7. Push the nsIDOMCSSRule implementation up to css::Rule. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/41dca86782fa part 8. Get rid of css::Rule::GetDOMRule. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/78bb9ce60976 part 9. Add a CSSRule Web IDL interface. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/cf9d73160d1a part 10. Convert CSSNamespaceRule to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/2a5a9437777f part 11. Convert CSSImportRule to WebIDL. r=peterv,heycam,mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/4e48a59adc5e part 12. Convert CSSStyleRule to WebIDL. r=peterv,heycam,mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/d50c9372cc2e part 13. Convert media, supports, and moz-document rules to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/e37ac7b0c913 part 14. Convert CSSPageRule to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/eda4ef1e0288 part 15. Convert CSSFontFaceRule to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/224432acd298 part 16. Convert CSSFontFeatureValuesRule to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/af8ee866cbed part 17. Convert CSSKeyframeRule to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/b882f9cdffa3 part 18. Convert CSSKeyframesRule to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/0a99b184da55 part 19. Convert CSSCounterStyleRule to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/ecaa5f64caf6 part 20. Remove the remaining bits we added to support a mix of WebIDL and non-WebIDL rules. r=peterv

Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/855a52109378 followup to placate static analysis and reopen the CLOSED TREE.

Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7a82c1099a05 More bug 851892 static analysis placation on CLOSED TREE

Backout by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e8b32aaa2eec Back out for Windows build bustage on CLOSED TREE

I filed bug 1331102 on the problem that caused this to be backed out.

Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3089dd379077 part 1. Make all CSS rules cycle-collected. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/65422477b3a5 part 2. Remove the now-unused GetExistingDOMRule method. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/dc81a167a75d part 3. Give CSS rules a PreCreate hook so we can safely wrappercache them. r=peterv https://hg.mozilla.org/integration/mozilla-inbound/rev/d6aa4c6192df part 4. Make css::Rule wrappercached. r=heycam,peterv https://hg.mozilla.org/integration/mozilla-inbound/rev/719bb9f41e5b part 5. Get rid of DOMCSSStyleRule. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/74c0ba66f108 part 6. Make css::Rule inherit from nsIDOMCSSRule. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/ca4b1fb9cae4 part 7. Push the nsIDOMCSSRule implementation up to css::Rule. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/2eab85b00159 part 8. Get rid of css::Rule::GetDOMRule. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/e841a2796375 part 9. Add a CSSRule Web IDL interface. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/c53cd7bdf8b3 part 10. Convert CSSNamespaceRule to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/29cf8acbd21e part 11. Convert CSSImportRule to WebIDL. r=peterv,heycam,mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/a8da3c34983f part 12. Convert CSSStyleRule to WebIDL. r=peterv,heycam,mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/d8da2a3d8f10 part 13. Convert media, supports, and moz-document rules to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/d58e0e5069ef part 14. Convert CSSPageRule to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/a25638588b6b part 15. Convert CSSFontFaceRule to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/73858e15c8c0 part 16. Convert CSSFontFeatureValuesRule to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/a9cab46e8b45 part 17. Convert CSSKeyframeRule to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/9bf5bcb3e8c5 part 18. Convert CSSKeyframesRule to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/5f491bf49b85 part 19. Convert CSSCounterStyleRule to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/b9c4115cdeac part 20. Remove the remaining bits we added to support a mix of WebIDL and non-WebIDL rules. r=peterv

https://hg.mozilla.org/mozilla-central/rev/3089dd379077 https://hg.mozilla.org/mozilla-central/rev/65422477b3a5 https://hg.mozilla.org/mozilla-central/rev/dc81a167a75d https://hg.mozilla.org/mozilla-central/rev/d6aa4c6192df https://hg.mozilla.org/mozilla-central/rev/719bb9f41e5b https://hg.mozilla.org/mozilla-central/rev/74c0ba66f108 https://hg.mozilla.org/mozilla-central/rev/ca4b1fb9cae4 https://hg.mozilla.org/mozilla-central/rev/2eab85b00159 https://hg.mozilla.org/mozilla-central/rev/e841a2796375 https://hg.mozilla.org/mozilla-central/rev/c53cd7bdf8b3 https://hg.mozilla.org/mozilla-central/rev/29cf8acbd21e https://hg.mozilla.org/mozilla-central/rev/a8da3c34983f https://hg.mozilla.org/mozilla-central/rev/d8da2a3d8f10 https://hg.mozilla.org/mozilla-central/rev/d58e0e5069ef https://hg.mozilla.org/mozilla-central/rev/a25638588b6b https://hg.mozilla.org/mozilla-central/rev/73858e15c8c0 https://hg.mozilla.org/mozilla-central/rev/a9cab46e8b45 https://hg.mozilla.org/mozilla-central/rev/9bf5bcb3e8c5 https://hg.mozilla.org/mozilla-central/rev/5f491bf49b85 https://hg.mozilla.org/mozilla-central/rev/b9c4115cdeac

Backed out in https://hg.mozilla.org/mozilla-central/rev/50bd08fad349 for what I fear is going to be a very inconvenient failure mode, as far as I know only Android opt, in some crashtest in crashtest-4 around 20% of the time something will fail with something like "load failed: timed out waiting for spell checks to end" or "load failed: null" or "load failed: timed out while taking snapshot (bug in harness?)" or "load failed: timed out waiting for test to complete (waiting for onload scripts to complete)" - there's a nice variety of reasons and tests in https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=5ce3882eec21be3a70e4afc050959ca2f76bfa76&filter-searchStr=crashtest-4&filter-resultStatus=testfailed&filter-resultStatus=busted

Clearly this changeset just has bad karma. ;) Some try runs suggest that the culprit is in fact the very first part. Compare https://treeherder.mozilla.org/#/jobs?repo=try&revision=dabc8c24f7dcc0e8705183398750dddcbd6fce5c (try push right on top of the backout) to https://treeherder.mozilla.org/#/jobs?repo=try&revision=4df4228c567f5b89f31bbbf7089b873d0a70e5a9 (try push with just part 1 on top of that). I'll look into this on Tuesday.

So just for my information, there are apparently failures in the following tests: 1) layout/style/crashtests/460209-1.html -- has a single @import rule of a sheet with no rules. @import rules are already cycle-collected, even before my changes. 2) layout/style/crashtests/448161-1.html, layout/style/crashtests/448161-2.html -- don't contain any CSS rules (or indeed any obvious document-level style) at all, other than UA sheets. 3) layout/style/crashtests/456196.html -- only has inline style (i.e. not a rule), plus UA rules. 4) layout/style/crashtests/460323-1.html -- this has some actual StyleRule instances. 5) layout/style/crashtests/460217-1.html -- has @font-face rules only Some of these failures (very few) are crashes, but they are highly uninformative; they just claim the crash is in libc.so somehere, with libvdm.so on the callstack for a while after that.

Some more try data: 1) Not actually telling the cycle collector about the edge from StyleRule to mDOMRule right on top of part 1 there seems to make the problem go away. See https://treeherder.mozilla.org/#/jobs?repo=try&revision=405efca1f8dc3f6543ad77dedfef02b68046de49 (mDeclaration is not actually cycle-collected, so that edge is not likely to be relevant). 2) With the full patch stack, and mDOMRule merged into StyleRule, but mDOMDeclaration still a separate object, not telling CC about the wrapper of the mDOMDeclaration (which is now the only thing StyleRule's CC really does, other than tracing its own wrapper) does NOT really help, except maybe in terms of frequency. See https://treeherder.mozilla.org/#/jobs?repo=try&revision=d77d6a97d44faf929c2e77b365904deaa23b0f2c I still have no idea why any of this makes android opt unhappy, of course. :(

And https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfd32818e9d062ca2fa53e73c8bf752056c22ba4 shows that not tracing through the actual wrapper of the StyleRule itself is also not enough to prevent the failures appearing....

And https://treeherder.mozilla.org/#/jobs?repo=try&revision=7602bb4b71b3dfb43f1c768ff76464f254d48ac4 shows that even tracing nothing at all from StyleRule doesn't help, with all the patches applied. At this point I'm at a bit of a loss for what the heck is going on. :(

This is a total guess, but you could try setting dom.cycle_collector.incremental to false and see if that helps. In bug 1023758 comment 53, there was some weird interaction between CSS and incremental CC.

No, that still fails, unfortunately... https://treeherder.mozilla.org/#/jobs?repo=try&revision=0741b2d48a70bcc4431bc8a20a8c57658d37571a

OK, with some printf debugging on try, the mystery is resolved. Starting with that first changeset where we add rules to CC, we end up having a single CC take somewhere near 5 minutes on Android opt, which is basically the test timeout. If that CC happens during a test, that test fails with one of the various timeout messages. If it happens between tests, we get a green run. The reason for that super-long CC is that it has about a million objects it's tracing and then freeing. Those objects all come from layout/style/crashtests/439184-1.html which creates a stylesheet with 2^20 StyleRule instances in it. Before my changes, since those rules were never touched from the CSSOM, they never ended up in the CC graph. Now they do. Presumably non-Android platforms have faster hardware so the CC doesn't take as long there (testing this hypothesis now). Presumably debug Android has a higher timeout and the CC doesn't take enough longer to hit that timeout. Still thinking about how to address this.

Per discussion with smaug and mccr8, I tried adding skippability stuff to StyleRule. That helps a lot. The longest CC pause on the Android opt crashtests goes from 300,000ms or so to 9,000ms. On linux64 opt, it goes from 1000ms to 318ms. I have no idea why Android was so slow, by the way... Oh, and as for Android debug builds... They do run layout/style/crashtests/439184-1.html but then never seem to end up with a CC that sees all those rules. Presumably something kills off the entire sheet and all its rules (which aren't implicated in any cycles I'm aware of to start with) before CC ever manages to run. That's why those weren't failing. Anyway, I'll post some interdiffs for the above patches that add skippability to StyleRule and ServoStyleRule.

Comment on attachment 8828456 [details] [diff] [review] Interdiff for part 4 to update rule skippability to the wrappercache changes > NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_BEGIN(Rule) >- // Note that we can't make use of IsKnownLive() here directly, because we may >- // be subclassed by something that needs tracing. Once we have a wrapper >- // cache we should be able to do better here. >- return tmp->IsCCLeaf(); >+ return tmp->IsCCLeaf() || >+ (tmp->IsKnownLive() && tmp->HasNothingToTrace(tmp)); Technically CanSkip doesn't need to ensure HasNothingToTrace, only CanSkipInCC (since CC needs to get right C++->grayJS edges to the graph). > NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_IN_CC_BEGIN(Rule) >- // Note that we can't make use of IsKnownLive() here directly, because we may >- // be subclassed by something that needs tracing. Once we have a wrapper >- // cache we should be able to do better here. >- return tmp->IsCCLeaf(); >+ return tmp->IsCCLeaf() || >+ (tmp->IsKnownLive() && tmp->HasNothingToTrace(tmp)); > NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_IN_CC_END So IsCCLeaf() doesn't ever return true if there is some js which might be traced, right? In all the places we check PreservingWrapper(); and wrapper is the only thing to trace.

Comment on attachment 8828455 [details] [diff] [review] Interdiff for part 1 to add skippability to rule cycle collection >@@ -1406,17 +1406,16 @@ StyleRule::DropReferences() > { > if (mDOMRule) { > mDOMRule->DOMDeclaration()->DropReference(); > mDOMRule = nullptr; > } > > if (mDeclaration) { > mDeclaration->SetOwningRule(nullptr); >- mDeclaration = nullptr; > } Why is this ok? Why we don't want to clear the reference? >+bool >+StyleRule::IsCCLeaf() const >+{ >+ if (!Rule::IsCCLeaf()) { >+ return false; >+ } >+ >+ return !mDOMRule || !mDOMRule->DOMDeclaration()->PreservingWrapper(); >+} So DOMRule doesn't keep anything else but declaration alive, right? >+Rule::IsKnownLive() const >+{ >+ StyleSheet* sheet = GetStyleSheet(); >+ if (!sheet) { >+ return false; >+ } >+ >+ if (!sheet->IsOwnedByDocument()) { >+ return false; >+ } ok, some patch in this bug is adding IsOwnedByDocument(). I assume it does what I think it does. Could you still explain the ownership model of StyleRule and mDOMRule and mDOMRule->DOMDeclaration()? Or perhaps how that is changed then in some patch? Which patch should I look at?

> Technically CanSkip doesn't need to ensure HasNothingToTrace Great. I will drop that. ;) > So IsCCLeaf() doesn't ever return true if there is some js which might be traced, right? That's correct, since those would be outgoing owning edges. > Why is this ok? Why we don't want to clear the reference? For the same reason it's OK to not cc-traverse the pointer: mDeclaration is not a CC-ed object, and hence we can't have a cycle through it. > So DOMRule doesn't keep anything else but declaration alive, right? That's correct. The only member it has is a DOMCSSDeclarationImpl (as an object, not a pointer). > ok, some patch in this bug is adding IsOwnedByDocument() In bug 1332353 if you want to look at it. And the idea is that it does what you think it does, yes. > Could you still explain the ownership model of StyleRule and mDOMRule and mDOMRule->DOMDeclaration()? In the end it doesn't matter too much, because that model is changed in part 5 in this bug. After that change, StyleRule has an mDOMDeclaration which it owns (via UniquePtr), and they share a single refcount via aggregation. Each has a separate nsWrapperCache, but only the StyleRule has a CC participant; the mDOMDeclaration forwards the CC-isupports and CC-participant QIs to StyleRule so from CC's point of view they're a single object. Hence the need to trace the mDOMDeclaration's wrapper from the StyleRule's Trace, and check it in StyleRule's IsCCLeaf().

Having never touched CC skippability stuff, and to the extent that the ownership description sounds OK to me, I'm happy to defer to smaug's r+es here.

OK, as long as you buy my claims about IsCCLeaf and IsKnownLive working as advertised!

Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9a4b96c10b23 part 1. Make all CSS rules cycle-collected. r=heycam,smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/795ebf4423ba part 2. Remove the now-unused GetExistingDOMRule method. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/ca8c00f9738d part 3. Give CSS rules a PreCreate hook so we can safely wrappercache them. r=peterv https://hg.mozilla.org/integration/mozilla-inbound/rev/3be6690b9f0a part 4. Make css::Rule wrappercached. r=heycam,peterv,smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/2c2afe732553 part 5. Get rid of DOMCSSStyleRule. r=heycam,smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/271c7cd7b59a part 6. Make css::Rule inherit from nsIDOMCSSRule. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/659550973d4d part 7. Push the nsIDOMCSSRule implementation up to css::Rule. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/9ef39356f62c part 8. Get rid of css::Rule::GetDOMRule. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/b975a988fb33 part 9. Add a CSSRule Web IDL interface. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/9dee4f98503c part 10. Convert CSSNamespaceRule to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/eab8ccda41b8 part 11. Convert CSSImportRule to WebIDL. r=peterv,heycam,mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/31103a03f2c2 part 12. Convert CSSStyleRule to WebIDL. r=peterv,heycam,mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/1bbbc436690d part 13. Convert media, supports, and moz-document rules to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/34c280ac5f73 part 14. Convert CSSPageRule to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/332bb85695a9 part 15. Convert CSSFontFaceRule to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/430f2759de65 part 16. Convert CSSFontFeatureValuesRule to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/76136a4cadcc part 17. Convert CSSKeyframeRule to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/663ad79f5c13 part 18. Convert CSSKeyframesRule to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/f62d345e9d41 part 19. Convert CSSCounterStyleRule to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/c79e8bee853e part 20. Remove the remaining bits we added to support a mix of WebIDL and non-WebIDL rules. r=peterv

Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8f8a4d1c49ec part 1. Make all CSS rules cycle-collected. r=heycam,smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/0d8f9b1c8d61 part 2. Remove the now-unused GetExistingDOMRule method. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/4f24650b72a1 part 3. Give CSS rules a PreCreate hook so we can safely wrappercache them. r=peterv https://hg.mozilla.org/integration/mozilla-inbound/rev/b2f8a7db0858 part 4. Make css::Rule wrappercached. r=heycam,peterv,smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/a0fd8df61f56 part 5. Get rid of DOMCSSStyleRule. r=heycam,smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/9d8b2eaea267 part 6. Make css::Rule inherit from nsIDOMCSSRule. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/030208c7e084 part 7. Push the nsIDOMCSSRule implementation up to css::Rule. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/a70bbdbbdf27 part 8. Get rid of css::Rule::GetDOMRule. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/b9e55a13fd3b part 9. Add a CSSRule Web IDL interface. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/5e0e575589a3 part 10. Convert CSSNamespaceRule to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/aa9330ed3a07 part 11. Convert CSSImportRule to WebIDL. r=peterv,heycam,mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/02bd976e7157 part 12. Convert CSSStyleRule to WebIDL. r=peterv,heycam,mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/336c0e3ea229 part 13. Convert media, supports, and moz-document rules to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/c70e7a0b7d29 part 14. Convert CSSPageRule to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/72c7a4a0b278 part 15. Convert CSSFontFaceRule to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/1dd6a0bbfb37 part 16. Convert CSSFontFeatureValuesRule to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/8e899668b28b part 17. Convert CSSKeyframeRule to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/2e061df954b3 part 18. Convert CSSKeyframesRule to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/8bf7bcdac8d3 part 19. Convert CSSCounterStyleRule to WebIDL. r=peterv,heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/5b65e31bdffb part 20. Remove the remaining bits we added to support a mix of WebIDL and non-WebIDL rules. r=peterv

Backout by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/mozilla-inbound/rev/7d4dc504eea5 Backed out changeset c79e8bee853e https://hg.mozilla.org/integration/mozilla-inbound/rev/b01edf06d37c Backed out changeset f62d345e9d41 https://hg.mozilla.org/integration/mozilla-inbound/rev/b1de38223150 Backed out changeset 663ad79f5c13 https://hg.mozilla.org/integration/mozilla-inbound/rev/26a4cd58030a Backed out changeset 76136a4cadcc https://hg.mozilla.org/integration/mozilla-inbound/rev/5bed364016fe Backed out changeset 430f2759de65 https://hg.mozilla.org/integration/mozilla-inbound/rev/14f30033ee60 Backed out changeset 332bb85695a9 https://hg.mozilla.org/integration/mozilla-inbound/rev/5523b32e5425 Backed out changeset 34c280ac5f73 https://hg.mozilla.org/integration/mozilla-inbound/rev/633b0b55c6af Backed out changeset 1bbbc436690d https://hg.mozilla.org/integration/mozilla-inbound/rev/1c5e1faf2fdd Backed out changeset 31103a03f2c2 https://hg.mozilla.org/integration/mozilla-inbound/rev/7dbef218327b Backed out changeset eab8ccda41b8 https://hg.mozilla.org/integration/mozilla-inbound/rev/23dc67f25818 Backed out changeset 9dee4f98503c https://hg.mozilla.org/integration/mozilla-inbound/rev/95d8e109513e Backed out changeset b975a988fb33 https://hg.mozilla.org/integration/mozilla-inbound/rev/aba82d453b03 Backed out changeset 9ef39356f62c https://hg.mozilla.org/integration/mozilla-inbound/rev/6a929848ba43 Backed out changeset 659550973d4d https://hg.mozilla.org/integration/mozilla-inbound/rev/ee32dc6b1a12 Backed out changeset 271c7cd7b59a https://hg.mozilla.org/integration/mozilla-inbound/rev/3ef461d046d5 Backed out changeset 2c2afe732553 https://hg.mozilla.org/integration/mozilla-inbound/rev/21fd8a323aa4 Backed out changeset 3be6690b9f0a https://hg.mozilla.org/integration/mozilla-inbound/rev/a1161010563e Backed out changeset ca8c00f9738d https://hg.mozilla.org/integration/mozilla-inbound/rev/e345379ea857 Backed out changeset 795ebf4423ba https://hg.mozilla.org/integration/mozilla-inbound/rev/6fb381da04b5 Backed out changeset 9a4b96c10b23 for build bustage. r=backout on a CLOSED TREE

Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/621e14f5c951 followup. Disable most of Rule::IsKnownLive for now to reopen CLOSED TREE

https://hg.mozilla.org/mozilla-central/rev/8f8a4d1c49ec https://hg.mozilla.org/mozilla-central/rev/0d8f9b1c8d61 https://hg.mozilla.org/mozilla-central/rev/4f24650b72a1 https://hg.mozilla.org/mozilla-central/rev/b2f8a7db0858 https://hg.mozilla.org/mozilla-central/rev/a0fd8df61f56 https://hg.mozilla.org/mozilla-central/rev/9d8b2eaea267 https://hg.mozilla.org/mozilla-central/rev/030208c7e084 https://hg.mozilla.org/mozilla-central/rev/a70bbdbbdf27 https://hg.mozilla.org/mozilla-central/rev/b9e55a13fd3b https://hg.mozilla.org/mozilla-central/rev/5e0e575589a3 https://hg.mozilla.org/mozilla-central/rev/aa9330ed3a07 https://hg.mozilla.org/mozilla-central/rev/02bd976e7157 https://hg.mozilla.org/mozilla-central/rev/336c0e3ea229 https://hg.mozilla.org/mozilla-central/rev/c70e7a0b7d29 https://hg.mozilla.org/mozilla-central/rev/72c7a4a0b278 https://hg.mozilla.org/mozilla-central/rev/1dd6a0bbfb37 https://hg.mozilla.org/mozilla-central/rev/8e899668b28b https://hg.mozilla.org/mozilla-central/rev/2e061df954b3 https://hg.mozilla.org/mozilla-central/rev/8bf7bcdac8d3 https://hg.mozilla.org/mozilla-central/rev/5b65e31bdffb https://hg.mozilla.org/mozilla-central/rev/621e14f5c951

I've had a look through the CSSOM MDN pages, and I don't think this bug really requires much in the way of docs changes. As it is, we've already got the interfaces described in the same kind of way as for any other DOM interface. What we could probably do with is to generally update these pages, to add missing properties, etc., make sure everything is up to date. I'll file a separate docs bug for that.