1402202 - Make FrameProperties::DeleteAll reentrancy safe

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P3)

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: frame-props

If you have properties that reference other properties, then we can end up trying to remove a frame property from within a property destructor and things fall apart.

This just makes sure we don't modify the array under us so that things still work.

Comment 1

[Mats Palmgren (inactive)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #0)
> If you have properties that reference other properties, then we can end up
> trying to remove a frame property from within a property destructor and
> things fall apart.

I wonder if that kind of complexity is something we should try to avoid
though.  Frame properties were only meant to be simple POD data, not
arbitrary graphs :-)

Could you describe which properties need this and what they are
used for please?  Or is this just a safety measure in case something
went wrong?

Comment 2

[Matt Woodrow (:mattwoodrow)]

See bug 1404181 for the patch queue, sorry about the size :)

We have new frame properties to track the set of display items allocated for each frame, and also new properties to store/retain an AnimatedGeometryRoot/ActiveScrolledRoot for the frame (if necessary).

We also have a property (on used on display roots) for storing the retained display list builder.

Deletion of the display list builder also triggers the deletion of all display items and AGR/ASRs for that frame.

If we decide to tear down the frame tree, and call DeleteAll on the display root, it can remove the display list builder entry first, and then recurse into trying to remove the entry for a display item or AGR/ASR and corrupts the vector.

Comment 3

[Mats Palmgren (inactive)]

Comment on attachment 8911024 [details] [diff] [review]
frame-props

This looks OK in principle, but we need to implement it differently.
The |mInDeleteAll| flag bloats nsIFrame by 64 bits, which I think
is unacceptable.

Could we do something like this instead:

  void DeleteAll(const nsIFrame* aFrame) {
    nsTArray<PropertyValue> toDelete;
    toDelete.SwapElements(mProperties);
    for (auto& prop : toDelete) {
      prop.DestroyValueFor(aFrame);
    }
    MOZ_ASSERT(mProperties.IsEmpty(), "a property dtor added new properties");
  }

The only requirement then is that your property dtors can't depend on
other properties to exist on the same frame, but I think that's
already an invariant since the order properties are deleted when
the frame is destroyed is undefined.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: frame-properties

Good catch on the increase to nsIFrame, we definitely don't want that.

Now that the recursive calls to DeleteProperty don't actually delete the property instantly (but when we get to them in the DeleteAll call), we need to make sure we're holding a strong ref to the values.

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 8921360 [details] [diff] [review]
frame-properties

> ... we need to make sure we're holding a strong ref to the values.

OK, makes sense.  Thanks.

Don't forget to add a commit message before landing...

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8763e22a77b2
Make FrameProperties::DeleteAll handle the case where deletion of a property triggers deletion of another. r=mats

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8763e22a77b2