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"

RESOLVED FIXED in mozilla14

Status

()

Core
Layout
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

({assertion})

Trunk
mozilla14
assertion
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Created attachment 607423 [details]
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.
(Assignee)

Updated

6 years ago
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"
(Assignee)

Comment 1

6 years ago
Created attachment 607432 [details] [diff] [review]
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.
Attachment #607432 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

6 years ago
(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.  :(
(Assignee)

Comment 4

6 years ago
(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.
(Assignee)

Comment 5

6 years ago
(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)
(Assignee)

Comment 6

6 years ago
Created attachment 607456 [details] [diff] [review]
fix v2

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)
(Assignee)

Comment 7

6 years ago
(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+
(Assignee)

Comment 9

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a403afe78c47
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/a403afe78c47
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

4 years ago
Blocks: 334514
You need to log in before you can comment on or make changes to this bug.