1274443 - Ownership handling of servo style structs is broken

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

Our current handling of ownership of servo style structs is kind of sloppy, and we don't maintain mBits in such a way as to avoid trying to delete the servo style structs via the PresArena. A few of the problems are:
* We don't set the inherit bit in nsStyleContext::StyleData
* We flip off the inherit bit in nsStyleContext::GetUniqueStyleData
* We don't cache the servo style structs everwhere (not a correctness issue but potentially a performance one).

This is currently causing shutdown crashes in stylo.

I have some patches, but there are still some issues to be worked out. I'll finish them up later today or shortly thereafter.

Comment 1

[Bobby Holley (:bholley)]

Also, it looks like there are various other consumers of NS_STYLE_INHERIT_BIT in the DOM, and I'm skeptical that they're actually interested in the question of "does the style context own the the style struct: http://searchfox.org/mozilla-central/search?q=NS_STYLE_INHERIT_BIT&redirect=true

Cameron, what are those doing, and what bits do they expect in the Servo case?

If we need to do something different with those bits, we can always just key struct struct destruction off of the mSource type as well as the bits. But in that case we'd still need to understand what we need to do with the bits.

Comment 2

[Cameron McCormack (:heycam)]

NS_STYLE_INHERIT_BIT is used for other reasons when we want to turn an nsStyleStructID into a bit.  I think we instead need to search for uses of nsStyleContext::mBits.  It looks like the only external use of the ownership information is through nsStyleContext::HasCachedDependentStyleData, which is used in RestyleManager (and so not relevant for stylo).

In bug 1188721 I plan to simplify the ownership of style structs by making them refcounted, which would remove the use of mBits to store ownership information.

Comment 3

[Bobby Holley (:bholley)]

(In reply to Cameron McCormack (:heycam) from comment #2)
> NS_STYLE_INHERIT_BIT is used for other reasons when we want to turn an
> nsStyleStructID into a bit.  I think we instead need to search for uses of
> nsStyleContext::mBits.  It looks like the only external use of the ownership
> information is through nsStyleContext::HasCachedDependentStyleData, which is
> used in RestyleManager (and so not relevant for stylo).

Ah I see! That makes sense.

> In bug 1188721 I plan to simplify the ownership of style structs by making
> them refcounted, which would remove the use of mBits to store ownership
> information.

Oh ok - I guess that would totally collide with the patches that I have for this bug. Are we just waiting on review from dbaron? If so maybe we should just try to get review time from him and solve this problem that way.

It's relatively urgent though, because this setup makes the current stylo implementation very memory-hazardous. I guess I can also just land some more hacky stuff that makes this more correct than before, without doing all the refactoring I was intending to do.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 1 - Replace mPresArenaAllocCount with a hashtable to make it easy to spot the cause of double-frees. v1

I think the overhead here should be fine, though I'm open to the argument that
it's too much.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 2 - Properly cache and flag servo style structs in nsStyleContext::StyleData. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 3 - Avoid calling ApplyStyleFixups for ServoComputedValues. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 4 - Always cache reset structs for servo. v1

The shared code always caches inherited structs, but not reset structs. Without
 change we will always do an FFI call to get the struct.

I'm not entirely sure this is correct or maps to what nsRuleNode does.
Cameron, can you take an extra close look at this one?

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8754656 [details] [diff] [review]
Part 1 - Replace mPresArenaAllocCount with a hashtable to make it easy to spot the cause of double-frees. v1

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

We do perform a lot of pres arena allocations, but since it's DEBUG-only, this is probably fine.  (Though I might just try out a debug build with and without this patch just to see if it you can notice any slow down.  Debug builds are already pretty slow.)


An alternative to checking for unfreed objects would be to go through the PLArenaPool yourself and compare against the FreeList entries.  You could do a quick check of number of allocated objects in the arena against the length of the FreeList, and if you wanted to know the specific pointers that weren't freed you could go iterate through the allocated regions in the arena and compare them to the FreeList entries.

Checking for double free would probably involve going through the FreeList, though, which is probably going to be slower than maintaining the hash table.

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8754657 [details] [diff] [review]
Part 2 - Properly cache and flag servo style structs in nsStyleContext::StyleData. v1

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

::: layout/style/nsStyleContext.cpp
@@ +453,5 @@
> +  } else {
> +    /* the Servo-backed StyleContextSource owns the struct */
> +    newData = StyleStructFromServoComputedValues(aSID);
> +    AddStyleBit(nsCachedStyleData::GetBitForSID(aSID));
> +    SetStyle(aSID, const_cast<void*>(newData));

It is not too common for reset style structs to be cached on style contexts, since we can usually cache them on the rule node.  (Unless you use font-relative units, or explicitly inherit values, etc.)  This is why mCachedResetData is not allocated by default.  So I think we should be taking the same approach here to save memory.  This is what the macro-generated DoGetStyleFoo methods do for Servo-backed reset structs currently.  (We should at least be making these behave similarly.)

So I suggest removing the SetStyle call but leaving the AddStyleBit call.

Comment 10

[Cameron McCormack (:heycam)]

Oh, I see that you do make those macro-generated methods consistent in part 4.  I'm not sure we should be doing that.

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8754658 [details] [diff] [review]
Part 3 - Avoid calling ApplyStyleFixups for ServoComputedValues. v1

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

::: layout/style/nsStyleContext.cpp
@@ +652,5 @@
> +nsStyleContext::SetStyleBits()
> +{
> +  // XXXbholley: We should get this information directly from the
> +  // ServoComputedValues rather than computing it here. This setup here is
> +  // probably not something we should ship.

Since we're not in |#ifdef MOZ_STYLO| code here it sounds a bit worrying to talk about not shipping code that is indeed shipping and being run.  Can you adjust your comment to say that this all applies only when we do have a ServoComputedValues backing our style context?

@@ +690,4 @@
>  nsStyleContext::ApplyStyleFixups(bool aSkipParentDisplayBasedStyleFixup)
>  {
> +  MOZ_ASSERT(!mSource.IsServoComputedValues(),
> +             "Can't do Gecko style fixups on servo values");

Nit: "Servo"

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8754659 [details] [diff] [review]
Part 4 - Always cache reset structs for servo. v1

>Bug 1274443 - Always cache reset structs for servo. v1
>
>The shared code always caches inherited structs, but not reset structs. Without
>this change we will always do an FFI call to get the struct.

OK, I see why we're doing this now.  I guess taking the extra memory hit is better than going back to the ServoComputedValues object each time.

(If we could generate bindings for the ComputedValues object we could poke at from C++ it might be good to just grab it out directly than go through the FFI calls.)

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8754657 [details] [diff] [review]
Part 2 - Properly cache and flag servo style structs in nsStyleContext::StyleData. v1

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

So let's leave the SetStyle() call in.  Can you add a comment saying that this defeats the memory optimization of allocating the nsCachedResetData separately, but that it's better when we have a Servo-backed style context to do this than the FFI call.  (Here, and in the Part 4 change.)

Comment 14

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2439f47700d1

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c33e04d605c
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e6513158f1f
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b5d2ad721da
https://hg.mozilla.org/integration/mozilla-inbound/rev/15df40db9c16

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7c33e04d605c
https://hg.mozilla.org/mozilla-central/rev/7e6513158f1f
https://hg.mozilla.org/mozilla-central/rev/0b5d2ad721da
https://hg.mozilla.org/mozilla-central/rev/15df40db9c16