737313 - nsCSSFrameConstructor::ConstructInline leaks the inline frame if one of its children fails to construct, triggering abort w/ "Assertion failure: mPresArenaAllocCount == 0 (Some pres arena objects were not freed), at nsPresShell.cpp:864"

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: testcase (only works w/ WIP css3-flexbox patches applied)

In cleaning up my flexbox patches & splitting them into bitesize pieces, I've made an intermediate patch that hooks up the "display: -moz-flexbox" keyword without actually doing the frame-construction. (It adds a nsCSSFrameConstructor method that simply returns a failure code.)

This nearly passes layout/style mochitests, but it actually triggers the following abort within test_value_computation.html:
{
Assertion failure: mPresArenaAllocCount == 0 (Some pres arena objects were not freed), at layout/base/nsPresShell.cpp:864
}

In this case, the not-freed pres arena object is a nsInlineFrame, which we create as a parent for a flexbox frame, and then when the flexbox frame-constructor fails, we propagate the failure upwards without freeing the inline frame.

The attached testcase reproduces this as well (in a flexbox-enabled build with only some of my patches applied, with the no-op frame constructor method as described above).

This probably isn't something we'd ever expect to hit in real life, because frame construction methods generally succeed, but it makes debugging & working with intermediate builds easier if this has sane fallback behavior.  Patch coming up.

Comment 1

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: fix v1

Here's the fix w/ crashtest.  Adds a line to destroy the inline frame if we fail while processing its children.  This seems to be what we do elsewhere in nsCSSFrameConstructor, and it's sufficient to fix the problem in my testing. (including if the inline frame has other children along with the flexbox)

I also added a clause to null-check the inline frame after its own construction, for good measure.  (IIRC NS_NewXXX isn't generally guaranteed to be infallible; correct me if I'm wrong on that.)

I'm including a crashtest, though the crashtest won't crash in any "real" builds -- only in builds with my flexbox patch-stack partially applied. I don't think it's possible to make a crash test for "real" builds, though (because I don't know of a way to reliably make a frame constructor fail), so this is (marginally) better than nothing.

Comment 2

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> In cleaning up my flexbox patches & splitting them into bitesize pieces,
> I've made an intermediate patch that hooks up the "display: -moz-flexbox"
> keyword without actually doing the frame-construction. (It adds a
> nsCSSFrameConstructor method that simply returns a failure code.)
> 
> This nearly passes layout/style mochitests, but it actually triggers the
> following abort within test_value_computation.html:

(For the record: After this bug's fix is applied, a few mochitests do fail with the partially-applied flexbox patch-stack described above, due to the frame-construction failure.  But this patch saves us from _aborting_, so that's a step in the right direction. :))

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 607432 [details] [diff] [review]
fix v1

Is calling newFrame->Destroy() really enough if we managed to create some frames and put them in childItems already?  What will clean up the frames in childItems?

We really need to move toward making all this frame construction stuff infallible.  :(

Comment 4

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(In reply to Boris Zbarsky (:bz) from comment #3)
> Comment on attachment 607432 [details] [diff] [review]
> fix v1
> 
> Is calling newFrame->Destroy() really enough if we managed to create some
> frames and put them in childItems already?

I believe so -- at least, this patch saves me from hitting that abort, even if I stick some extra content (e.g. text, <img>) before the flexbox in the crashtest.

I'll step through and see how those children actually get cleaned up -- I suspect Destroy() ends up recursively cleaning up the child subtree that has been created (and attached, I think) thus far.

Comment 5

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> I believe so -- at least, this patch saves me from hitting that abort, even
> if I stick some extra content (e.g. text, <img>) before the flexbox in the
> crashtest.

Just kidding -- that does make me abort. I thought I'd tested that, but I must've loaded the wrong testcase.

> I suspect Destroy() ends up recursively cleaning up the child subtree that has
> been created (and attached, I think) thus far.

(on further inspection: this doesn't save us because the child frames haven't yet been attached at that point)

Comment 6

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: fix v2

So one option is to just attach our in-progress child-list right before we call Destroy()... That seems to work here.

Comment 7

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(so that we'll recursively Destroy() any children that have been created)

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 607456 [details] [diff] [review]
fix v2

r=me

Comment 9

[Daniel Holbert [:dholbert] (PTO until 2/24)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a403afe78c47

Comment 10

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/a403afe78c47