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.

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?

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.

(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.

(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.

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.

(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.

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).

(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.

> 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.

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.

This should do the ArenaRefPtr special casing. I guess after this I just need to make mPresContext a strong pointer?

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.

> 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?

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

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