This happens when I'm trying to enable stylo on XUL document in bug 1407847. When I try to inspect an XUL element with XBL binding, the content process crashes with "We should be able to map a raw rule to a rule". It seems that in inDOMUtils::GetCSSStyleRules, we do find out that the element has XBL binding (GetXBLBinding returns non-null), but the XBL binding doesn't seem to have Servo style set (GetServoStyleSet returns null). Further investigating shows that the XBL binding's mPrototypeBinding doesn't have mResources, and thus it returns null for Servo style set. It is not clear to me why the element is styled correctly, but we cannot find its styleset. I believe this is also the reason of crash in test devtools/client/inspector/test/browser_inspector_highlighter-xbl.js

I think I know what's going on here.

Hmmm... So it seems there are actually multiple reasons for this. And the first one I found is that inDOMUtils::GetCSSStyleRules doesn't traverse through binding base, and thus styleset inherited from base binding is not collected. The second issue is that the rule we collect from binding's style sheet is different from that used in styling, so even if the rules are all collected, we get different pointer. I have no idea why that happens...

I guess I know the other reason now.

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #2) > Hmmm... > > So it seems there are actually multiple reasons for this. And the first one > I found is that inDOMUtils::GetCSSStyleRules doesn't traverse through > binding base, and thus styleset inherited from base binding is not collected. > > The second issue is that the rule we collect from binding's style sheet is > different from that used in styling, so even if the rules are all collected, > we get different pointer. I have no idea why that happens... Is this because https://bugzilla.mozilla.org/show_bug.cgi?id=1406875#c6 ? I think that may be the case...

(In reply to Emilio Cobos Álvarez [:emilio] from comment #4) > Is this because https://bugzilla.mozilla.org/show_bug.cgi?id=1406875#c6 ? I > think that may be the case... Somehow I guess yes. At least, it seems that a newly-created unique inner may not be used in restyle, which is probably a problem.

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #7) > (In reply to Emilio Cobos Álvarez [:emilio] from comment #4) > > Is this because https://bugzilla.mozilla.org/show_bug.cgi?id=1406875#c6 ? I > > think that may be the case... > > Somehow I guess yes. At least, it seems that a newly-created unique inner > may not be used in restyle, which is probably a problem. Oh, I bet I know the reason for that. EnsureUniqueInnerOnCSSSheets does: if (mNeedsRestyleAfterEnsureUniqueInner) { MarkOriginsDirty(OriginFlags::All); } And MarkOriginsDirty does: SetStylistStyleSheetsDirty(); But if the set is an XBL styleset, it should also do SetStylistXBLStyleSheetsDirty(); on the "master" StyleSet. Otherwise there's no way the styleset knows that it needs to go through the bindings and update them.

Hmmm... Sounds like I can rework the part 2, then.

Comment on attachment 8917734 [details] Bug 1407888 part 2 - Make ServoStyleRuleMap::FillTableFromStyleSheet not make unique inner. I changed my mind. I think this is good enough. Ensuring unique binding sheet sounds quite wasteful, especially given that they can appear in normal document (for things like scrollbar), but we hardly show them in the inspector. I suspect that they can be seen only when some hidden prefs are turned on, which should be quite uncommon. Also, Gecko doesn't require unique inner in this case either.

Comment on attachment 8917733 [details] Bug 1407888 part 1 - Collect styleset from binding base in inDOMUtils::GetCSSStyleRules. https://reviewboard.mozilla.org/r/188654/#review193990

Comment on attachment 8917734 [details] Bug 1407888 part 2 - Make ServoStyleRuleMap::FillTableFromStyleSheet not make unique inner. https://reviewboard.mozilla.org/r/188656/#review193992 ::: layout/inspector/ServoStyleRuleMap.cpp:190 (Diff revision 1) > + // unique inner may not be taken to restyle properly, so we may end > + // up not being able to match rules there anyway. > + MOZ_ASSERT(aSheet->IsInnerUnique() || mStyleSet->IsForXBL(), > + "Non-XBL stylesheets should be made unique before " > + "initializing ServoStyleRuleMap"); > + FillTableFromRuleList(aSheet->GetCssRulesInternal(false)); Do /* aRequireUniqueInner = */ false or something to clarify what the boolean argument is. ::: layout/style/ServoStyleSheet.cpp:399 (Diff revision 1) > > ServoCSSRuleList* > -ServoStyleSheet::GetCssRulesInternal() > +ServoStyleSheet::GetCssRulesInternal(bool aRequireUniqueInner) > { > if (!mRuleList) { > + MOZ_ASSERT(IsInnerUnique() || aRequireUniqueInner || !mDocument, Do we need to assert the "IsInnerUnique()" bit here? Seems like we should just assert that aRequireUniqueInner is only allowed to be false if the sheet is not for a document. Actually, are there cases where the sheet has no associated document yet we do want to call EnsureUniqueInner()? ::: layout/style/StyleSheet.h:141 (Diff revision 1) > nsIDocument* aCloneDocument, > nsINode* aCloneOwningNode) const = 0; > > bool IsModified() const { return mDirty; } > > + inline bool IsInnerUnique() const; (Maybe "HasUniqueInner" reads better.)

Comment on attachment 8917734 [details] Bug 1407888 part 2 - Make ServoStyleRuleMap::FillTableFromStyleSheet not make unique inner. https://reviewboard.mozilla.org/r/188656/#review193998 ::: layout/inspector/ServoStyleRuleMap.cpp:190 (Diff revision 1) > + // unique inner may not be taken to restyle properly, so we may end > + // up not being able to match rules there anyway. > + MOZ_ASSERT(aSheet->IsInnerUnique() || mStyleSet->IsForXBL(), > + "Non-XBL stylesheets should be made unique before " > + "initializing ServoStyleRuleMap"); > + FillTableFromRuleList(aSheet->GetCssRulesInternal(false)); (Or an enum class, with `Yes`, `No` variants, too)

Comment on attachment 8917734 [details] Bug 1407888 part 2 - Make ServoStyleRuleMap::FillTableFromStyleSheet not make unique inner. https://reviewboard.mozilla.org/r/188656/#review193992 > Do we need to assert the "IsInnerUnique()" bit here? Seems like we should just assert that aRequireUniqueInner is only allowed to be false if the sheet is not for a document. Actually, are there cases where the sheet has no associated document yet we do want to call EnsureUniqueInner()? Yes, we do, otherwise it would also assert when called from ServoStyleRuleMap, because we use `aRequireUniqueInner = false` there unconditionally because we assume that the inner has already been made unique at that point.

Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e1287860a439 part 1 - Collect styleset from binding base in inDOMUtils::GetCSSStyleRules. r=heycam https://hg.mozilla.org/integration/autoland/rev/50e535750cc9 part 2 - Make ServoStyleRuleMap::FillTableFromStyleSheet not make unique inner. r=heycam

Backed out in https://hg.mozilla.org/integration/autoland/rev/1af997368071 for https://treeherder.mozilla.org/logviewer.html#?job_id=136681989&repo=autoland

Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/87d4de273b89 part 1 - Collect styleset from binding base in inDOMUtils::GetCSSStyleRules. r=heycam https://hg.mozilla.org/integration/autoland/rev/08e286687c40 part 2 - Make ServoStyleRuleMap::FillTableFromStyleSheet not make unique inner. r=heycam

https://hg.mozilla.org/mozilla-central/rev/87d4de273b89 https://hg.mozilla.org/mozilla-central/rev/08e286687c40