1379830 - stylo: refcount ServoStyleContext::mPresContext

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Bobby Holley (:bholley)]

Right now we're storing a weak prescontext pointer on ServoStyleContext. This is a problem since style contexts can outlive the prescontext via getComputedStyle. This works fine for Gecko, since the computed style object holds an ArenaRefPtr to the style context, which is nulled out when the arena is destroyed, which happens before the prescontext goes away, which prevents style contects from outliving the prescontext. But in the Servo case we just have normal refcounting, so style contexts can live arbitrarily long.

This is a problem because the style context destructor does this:

http://searchfox.org/mozilla-central/rev/b7e531410ebc1c7d74a78e86a894e69608db318c/layout/style/nsStyleContext.cpp#166

So if the PresContext is gone, we're invoking a method on a freed |this|, which is bad.

Currently ServoStyleContexts are only minted on the main thread, but after bug 1367904 they'll be created in parallel. So we'll need to use atomic refcounting, but we should be able to get away with using atomics from Rust code only. This will avoid unnecessary overhead for all the other consumers of nsPresContext.

Comment 1

[Bobby Holley (:bholley)]

Manish, given that this would bitrot bug 1367904, we should coordinate the fix. Do you want to work the fix into your patches in that bug, or should we land it on top?

Comment 2

[Manish Goregaokar [:manishearth]]

I think there are also cases where StyleSet() is null when that dtor is called. Unsure what we're supposed to do there. My code currently bypasses all this; the destructor was moved completely into Gecko.

Comment 3

[Bobby Holley (:bholley)]

(In reply to Manish Goregaokar [:manishearth] from comment #2)
> My code currently bypasses
> all this; the destructor was moved completely into Gecko.

Sure, but my point is that we want to make mPresContext a strong pointer, which interacts with your patches.

Comment 4

[Manish Goregaokar [:manishearth]]

(That comment was typed before I got your next comment, and I missed posting this one)

So the root style context count machinery isn't necessary for Servo AFAICT. The only time mRootStyleContextCount gets read is in nsPresContext::HasCachedStyleData(), and it returns early in the servo case.


The added/removed calls do nothing in the servo case too.

But in general we shouldn't be holding onto a pointer that may be invalidated, so yeah, we should make this change. I would prefer it to be on top of my patches.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Won't holding to the prescontext hold on too much data? Can we find a way to clear the DOM style context pointer instead of keeping the whole thing alive?

Alternatively, we could consider moving the prescontext pointer into the frame.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> Won't holding to the prescontext hold on too much data?

I'm not super worried about it. Holding a style context alive past presshell destruction _can_ happen, but I don't think it's particularly common or easy to do by accident - so we just need to make it safe.

> Can we find a way to
> clear the DOM style context pointer instead of keeping the whole thing alive?

That would require either registering every style context with something (which would be slow, complex, and difficult to do from servo), or using a weak pointer, which would impose an unacceptable overhead during layout when we need the prescontext.

> 
> Alternatively, we could consider moving the prescontext pointer into the
> frame.

This could work, but would be a big change, so I'm not itching to do it unless we have a clear reason to.

The change I'm proposing in this bug is basically a no-op (modulo the refcounting traffic), and the only behavior change comes in the situations where we currently risk UAF.

Comment 7

[Boris Zbarsky [:bzbarsky]]

This is only an issue for style contexts coming from computed style, right?

So really, we would only really need to register the computed style stylecontexts, not all of them, and we wouldn't have to do it from servo or background threads or anything...  That's what Gecko does, really, with its ArenaRefPtr thing (the thing that gets registered is the ArenaRefPtr, not the style context; we could also simply register the computed style object somewhere).

Comment 8

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #7)
> This is only an issue for style contexts coming from computed style, right?

That I'm aware of. But in general, given that the objects holding the raw prescontext pointer are not arena-managed, it seems dicey.
 
> So really, we would only really need to register the computed style
> stylecontexts, not all of them, and we wouldn't have to do it from servo or
> background threads or anything...  That's what Gecko does, really, with its
> ArenaRefPtr thing (the thing that gets registered is the ArenaRefPtr, not
> the style context; we could also simply register the computed style object
> somewhere).

And then have it just clear the strong ref to the style context? Seems like that could work, though it does make me a bit nervous per above.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> But in general, given that the objects holding the raw prescontext pointer are not arena-managed, it seems dicey

Hmm.  I guess the failure mode with Gecko would be a deref of the style context pointer that it thinks it owns but is actually dead.  But the failure mode with stylo would be that this would succeed but then the prescontext pointer is dead...  I suppose the latter could be worse because it's got virtual functions and whatnot?

> And then have it just clear the strong ref to the style context?

Right.

Comment 10

[Bobby Holley (:bholley)]

So as discussed using ArenaRefPtr would basically solve the problem here. However, we had to disable ArenaRefPtr for ServoStyleContext because the current implementation is leading to invalid casts (see bug 1367904 comment 224). We should revisit that in this bug.

Comment 11

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1379830 - stylo: Allow ServoStyleContext to participate in ArenaRefPtr

This should do the ArenaRefPtr special casing.

I guess after this I just need to make mPresContext a strong pointer?

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 8887715 [details] [diff] [review]
Bug 1379830 - stylo: Allow ServoStyleContext to participate in ArenaRefPtr

Review of attachment 8887715 [details] [diff] [review]:
-----------------------------------------------------------------

At this point the whole PRES_ARENA_OBJECT_WITH_ARENAREFPTR_SUPPORT business is unused, right? Can we kill it?

> I guess after this I just need to make mPresContext a strong pointer?

No, the strong pointer and this patch are mutually exclusive.

Comment 13

[Manish Goregaokar [:manishearth]]

> At this point the whole PRES_ARENA_OBJECT_WITH_ARENAREFPTR_SUPPORT business is unused, right? Can we kill it?


Still need it to opt out of this switch, right?

Comment 14

[Manish Goregaokar [:manishearth]]

Ah, I think I can simplify a lot of this code

Comment 15

[Manish Goregaokar [:manishearth]]

Attachment: stylo: Allow ServoStyleContext to participate in ArenaRefPtr

Comment 16

[Bobby Holley (:bholley)]

We're pretty sure this is not exploitable on trunk, and is just a belt-and-suspenders fix.

Comment 17

[Manish Goregaokar [:manishearth]]

https://hg.mozilla.org/integration/autoland/rev/31ae02774ca909703e18c7b9f0a0f0a4f133e1ed

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/31ae02774ca9