Last Comment Bug 468563 - Another crash [@ GetChildListNameFor] with -moz-column, position:absolute
: Another crash [@ GetChildListNameFor] with -moz-column, position:absolute
Status: RESOLVED FIXED
[sg:critical? on 1.9.1][sg:dos frame-...
: assertion, crash, regression, testcase
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: P3 critical (vote)
: ---
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on:
Blocks: randomstyles 411835 489675 508473 526217
  Show dependency treegraph
 
Reported: 2008-12-08 20:22 PST by Jesse Ruderman
Modified: 2011-06-20 18:07 PDT (History)
17 users (show)
roc: wanted1.9.2+
roc: blocking1.9.1-
roc: wanted1.9.1+
dveditz: wanted1.9.0.x+
jruderman: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.13+
.13-fixed
.16+
.16-fixed


Attachments
testcase (crashes Firefox when loaded) (234 bytes, text/html)
2008-12-08 20:22 PST, Jesse Ruderman
no flags Details
Frame dump #1 (10.91 KB, text/html)
2008-12-08 21:35 PST, Mats Palmgren (:mats)
no flags Details
WIP (707 bytes, patch)
2008-12-08 22:10 PST, Mats Palmgren (:mats)
no flags Details | Diff | Review
Patch rev. 1 (4.95 KB, patch)
2008-12-09 00:26 PST, Mats Palmgren (:mats)
no flags Details | Diff | Review
Patch rev. 1b (4.95 KB, patch)
2008-12-09 08:20 PST, Mats Palmgren (:mats)
no flags Details | Diff | Review
Patch rev. 2 (5.19 KB, patch)
2008-12-09 13:22 PST, Mats Palmgren (:mats)
roc: review-
roc: superreview-
Details | Diff | Review
Patch rev. 3 (5.59 KB, patch)
2008-12-09 16:41 PST, Mats Palmgren (:mats)
no flags Details | Diff | Review
Patch rev. 3 (updated to 1.9.2 tip) (4.75 KB, patch)
2010-11-17 18:30 PST, Mats Palmgren (:mats)
roc: review+
dveditz: approval1.9.2.13+
Details | Diff | Review
Patch rev. 3 + bug 411835 patch rev. 4 (for 1.9.1) (7.07 KB, patch)
2010-11-17 18:31 PST, Mats Palmgren (:mats)
roc: review+
dveditz: approval1.9.1.16+
Details | Diff | Review

Description Jesse Ruderman 2008-12-08 20:22:22 PST
Created attachment 352043 [details]
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 (:mats) 2008-12-08 21:01:19 PST
Regression from bug 411835, I'm investigating...
Comment 2 Mats Palmgren (:mats) 2008-12-08 21:35:39 PST
Created attachment 352052 [details]
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 (:mats) 2008-12-08 21:37:24 PST
... or is this bit overloaded somehow?
Comment 4 Mats Palmgren (:mats) 2008-12-08 21:55:54 PST
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 (:mats) 2008-12-08 21:56:35 PST
s/are no/are on/
Comment 6 Mats Palmgren (:mats) 2008-12-08 22:10:20 PST
Created attachment 352054 [details] [diff] [review]
WIP

Perhaps like so?
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-12-08 22:21:38 PST
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 (:mats) 2008-12-08 22:25:27 PST
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 (:mats) 2008-12-09 00:26:40 PST
Created attachment 352070 [details] [diff] [review]
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 (:mats) 2008-12-09 08:20:49 PST
Created attachment 352116 [details] [diff] [review]
Patch rev. 1b

Minor macro param name change.

Pass mochitests/reftests/crashtests locally on Linux.
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-12-09 12:53:51 PST
I think it would be simpler to just drop the macro and write out the three calls explicitly.
Comment 12 Mats Palmgren (:mats) 2008-12-09 13:22:12 PST
Created attachment 352186 [details] [diff] [review]
Patch rev. 2
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-12-09 13:30:22 PST
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 (:mats) 2008-12-09 15:48:01 PST
(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) (Exited; email my personal email if necessary) 2008-12-09 16:31:35 PST
+      if (mixedList && (childFrame->GetStateBits() & NS_FRAME_OUT_OF_FLOW)) {

Did you mean NS_FRAME_IS_OVERFLOW_CONTAINER here?
Comment 16 Mats Palmgren (:mats) 2008-12-09 16:41:29 PST
Created attachment 352225 [details] [diff] [review]
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 (:mats) 2008-12-09 16:46:57 PST
(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) (Exited; email my personal email if necessary) 2008-12-09 16:51:44 PST
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) (Exited; email my personal email if necessary) 2008-12-28 12:00:42 PST
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] 2009-01-30 15:35:15 PST
--> P1 as per bug 411835 comment 28
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-01-31 02:55:33 PST
See https://bugzilla.mozilla.org/show_bug.cgi?id=411835#c29
Comment 22 Samuel Sidler (old account; do not CC) 2009-02-02 07:49:39 PST
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 2009-06-15 17:31:13 PDT
Still crashes on trunk.
Comment 24 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2009-06-16 00:19:38 PDT
*** Bug 489675 has been marked as a duplicate of this bug. ***
Comment 25 Jesse Ruderman 2009-08-03 16:33:04 PDT
This bug makes me sad due to being easy to trigger and showing up with many different crash signatures.
Comment 26 Jesse Ruderman 2009-09-10 15:24:42 PDT
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 2009-12-24 11:27:07 PST
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] 2010-05-09 18:23:41 PDT
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] 2010-05-10 10:31:05 PDT
Reassigning to roc (from nobody) to get an answer to comment 28.
Comment 30 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-06-20 21:39:12 PDT
I'm not sure. Maybe we could take Mats' patch, but is it worth the risk?
Comment 31 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-06-23 22:28:28 PDT
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) (Exited; email my personal email if necessary) 2010-06-28 15:55:00 PDT
Please take this off the blocker list.
Comment 33 christian 2010-06-29 11:12:04 PDT
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] 2010-06-29 11:20:08 PDT
(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) (Exited; email my personal email if necessary) 2010-06-29 11:23:55 PDT
Alright, we'll try to revive Mats' patch for the next release then.
Comment 36 Daniel Veditz [:dveditz] 2010-08-23 10:52:25 PDT
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 2010-08-23 14:26:34 PDT
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 christian 2010-09-24 10:50:36 PDT
Though it could help other issues as well, I feel bug 508473 is too much churn and risk for a branch.
Comment 39 christian 2010-09-27 14:45:59 PDT
This is most likely not coming in for 1.9.2.11 and 1.9.1.14.
Comment 40 Daniel Veditz [:dveditz] 2010-10-13 10:48:26 PDT
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 (:mats) 2010-11-17 18:30:10 PST
Created attachment 491416 [details] [diff] [review]
Patch rev. 3 (updated to 1.9.2 tip)
Comment 42 Mats Palmgren (:mats) 2010-11-17 18:31:49 PST
Created attachment 491418 [details] [diff] [review]
Patch rev. 3 + bug 411835 patch rev. 4 (for 1.9.1)
Comment 43 Mats Palmgren (:mats) 2010-11-17 18:40:59 PST
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) (Exited; email my personal email if necessary) 2010-11-17 19:15:55 PST
Let's take your latest two patches.
Comment 45 Mats Palmgren (:mats) 2010-11-18 16:38:56 PST
Ok, let's get this formally reviewed and I'll land it today for the releases.
Comment 46 Daniel Veditz [:dveditz] 2010-11-18 19:23:56 PST
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 (:mats) 2010-11-18 19:39:45 PST
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] 2010-11-20 16:31:43 PST
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 50 Jesse Ruderman 2011-06-20 18:07:46 PDT
Crashtest: http://hg.mozilla.org/mozilla-central/rev/9388e2966118

Note You need to log in before you can comment on or make changes to this bug.