468563 - Another crash [@ GetChildListNameFor] with -moz-column, position:absolute

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

###!!! ASSERTION: out-of-flow is already in the destroy queue: 'aDestroyQueue.IndexOf(outOfFlowFrame) == kNotFound', file /Users/jruderman/central/layout/base/nsCSSFrameConstructor.cpp, line 9240

Crash [@ GetChildListNameFor] dereferencing 0xdddddddd, like in bug 411835.

Comment 1

[Mats Palmgren (inactive)]

Regression from bug 411835, I'm investigating...

Comment 2

[Mats Palmgren (inactive)]

Attachment: Frame dump #1

There's a frame (blue) on the normal flow child list that has the frame
bit NS_FRAME_IS_OVERFLOW_CONTAINER (0x00000080) and thus we walk it twice.

roc, I'm guessing ColumnSetFrame pulled in a frame from
[excess]overflowContainersList and put it on its normal flow child list
without resetting this frame bit, does that sound plausible?

Comment 3

[Mats Palmgren (inactive)]

... or is this bit overloaded somehow?

Comment 4

[Mats Palmgren (inactive)]

It's ColumnSetFrame that sets the bit here:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsColumnSetFrame.cpp#716
it looks like these are no the normal flow child list by design.

As I pointed out in bug 411835 comment 17, DoDeletingFrameSubtree()
assumes they are always on [excess]overflowContainersList.

Suggestions on how to solve this conflict?

Comment 5

[Mats Palmgren (inactive)]

s/are no/are on/

Comment 6

[Mats Palmgren (inactive)]

Attachment: WIP

Perhaps like so?

Comment 7

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

Oh yeah, some frames do that. See
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsColumnSetFrame.cpp#91
CanvasFrame also does that:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLFrame.cpp#151

Maybe we can check for NS_FRAME_IS_OVERFLOW_CONTAINER while walking the normal child list?

Comment 8

[Mats Palmgren (inactive)]

Yes, that's better.  I see now that fantasai said in bug 411835 comment 11
that [excess]overflowContainersList can contain in-flow frames too...

Comment 9

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

What you suggested, 'childListName' is for the placeholder when recursing
its OOF though.

It passes the tests on related bugs, but I haven't run full regression
tests yet.  I'll do that tomorrow and ask for review then.

Comment 10

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1b

Minor macro param name change.

Pass mochitests/reftests/crashtests locally on Linux.

Comment 11

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

I think it would be simpler to just drop the macro and write out the three calls explicitly.

Comment 12

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Comment 13

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

Do we actually need that "if (childListName) DoDeletingOverflowContainers"? I don't think they can appear on any list other than the primary child list and the overflowContainersList/excessOverflowContainersList.

Comment 14

[Mats Palmgren (inactive)]

(In reply to comment #13)
> Do we actually need that "if (childListName) DoDeletingOverflowContainers"?

Probably not.  (I worried about not reaching the in-flow frames on
[excess]overflowContainersList that fantasai mentioned...)

> I don't think they can appear on any list other than the primary child list
> and the overflowContainersList/excessOverflowContainersList.

nsGkAtoms::overflowList maybe? (if so, we should treat it the same as
the primary child list anyway so it's not important)

So, I think what I suggested in bug 411835 comment 9 is probably
better then (i.e. walk [excess]overflowContainersList but process
their in-flow frames only), as in attachment 296632 [details] [diff] [review].

How else do we reach these in-flow overflow containers?

Comment 15

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

+      if (mixedList && (childFrame->GetStateBits() & NS_FRAME_OUT_OF_FLOW)) {

Did you mean NS_FRAME_IS_OVERFLOW_CONTAINER here?

Comment 16

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

I meant something like this.  Perhaps also

  if (childListName == nsGkAtoms::overflowContainersList ||
      childListName == nsGkAtoms::excessOverflowContainersList) {
    DoDeletingOverflowContainers(aFrameManager, aDestroyQueue,
                                 aRemovedFrame, childFrame);
  }

after the first DoDeletingFrameSubtree() call, unless the next-in-flow
overflow container is otherwise reachable...

Comment 17

[Mats Palmgren (inactive)]

(In reply to comment #15)
> +      if (mixedList && (childFrame->GetStateBits() & NS_FRAME_OUT_OF_FLOW)) {
> 
> Did you mean NS_FRAME_IS_OVERFLOW_CONTAINER here?

In that patch, mixedList is true when walking [excess]OverflowContainersList
where all frames should have the NS_FRAME_IS_OVERFLOW_CONTAINER bit.
I meant NS_FRAME_OUT_OF_FLOW to exclude out-of-flows.

Comment 18

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

That sounds reasonable.

I wonder if it would be better overall to just make DoDeletingFrameSubtree iterate over the continuation chains for all first-in-flow frames it encounters.

Comment 19

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

Comment on attachment 352186 [details] [diff] [review]
Patch rev. 2

need an answer to my last question

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

--> P1 as per bug 411835 comment 28

Comment 21

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

See https://bugzilla.mozilla.org/show_bug.cgi?id=411835#c29

Comment 22

[Samuel Sidler (old account; do not CC)]

Mats: What's going on with this bug and bug 411835? They're technically blockers for 1.9.0.7 which freezes tomorrow (Tuesday) at 11:59pm PST.

Comment 23

[Jesse Ruderman]

Still crashes on trunk.

Comment 25

[Jesse Ruderman]

This bug makes me sad due to being easy to trigger and showing up with many different crash signatures.

Comment 26

[Jesse Ruderman]

Fantasai says her patch in bug 508473 fixes this :)

Not changing the dependency because it would create a circular dependency (with bug 411835).

Comment 27

[Jesse Ruderman]

Fixed by bug 508473.  Hooray!

This testcase should go into the test suite once this bug can be made public.

Comment 28

[Daniel Veditz [:dveditz]]

I'm guessing bug 508473 isn't going to land on the branches. Is there a small/safe band-aide, maybe based on the patches Mats was originally working on?

Comment 29

[Daniel Veditz [:dveditz]]

Reassigning to roc (from nobody) to get an answer to comment 28.

Comment 30

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

I'm not sure. Maybe we could take Mats' patch, but is it worth the risk?

Comment 31

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

My inclination is to not fix this on branches. There's some work to come up with an alternative patch and there's some risk. Can we take this off the blocker list?

Comment 32

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

Please take this off the blocker list.

Comment 33

[LegNeato]

Ok, off the list for .7, though we still would eventually want a band-aid fix if possible (as asked in comment 28).

Comment 34

[Daniel Veditz [:dveditz]]

(In reply to comment #31)
> My inclination is to not fix this on branches. There's some work to come up
> with an alternative patch and there's some risk. Can we take this off the
> blocker list?

In comment 25 Jesse says this crash shows up frequently, and we know folks run fuzzers like Jesse's (Nils of the past two Pwn2Own Firefox winners, for one). 3.6 in particular has another year of supported life -- if we take this off the blocker list this time when do you plan on fixing it?

Comment 35

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

Alright, we'll try to revive Mats' patch for the next release then.

Comment 36

[Daniel Veditz [:dveditz]]

Please come up with a fix for this bug on the branches for the next round of releases. Bug Bounty hunters are fuzzing this area. Thankfully they often hit frame-poisoned crashes but, as in this case, not always.

Comment 37

[fantasai]

Do you want someone to write a patch for this specifically, or do you want the patch for 508473, which is what fixed this bug and others on trunk?

Comment 38

[LegNeato]

Though it could help other issues as well, I feel bug 508473 is too much churn and risk for a branch.

Comment 39

[LegNeato]

This is most likely not coming in for 1.9.2.11 and 1.9.1.14.

Comment 40

[Daniel Veditz [:dveditz]]

Bug 508473 doesn't seem sane for 1.9.2 (also requires bug 492627) and isn't even an option for 1.9.1. Variations on this bug keep popping out of various fuzzers, including those used by ZDI's stable of researchers. Please let's fix this.

Comment 41

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3 (updated to 1.9.2 tip)

Comment 42

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3 + bug 411835 patch rev. 4 (for 1.9.1)

Comment 43

[Mats Palmgren (inactive)]

The crash on the testcase in this bug and the testcase in bug 526217
are [sg:dos frame-poisoned crash] for the 1.9.2 branch.

Comment 44

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

Let's take your latest two patches.

Comment 45

[Mats Palmgren (inactive)]

Ok, let's get this formally reviewed and I'll land it today for the releases.

Comment 46

[Daniel Veditz [:dveditz]]

Comment on attachment 491418 [details] [diff] [review]
Patch rev. 3 + bug 411835 patch rev. 4 (for 1.9.1)

On later branches fixing bug 411835 caused bug 526217, which is not going to be frame-poisoned on the 1.9.1 branch. Have you done something to avoid that or are we just moving this -moz-column crash around somewhere else?

Comment 47

[Mats Palmgren (inactive)]

Yes, bug 526217 is fixed by Patch rev. 3 on this bug, so it will be fixed
on both branches.

Comment 48

[Daniel Veditz [:dveditz]]

Comment on attachment 491418 [details] [diff] [review]
Patch rev. 3 + bug 411835 patch rev. 4 (for 1.9.1)

Thanks! Approved for 1.9.2.13 and 1.9.1.16, a=dveditz

Comment 49

[Reed Loden [:reed]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5d2cbf9c9669
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/cf3d01c081fd

Comment 50

[Jesse Ruderman]

Crashtest: http://hg.mozilla.org/mozilla-central/rev/9388e2966118