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

RESOLVED FIXED

Status

()

Core
Layout
P3
critical
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: mats)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
assertion, crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.2 +
blocking1.9.1 -
wanted1.9.1 +
wanted1.9.0.x +
in-testsuite +

Firefox Tracking Flags

(blocking1.9.2 .13+, status1.9.2 .13-fixed, blocking1.9.1 .16+, status1.9.1 .16-fixed)

Details

(Whiteboard: [sg:critical? on 1.9.1][sg:dos frame-poisoned crash on 1.9.2], crash signature)

Attachments

(5 attachments, 4 obsolete attachments)

(Reporter)

Description

9 years ago
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.
(Reporter)

Updated

9 years ago
Whiteboard: [sg:critical?]
(Assignee)

Comment 1

9 years ago
Regression from bug 411835, I'm investigating...
Assignee: nobody → mats.palmgren
Blocks: 411835
Keywords: regression
OS: Mac OS X → All
Hardware: PC → All
(Assignee)

Comment 2

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

Comment 3

9 years ago
... or is this bit overloaded somehow?
(Assignee)

Comment 4

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

Comment 5

9 years ago
s/are no/are on/
(Assignee)

Comment 6

9 years ago
Created attachment 352054 [details] [diff] [review]
WIP

Perhaps like so?
Flags: blocking1.9.1?
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?
(Assignee)

Comment 8

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

Comment 9

9 years ago
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.
Attachment #352054 - Attachment is obsolete: true
(Assignee)

Comment 10

9 years ago
Created attachment 352116 [details] [diff] [review]
Patch rev. 1b

Minor macro param name change.

Pass mochitests/reftests/crashtests locally on Linux.
Attachment #352070 - Attachment is obsolete: true
Attachment #352116 - Flags: superreview?(roc)
Attachment #352116 - Flags: review?(roc)
I think it would be simpler to just drop the macro and write out the three calls explicitly.
(Assignee)

Comment 12

9 years ago
Created attachment 352186 [details] [diff] [review]
Patch rev. 2
Attachment #352116 - Attachment is obsolete: true
Attachment #352186 - Flags: superreview?(roc)
Attachment #352186 - Flags: review?(roc)
Attachment #352116 - Flags: superreview?(roc)
Attachment #352116 - Flags: review?(roc)
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.
(Assignee)

Comment 14

9 years ago
(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?
+      if (mixedList && (childFrame->GetStateBits() & NS_FRAME_OUT_OF_FLOW)) {

Did you mean NS_FRAME_IS_OVERFLOW_CONTAINER here?
(Assignee)

Comment 16

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

Comment 17

9 years ago
(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.
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.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Whiteboard: [sg:critical?] → [sg:critical?][needs revised patch]
Comment on attachment 352186 [details] [diff] [review]
Patch rev. 2

need an answer to my last question
Attachment #352186 - Flags: superreview?(roc)
Attachment #352186 - Flags: superreview-
Attachment #352186 - Flags: review?(roc)
Attachment #352186 - Flags: review-
Flags: wanted1.9.0.x?
Flags: blocking1.9.0.7?
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.7?
Flags: blocking1.9.0.7+
--> P1 as per bug 411835 comment 28
Priority: P2 → P1
See https://bugzilla.mozilla.org/show_bug.cgi?id=411835#c29
Flags: wanted1.9.1+
Flags: blocking1.9.1-
Flags: blocking1.9.1+
Priority: P1 → P3
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.
Flags: blocking1.9.0.7+ → blocking1.9.0.8+
Flags: blocking1.9.0.8+ → blocking1.9.0.9?
Flags: blocking1.9.0.10?
(Reporter)

Comment 23

8 years ago
Still crashes on trunk.
(Reporter)

Updated

8 years ago
Blocks: 489675

Updated

8 years ago
Duplicate of this bug: 489675

Updated

8 years ago
No longer blocks: 489675
Flags: blocking1.9.2?
Flags: blocking1.9.2? → wanted1.9.2+
(Reporter)

Comment 25

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

Updated

8 years ago
Blocks: 508473

Updated

8 years ago
Blocks: 489675
(Reporter)

Comment 26

8 years ago
Fantasai says her patch in bug 508473 fixes this :)

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

Updated

8 years ago
Assignee: matspal → nobody
(Reporter)

Comment 27

8 years ago
Fixed by bug 508473.  Hooray!

This testcase should go into the test suite once this bug can be made public.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [sg:critical?][needs revised patch] → [sg:critical?]
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?
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
status1.9.1: --- → wanted
status1.9.2: --- → wanted

Updated

7 years ago
blocking1.9.1: ? → .11+
blocking1.9.2: ? → .5+
Reassigning to roc (from nobody) to get an answer to comment 28.
Assignee: nobody → roc
blocking1.9.1: .11+ → ?
blocking1.9.2: .5+ → ?

Updated

7 years ago
blocking1.9.1: ? → .11+
blocking1.9.2: ? → .5+
blocking1.9.2: .5+ → .6+
I'm not sure. Maybe we could take Mats' patch, but is it worth the risk?
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?
Please take this off the blocker list.

Comment 33

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

Updated

7 years ago
blocking1.9.1: .11+ → needed
blocking1.9.2: .7+ → needed
(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?
Alright, we'll try to revive Mats' patch for the next release then.
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.
blocking1.9.1: needed → .13+
blocking1.9.2: needed → .10+

Comment 37

7 years ago
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

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

Comment 39

7 years ago
This is most likely not coming in for 1.9.2.11 and 1.9.1.14.
blocking1.9.1: .14+ → needed
blocking1.9.2: .11+ → needed
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.
blocking1.9.1: needed → .15+
blocking1.9.2: needed → .12+
Assignee: roc → matspal
(Assignee)

Comment 41

7 years ago
Created attachment 491416 [details] [diff] [review]
Patch rev. 3 (updated to 1.9.2 tip)
Attachment #352225 - Attachment is obsolete: true
(Assignee)

Comment 42

7 years ago
Created attachment 491418 [details] [diff] [review]
Patch rev. 3 + bug 411835 patch rev. 4 (for 1.9.1)
(Assignee)

Comment 43

7 years ago
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.
Whiteboard: [sg:critical?] → [sg:critical? on 1.9.1][sg:dos frame-poisoned crash on 1.9.2]
Let's take your latest two patches.
(Assignee)

Comment 45

7 years ago
Ok, let's get this formally reviewed and I'll land it today for the releases.
(Assignee)

Updated

7 years ago
Attachment #491416 - Flags: review?(roc)
(Assignee)

Updated

7 years ago
Attachment #491418 - Flags: review?(roc)
Attachment #491416 - Flags: review?(roc) → review+
Attachment #491418 - Flags: review?(roc) → review+
(Assignee)

Updated

7 years ago
Attachment #491416 - Flags: approval1.9.2.13?
(Assignee)

Updated

7 years ago
Attachment #491418 - Flags: approval1.9.1.16?
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?
(Assignee)

Comment 47

7 years ago
Yes, bug 526217 is fixed by Patch rev. 3 on this bug, so it will be fixed
on both branches.
(Assignee)

Updated

7 years ago
Blocks: 526217
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
Attachment #491418 - Flags: approval1.9.1.16? → approval1.9.1.16+
Attachment #491416 - Flags: approval1.9.2.13? → approval1.9.2.13+
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5d2cbf9c9669
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/cf3d01c081fd
status1.9.1: wanted → .16-fixed
status1.9.2: wanted → .13-fixed
Group: core-security
Crash Signature: [@ GetChildListNameFor]
(Reporter)

Comment 50

6 years ago
Crashtest: http://hg.mozilla.org/mozilla-central/rev/9388e2966118
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.