806056 - Assertion failure: "Some pres arena objects were not freed" with table, abs pos, preserve-3d

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase (asserts fatally when closed)

Closing the testcase gives me:

Assertion failure: mPresArenaAllocCount == 0 (Some pres arena objects were not freed), at layout/base/nsPresShell.cpp:767

Comment 1

[Mats Palmgren (inactive)]

I think this is a recent regression - in the last two weeks or so?

Comment 2

[Mats Palmgren (inactive)]

Attachment: frame tree

Comment 3

[Mats Palmgren (inactive)]

It's the abs.pos. frame that leaks.  It's a child of the row-group which
I guess got to be an abs.pos. container from this :
getElementsByTagName("tbody")[0].style.transformStyle = "preserve-3d"

So I suspect bug 804323 is the culprit.

Comment 4

[Mats Palmgren (inactive)]

Attachment: wip1

This fixes it, but I don't think it's the right approach.
Since any frame can have abs.pos. children now (right?) we should probably
just remove all DestroyAbsoluteFrames() calls and do it in one place...

Comment 5

[Mats Palmgren (inactive)]

Attachment: wip2

Something like this.  (also fixes the crash, but otherwise untested)

Comment 6

[Mats Palmgren (inactive)]

I'm wondering about the changes in bug 804323.
https://bugzilla.mozilla.org/attachment.cgi?id=675358&action=diff#a/layout/base/nsCSSFrameConstructor.cpp_sec1

> frame->MarkAsNotAbsoluteContainingBlock()

what if that frame has abs.pos. children?

> frame->MarkAsAbsoluteContainingBlock()

what if that frame has descendant placeholders with its out-of-flow
placed in a frame ancestor?

Should we move the abs.pos. frames in both those cases?

Comment 7

[Mats Palmgren (inactive)]

Oh, I see you have 
NS_ASSERTION(!HasAbsolutelyPositionedChildren(), "Think of the children!");
so I guess I just missed the code that does that.

Comment 8

[Mats Palmgren (inactive)]

nsContainerFrame::DestroyFrom is probably a better place for destroying
the abs.pos. children.  I think doing it in nsFrame would change the order
of destroying the out-of-flow first then the placeholder we currently have.
Not sure if it matters much though.  nsContainerFrame also seems a more
logical place since non-nsContainerFrames shouldn't have children anyway.

Comment 9

[Mats Palmgren (inactive)]

Attachment: testcase #2

Odd observation: flushing between the to style changes leads to
a different result, like so:
    document.getElementsByTagName("td")[0].style.position = "absolute";
    document.body.getClientRects();  // flush
    document.getElementsByTagName("tbody")[0].style.transformStyle = "preserve-3d";

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Mats Palmgren [:mats] from comment #6)
> I'm wondering about the changes in bug 804323.
> https://bugzilla.mozilla.org/attachment.cgi?id=675358&action=diff#a/layout/
> base/nsCSSFrameConstructor.cpp_sec1
> 
> > frame->MarkAsNotAbsoluteContainingBlock()
> 
> what if that frame has abs.pos. children?

Then NeedToReframeForAddingOrRemovingTransform would be true or frame->IsPositioned would be true. Well, that's the idea anyway.

> > frame->MarkAsAbsoluteContainingBlock()
> 
> what if that frame has descendant placeholders with its out-of-flow
> placed in a frame ancestor?

Then NeedToReframeForAddingOrRemovingTransform would be true. Hopefully.

Comment 11

[Mats Palmgren (inactive)]

Attachment: frame tree for testcase #2

Yes, I somehow missed NeedToReframeForAddingOrRemovingTransform :-)

It looks fine at first glance, for testcase #2 it does result in a frame
reconstruct for the <tbody> -- so why is the abs.pos. not its child then?
The final result doesn't seem to have NS_FRAME_HAS_ABSPOS_CHILDREN:
TableRowGroup(tbody)... [state=0000120000010000]

Whereas in the first frame dump it has:
TableRowGroup(tbody)... [state=0000122000010000]

Comment 12

[Mats Palmgren (inactive)]

Attachment: wip3

Destroy the abs/fixed pos. children in nsContainerFrame instead,
and assert they are gone in nsFrame::DestroyFrom.

Comment 13

[Mats Palmgren (inactive)]

I think the first frame tree is wrong, the table-row-group shouldn't be an
abs.pos. container even if it has some transform style.
I think the frame construction prevents this with FCDATA_SKIP_ABSPOS_PUSH:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#4408
  /* If FCDATA_SKIP_ABSPOS_PUSH is set, don't push this frame as an
     absolute containing block, no matter what its style says. */

And the Reflow methods for these frame classes doesn't reflow any abs.pos.
children anyway.

So, the current "frame->IsPositioned()" check is too liberal for making
a frame an abs.pos. container.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Yes, we still have various frame types that don't know how to be absolute containing blocks.  :(  I'd forgotten that.  :(

So yeah, we should probably sync up the check here with that information...

Comment 15

[Mats Palmgren (inactive)]

I guess we could add ReflowAbsoluteFrames() where needed and remove the
FCDATA_SKIP_ABSPOS_PUSH bit if we think that's the way to go...

But given that bug 804323 is scheduled for beta (afaict), perhaps
we should add a "eCannotBeAbsPosContainer" bit for IsFrameOfType()
in the relevant classes and skip those for now?

Comment 16

[Mats Palmgren (inactive)]

Boris, I wrote my comment before I saw yours, so can you think of a better
solution than a IsFrameOfType bit?

Comment 17

[Boris Zbarsky [:bzbarsky]]

> I guess we could add ReflowAbsoluteFrames() where needed and remove the
> FCDATA_SKIP_ABSPOS_PUSH bit if we think that's the way to go...

It is in general.  The problem is getting it right.  In particular, table-internal frames are randomly resized after reflow, which should in theory redo reflow of abs-pos kids because the containing block size changed.  :(  Pretty sure we have a bug on that.

Right now we use FCDATA_SKIP_ABSPOS_PUSH on all sorts of XUL frames (but not all of them!), popups, table captions, rowgroups, colgroups, various SVG stuff, plus some subset of the FULL_CTOR things.

Also, there's FCDATA_FORCE_NULL_ABSPOS_CONTAINER, which we might need to treat similarly, maybe.

I think we have two options.  Either we add the IsFrameOfType bit to all of those, or we add a framestate bit, and make sure that bit is set in nsCSSFrameConstructor::ConstructFrameFromItemInternal where we check for SKIP_ABSPOS_PUSH and in whatever FULL_CTOR cases skip pushing as the absolute containing block.  I suspect this will be a lot less in the way of code changes, and will be fully localized to the frame constructor, so if we have free framestate bits I would somewhat prefer this.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Mats Palmgren [:mats] from comment #15)
> But given that bug 804323 is scheduled for beta (afaict), perhaps
> we should add a "eCannotBeAbsPosContainer" bit for IsFrameOfType()
> in the relevant classes and skip those for now?

I think we will simply back out bug 691591 for beta.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I mean bug 691651.

Comment 20

[Mats Palmgren (inactive)]

Attachment: Destroy abs/fixed pos. child frames in a central location.

Comment 21

[Mats Palmgren (inactive)]

Attachment: Add a frame bit that says if a frame can have abs/fixed pos. children.

Add a frame bit that says if a frame can have abs/fixed pos. children.  Set the bit except for FCDATA_SKIP_ABSPOS_PUSH | FCDATA_FORCE_NULL_ABSPOS_CONTAINER.  Also set it unconditionally in NS_NewBlockFrame (includes NS_NewBlockFormattingContext), and for nsCanvasFrame and nsViewportFrame (includes nsPageContentFrame) since frames aren't created from frame constructor items in these cases (and all these classes (including derived classes) are expected to deal with abs/fixed pos. children).  Don't call MarkAsAbsoluteContainingBlock() on frames with the bit is unset.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 676443 [details] [diff] [review]
Add a frame bit that says if a frame can have abs/fixed pos. children.

I don't think this correctly handles the fact that some full constructors treat the frame as a containing block and some don't.

For example, I'm not sure this will set the bits correctly on fieldsets.  Does it?

I would somewhat prefer we just set the bit directly when we PushAbsoluteContainingBlock a frame....

Comment 23

[Mats Palmgren (inactive)]

All full constructor items will set the bit since they don't have
FCDATA_SKIP_ABSPOS_PUSH or FCDATA_FORCE_NULL_ABSPOS_CONTAINER,
so I'm overly liberal there.

Maybe I mislead you about what the bit means with my patch description...
The bit does not mean that the frame is a abs.pos. container, it just means
that if the *style* indicates it should be, then the bit must be set or the
PushAbsoluteContainingBlock/MarkAsAbsoluteContainingBlock is suppressed.

But yeah, setting it in PushAbsoluteContainingBlock seems a lot simpler...

Comment 24

[Boris Zbarsky [:bzbarsky]]

> The bit does not mean that the frame is a abs.pos. container, it just means
> that if the *style* indicates it should be, then the bit must be set

Right, but the point is some full-constructor things should be abs-pos containers if the style says so and some should not.

Doing it inside PushAbsoluteContainingBlock doesn't quite work because that only gets called when the frame is in fact positioned, no?  Or am I still missing something?

Comment 25

[Mats Palmgren (inactive)]

Comment on attachment 676441 [details] [diff] [review]
Destroy abs/fixed pos. child frames in a central location.

This patch actually did change the order of out-of-flow / placeholder
destruction since the block frame destroys all its non-positioned
frames when it destroys the lines so mFrames is empty when we
reach nsContainerFrame::DestroyFrom that now destroys the out-of-flows.
http://hg.mozilla.org/mozilla-central/annotate/e19e170d2f6d/layout/generic/nsBlockFrame.cpp#l275

It would be nice to have this in a central location but since Destroy
methods are called bottom-up it's hard to do that.  Maybe we should
leave it as is and just add the assertion that they are gone in nsFrame,
and perhaps also add the DestroyAbsoluteFrames in nsContainer for some
derived classes that doesn't need to deal with it, like
nsFieldSetFrame::DestroyFrom(nsIFrame* aDestructRoot)
{
  DestroyAbsoluteFrames(aDestructRoot);
  nsContainerFrame::DestroyFrom(aDestructRoot);
}

Doing it also in nsContainer will have the effect of protecting
against the crash we had here, although it's an extra call for
classes like nsBlockFrame that already did it.  I don't think
it matters performance-wise since it's behind a frame bit.

Comment 26

[Mats Palmgren (inactive)]

> Right, but the point is some full-constructor things should be abs-pos
> containers if the style says so and some should not.

Ah, you're saying that the MarkAsAbsoluteContainingBlock for the
AddOrRemoveTransform style change will cause bugs also for the
full ctor things that sometimes shouldn't be containers.
Sorry, I'm a little thick-headed sometimes, and it *is* 5am here :-)

> Doing it inside PushAbsoluteContainingBlock doesn't quite work because that
> only gets called when the frame is in fact positioned, no?

Yeah, you're right, we probably just need to deal with full ctors manually
and not be so liberal there.  I'll sleep on it... ;-)

Comment 27

[Al Billings [:abillings - ex-MoCo]]

Can someone propose a security rating?

Comment 28

[Mats Palmgren (inactive)]

Attachment: Make nsContainerFrame destroy abs/fixed pos. child frames unless a derived class already did so

Comment 29

[Mats Palmgren (inactive)]

Attachment: Add a frame bit that says if a frame can have abs/fixed pos. children

I'm adding the bit in all places where we *consider* calling
PushAbsoluteContainingBlock for a frame, and also in a couple of places
where we call MarkAsAbsoluteContainingBlock() directly.

https://tbpl.mozilla.org/?tree=Try&rev=0935b0976dcb

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 677268 [details] [diff] [review]
Make nsContainerFrame destroy abs/fixed pos. child frames unless a derived class already did so

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

::: layout/generic/nsContainerFrame.cpp
@@ +209,5 @@
> +nsContainerFrame::DestroyAbsoluteFrames(nsIFrame* aDestructRoot)
> +{
> +  if (IsAbsoluteContainer()) {
> +    GetAbsoluteContainingBlock()->DestroyFrames(this, aDestructRoot);
> +    MarkAsNotAbsoluteContainingBlock();

Why we do we have to call MarkAsNotAbsoluteContainingBlock?

::: layout/generic/nsGfxScrollFrame.cpp
@@ +90,5 @@
>  
>  void
>  nsHTMLScrollFrame::DestroyFrom(nsIFrame* aDestructRoot)
>  {
> +  DestroyAbsoluteFrames(aDestructRoot);

Why is this needed?

Comment 31

[Mats Palmgren (inactive)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30)
> Why we do we have to call MarkAsNotAbsoluteContainingBlock?

It's not strictly necessary, but it makes IsAbsoluteContainer()
false so we avoid doing this again.  I guess it's just an
extra property lookup though, so maybe not worth it...
I'll take it out if you want.

> >  nsHTMLScrollFrame::DestroyFrom(nsIFrame* aDestructRoot)
> >  {
> > +  DestroyAbsoluteFrames(aDestructRoot);
> 
> Why is this needed?

I figured that mInner could have descendant placeholders so it's to
satisfy the invariant that the out-flow-frames are destroyed first.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 677270 [details] [diff] [review]
Add a frame bit that says if a frame can have abs/fixed pos. children

r=me

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 677268 [details] [diff] [review]
Make nsContainerFrame destroy abs/fixed pos. child frames unless a derived class already did so

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

In practice, mInner.Destroy() won't destroy abs-pos stuff, since it only destroys anonymous scrollparts. But I guess this doesn't hurt and better safe than sorry!

Comment 34

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d15d35d7443b
https://hg.mozilla.org/integration/mozilla-inbound/rev/a718b68248ed

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d15d35d7443b
https://hg.mozilla.org/mozilla-central/rev/a718b68248ed

Comment 36

[Mats Palmgren (inactive)]

Comment on attachment 677270 [details] [diff] [review]
Add a frame bit that says if a frame can have abs/fixed pos. children

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 804323
User impact if declined: crash
Testing completed (on m-c, etc.): on m-c since 2012-11-02
Risk to taking this patch (and alternatives if risky):
alternative 1, land this + bug 804323 on aurora (medium risk), 
alternative 2, back out bug 691651 on aurora as we did for beta (low risk).
String or UUID changes made by this patch: none

Comment 37

[bhavana bajaj [:bajaj]]

(In reply to Mats Palmgren [:mats] from comment #36)
> Comment on attachment 677270 [details] [diff] [review]
> Add a frame bit that says if a frame can have abs/fixed pos. children
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): 804323
> User impact if declined: crash
> Testing completed (on m-c, etc.): on m-c since 2012-11-02
> Risk to taking this patch (and alternatives if risky):
> alternative 1, land this + bug 804323 on aurora (medium risk), 
> alternative 2, back out bug 691651 on aurora as we did for beta (low risk).
> String or UUID changes made by this patch: none

[Triage comment] : Can we go with alternative 2 at this point and get a backout patch ready which is low risk ?

Comment 38

[Mats Palmgren (inactive)]

(In reply to bhavana bajaj [:bajaj] from comment #37)
> [Triage comment] : Can we go with alternative 2 at this point and get a
> backout patch ready which is low risk ?

I've attached a backout patch for aurora in bug 804323 - it's the beta backout
patch with some minor tweaks, so it's about the same risk.

Comment 39

[bhavana bajaj [:bajaj]]

Comment on attachment 677270 [details] [diff] [review]
Add a frame bit that says if a frame can have abs/fixed pos. children

This patch is no longer needed, as an alternative backout patch was approved in bug 804323 to address the issue on aurora

Comment 40

[Mats Palmgren (inactive)]

Bug 804323 backed out bug 691591 on aurora, so Fx18:unaffected now.

Comment 41

[Mats Palmgren (inactive)]

Landed the crash tests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9646abfef92

Comment 42

[Ryan VanderMeulen [:RyanVM]]

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