Closed Bug 1407888 Opened 2 years ago Closed 2 years ago

stylo: inDOMUtils::GetCSSStyleRules cannot get StyleSet for XBL bindings in XUL document

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(2 files)

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.
Assignee: nobody → xidorn+moz
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.
Attachment #8917734 - Flags: review?(cam)
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.
Attachment #8917734 - Flags: review?(cam)
Comment on attachment 8917733 [details]
Bug 1407888 part 1 - Collect styleset from binding base in inDOMUtils::GetCSSStyleRules.

https://reviewboard.mozilla.org/r/188654/#review193990
Attachment #8917733 - Flags: review?(cam) → review+
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.)
Attachment #8917734 - Flags: review?(cam) → review+
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)
Priority: -- → P3
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
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
You need to log in before you can comment on or make changes to this bug.