Closed Bug 737313 Opened 8 years ago Closed 8 years ago

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"

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

(Keywords: assertion)

Attachments

(2 files, 1 obsolete file)

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.
Keywords: assertion
Summary: nsCSSFrameConstructor::ConstructInline leaks if one of its child frames fails to construct (and triggers Assertion failure: mPresArenaAllocCount == 0 (Some pres arena objects were not freed), at nsPresShell.cpp:864) → 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"
Attached patch fix v1 (obsolete) — Splinter Review
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.
Attachment #607432 - Flags: review?(bzbarsky)
(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 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.  :(
(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.
(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)
Attached patch fix v2Splinter Review
So one option is to just attach our in-progress child-list right before we call Destroy()... That seems to work here.
Attachment #607432 - Attachment is obsolete: true
Attachment #607432 - Flags: review?(bzbarsky)
Attachment #607456 - Flags: review?(bzbarsky)
(so that we'll recursively Destroy() any children that have been created)
Comment on attachment 607456 [details] [diff] [review]
fix v2

r=me
Attachment #607456 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/a403afe78c47
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Blocks: framedest
You need to log in before you can comment on or make changes to this bug.