Stylo: Opening DevTools inspector crashes during `inDOMUtils::GetCSSStyleRules`

NEW
Assigned to

Status

()

Core
CSS Parsing and Computation
P1
normal
a month ago
9 days ago

People

(Reporter: jryans, Assigned: xidorn)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {crash})

Firefox Tracking Flags

(Not tracked)

Details

STR:

1. Go to http://example.com in a Stylo build
2. Open the DevTools inspector panel

ER:

No crash.

AR:

We crash while AddRef'ing a bad pointer, looks like line 305 from inDOMUtils.cpp, which is:

frame #4: 0x0000000104b7fd81 XUL`inDOMUtils::GetCSSStyleRules(this=<unavailable>, aElement=<unavailable>, aPseudo=<unavailable>, _retval=<unavailable>) at inDOMUtils.cpp:305 [opt]
   302 	      ServoStyleRule* rule;
   303 	      rawRulesToRules.Get(rawRule, &rule);
   304 	      MOZ_ASSERT(rule, "We should always be able to map a raw rule to a rule.");
-> 305 	      RefPtr<css::Rule> ruleObj(rule);
   306 	      rules->AppendElement(ruleObj, false);
   307 	    }
   308 	  }

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x1)
  * frame #0: 0x0000000104b7fd81 XUL`inDOMUtils::GetCSSStyleRules(nsIDOMElement*, nsAString const&, nsIArrayExtensions**) [inlined] mozilla::RefPtrTraits<mozilla::css::Rule>::AddRef(aPtr=0x0000000000000001) at RefPtr.h:37 [opt]
    frame #1: 0x0000000104b7fd81 XUL`inDOMUtils::GetCSSStyleRules(nsIDOMElement*, nsAString const&, nsIArrayExtensions**) [inlined] RefPtr<mozilla::css::Rule>::ConstRemovingRefPtrTraits<mozilla::css::Rule>::AddRef(aPtr=0x0000000000000001) at RefPtr.h:392 [opt]
    frame #2: 0x0000000104b7fd81 XUL`inDOMUtils::GetCSSStyleRules(nsIDOMElement*, nsAString const&, nsIArrayExtensions**) [inlined] RefPtr<mozilla::css::Rule>::RefPtr(aRawPtr=0x0000000000000001) at RefPtr.h:111 [opt]
    frame #3: 0x0000000104b7fd81 XUL`inDOMUtils::GetCSSStyleRules(nsIDOMElement*, nsAString const&, nsIArrayExtensions**) [inlined] RefPtr<mozilla::css::Rule>::RefPtr(aRawPtr=0x0000000000000001) at RefPtr.h:109 [opt]
    frame #4: 0x0000000104b7fd81 XUL`inDOMUtils::GetCSSStyleRules(this=<unavailable>, aElement=<unavailable>, aPseudo=<unavailable>, _retval=<unavailable>) at inDOMUtils.cpp:305 [opt]
    frame #5: 0x0000000101c43a9e XUL`NS_InvokeByIndex at xptcinvoke_asm_x86_64_unix.S:129
    frame #6: 0x00000001026b50e6 XUL`CallMethodHelper::Call() [inlined] CallMethodHelper::Invoke(this=0x00007fff5fbf8230) at XPCWrappedNative.cpp:2010 [opt]
    frame #7: 0x00000001026b50cf XUL`CallMethodHelper::Call(this=<unavailable>) at XPCWrappedNative.cpp:1329 [opt]
    frame #8: 0x00000001026b7217 XUL`XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) [inlined] XPCWrappedNative::CallMethod(ccx=<unavailable>) at XPCWrappedNative.cpp:1296 [opt]
    frame #9: 0x00000001026b71e6 XUL`XPC_WN_CallMethod(cx=<unavailable>, argc=<unavailable>, vp=0x00007fff5fbf83c8) at XPCWrappedNativeJSOps.cpp:983 [opt]
    frame #10: 0x0000122b192f27df
(Reporter)

Comment 1

a month ago
:bholley mentioned in today's meeting that :xidorn should look into this.
Flags: needinfo?(xidorn+moz)
Assignee: nobody → xidorn+moz
Priority: -- → P1
Fwiw, the problem is that the code that fills the rawRulesToRules hashtable is broken in multiple ways:

1)  It only walks document stylesheets, whereas the rule list can contain rules
    from UA and user sheets, as well as from the mAdditionalSheets bits in nsIDocument.
    I expect this is the main problem on example.com
2)  It doesn't walk through group rules other than MEDIA_RULE.
3)  It doesn't walk through child sheets (@import).

In general, it seems very fragile and somewhat expensive.  It would be much better if we could directly map back from a RawServoStyleRule* to its DOM representation...
(Assignee)

Comment 3

a month ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #2)
> 1)  It only walks document stylesheets, whereas the rule list can contain rules
>     from UA and user sheets, as well as from the mAdditionalSheets bits in nsIDocument.
>     I expect this is the main problem on example.com

Hmmm, probably we should just walk through all of them and create the CSSOM wrapper objects.

> 2)  It doesn't walk through group rules other than MEDIA_RULE.
> 3)  It doesn't walk through child sheets (@import).

Others are not implemented yet... but shouldn't be too hard to fix.

> In general, it seems very fragile and somewhat expensive.  It would be much
> better if we could directly map back from a RawServoStyleRule* to its DOM
> representation...

It seems to be fragile because of lack of some unimplemented things...

And yeah, it is expensive. If we want to make it cheaper, we may want to cache the hash table somehow, but that would make things more fragile since we need to be careful about when we need to rebuild that map.

Maybe we can store the pointer of ServoStyleRule in the servo side StyleRule, then set / clear it when construct / destruct the corresponding wrapper object.

We would still need to walk through the CSSOM tree to build up the objects when someone hasn't been constructed. But that is probably cheaper than walking through every time?
(Assignee)

Comment 4

a month ago
If the first one is the only relevant one here, we can probably just fix the collecting code in this bug. If 2 and 3 are also relevant, I probably should implement those bits first.
(Assignee)

Updated

a month ago
Depends on: 1352968, 1355394
Flags: needinfo?(xidorn+moz)
> It seems to be fragile because of lack of some unimplemented things...

No, it's fragile because it's duplicating logic found elsewhere (actual "find all the matching rules" code) and will do bad things if the two ever get out of sync for any reason.

> Maybe we can store the pointer of ServoStyleRule in the servo side StyleRule

That would make a lot more sense.

> We would still need to walk through the CSSOM tree to build up the objects

Why?  Why can't we just trigger construction of a ServoStyleRule for a servo-side StyleRule directly?

> If the first one is the only relevant one here, we can probably just fix the collecting code in this bug.

For purposes of fixing this crash, the important part is that this code be in sync with the code that actually does rule matching.  If stylo doesn't implement non-@media grouping rules and doesn't implement @import (I mean on the matching level, not the CSSOM level!), then my item (1) is the only relevant one.  If it does implement those, then items (2) and (3) are relevant.
(Assignee)

Comment 6

a month ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #5)
> > It seems to be fragile because of lack of some unimplemented things...
> 
> No, it's fragile because it's duplicating logic found elsewhere (actual
> "find all the matching rules" code) and will do bad things if the two ever
> get out of sync for any reason.
> 
> > Maybe we can store the pointer of ServoStyleRule in the servo side StyleRule
> 
> That would make a lot more sense.
> 
> > We would still need to walk through the CSSOM tree to build up the objects
> 
> Why?  Why can't we just trigger construction of a ServoStyleRule for a
> servo-side StyleRule directly?

That means we would need to construct lots of additional objects which are not used at all in majority of cases. Also having Servo objects own Gecko refcounted objects, especially those can involve cycle reference, has the danger of either leaking or double free, so I think we should avoid it as much as possible. We've done that for CSSFontFaceRule with some trick (see bug 1345696 part 6), but I think we should eventually move away from that.

Also, if we don't walk through the CSSOM tree to link up the whole tree, but instead just construct the separate objects, we may need more complicated mechanism to setup their stylesheet pointer, otherwise changing on those rules may not trigger restyle correctly.
> That means we would need to construct lots of additional objects which are not used at all 

No, I meant why can't we have a method that takes a servo-side StyleRule and constructs the ServoStyleRule?  I'm not saying to do that construction eagerly.

I agree that some other CSSOM-walking may be involved.  What I am suggesting is that we start with the rule we want to construct and construct things as needed (its parent stylesheet, etc), not try to start at the "root" of the CSSOM and walk our way to this rule.  That should be much harder to mess up, I would think.
(Assignee)

Comment 8

a month ago
Because you don't know what are the parent rule and stylesheet of a StyleRule without walking through the tree. That object doesn't have any up pointer.
Ah.  That's ... really annoying.

In that case we do need to do the walk but at the very least:

1)  We need to be _extremely_ sure that it considers all possible sources of style rules.  We need to have some sort of pretty exhaustive tests for it, etc.

2)  We need to have some sort of dirty flag so we don't redo the walk when not needed.  Otherwise we're going to seriously regress inspector performance, and it's already not great.
Bug 1357716 has a patch that changes this from a crash to a warning, but doesn't fix the underlying problem of missing ServoStyleRules.
Depends on: 1357716
You need to log in before you can comment on or make changes to this bug.