718516 - crash @xul!nsOverflowContinuationTracker::SetUpListWalker

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Nils]

Attachment: testcase (crashes trunk)

Description:
This crash seems to be a problem during the layout of the page. It affects the current trunk version and does not crash in Firefox 9.0.1.

The following assertions are hit before a crash on a debug build:
###!!! ASSERTION: this type of frame can't have overflow containers: '(aProperty != nsContainerFrame::OverflowContainersProperty() && aProperty != nsContainerFrame::ExcessOverflowContainersProperty()) || IsFrameOfType(nsIFrame::eCanContainOverflowContainers)', file ../../../layout/generic/nsContainerFrame.cpp, line 1455
###!!! ASSERTION: StealFrame: can't find aChild: 'removed', file ../../../layout/generic/nsContainerFrame.cpp, line 1245
###!!! ASSERTION: StealFrame failure: 'NS_SUCCEEDED(rv)', file ../../../layout/generic/nsContainerFrame.cpp, line 1374 

Affected Versions:
	Firefox 12.0a (trunk)

Testcase:
	A testcase is attached

Stack Backtrace:

On Windows:
xul!nsOverflowContinuationTracker::SetUpListWalker+0x1c
xul!nsAbsoluteContainingBlock::Reflow+0x2bb7e2
xul!nsFrame::ReflowAbsoluteFrames+0x9f
xul!nsIFrame::GetOverflowAreasProperty+0x2b
xul!nsBlockReflowContext::ReflowBlock+0xd1
xul!nsBlockFrame::ReflowBlockFrame+0x46c
xul!nsBlockFrame::ReflowLine+0x13e
xul!nsBlockFrame::ReflowDirtyLines+0x21e
xul!nsBlockFrame::Reflow+0x315
xul!nsBlockReflowContext::ReflowBlock+0xd1
xul!nsBlockFrame::ReflowBlockFrame+0x46c
xul!nsBlockFrame::ReflowLine+0x13e
xul!nsBlockFrame::ReflowDirtyLines+0x21e
xul!nsBlockFrame::Reflow+0x315
xul!nsContainerFrame::ReflowChild+0x6a
xul!nsColumnSetFrame::ReflowChildren+0x294
xul!nsColumnSetFrame::Reflow+0x29a
xul!nsBlockReflowContext::ReflowBlock+0xd1
xul!nsBlockFrame::ReflowBlockFrame+0x46c
xul!nsBlockFrame::ReflowLine+0x13e

It crashes on following instruction:
xul!nsOverflowContinuationTracker::SetUpListWalker+0x1c:
71166a00 ff90b8000000    call    dword ptr [eax+0B8h] ds:002b:f0de80b7=????????


reported by nils of vulndev ltd.

Comment 1

[Ludovic Hirlimann [:Usul]]

Also crashes Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:11.0a2) Gecko/20120116 Firefox/11.0a2

Comment 2

[Daniel Veditz [:dveditz]]

Jet: please find someone on the layout team to investigate this security bug.

Comment 3

[Daniel Veditz [:dveditz]]

abillings crashed in beta as well (Fx10)

Comment 4

[Daniel Veditz [:dveditz]]

Jet: a regression range will help us find the flaw. It's sometime between 9.0.1 and 10b5 (but probably the Fx10 Nightly period).

Comment 5

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

Firefox 10b5 crash report for this: https://crash-stats.mozilla.com/report/index/bp-00adaa18-705c-4587-b9bb-9a4982120119

Comment 6

[Mats Palmgren (inactive)]

Attachment: stack + frame dump for the asserion

###!!! ASSERTION: this type of frame can't have overflow containers: '(aProperty != nsContainerFrame::OverflowContainersProperty() && aProperty != nsContainerFrame::ExcessOverflowContainersProperty()) || IsFrameOfType(nsIFrame::eCanContainOverflowContainers)', file layout/generic/nsContainerFrame.cpp, line 1455

Comment 7

[Mats Palmgren (inactive)]

Before the crash there is also:
###!!! ASSERTION: StealFrame: can't find aChild: 'removed', file layout/generic/nsContainerFrame.cpp, line 1245
###!!! ASSERTION: StealFrame failure: 'NS_SUCCEEDED(rv)', file layout/generic/nsContainerFrame.cpp, line 1374


http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsColumnSetFrame.cpp#93
93   virtual nsresult StealFrame(nsPresContext* aPresContext,
94                               nsIFrame*      aChild,
95                               bool           aForceNormal)
96   { // nsColumnSetFrame keeps overflow containers in main child list
97     return nsContainerFrame::StealFrame(aPresContext, aChild, true);
98   }

nsColumnSetFrame does not implement IsFrameOfType(eCanContainOverflowContainers).

It looks like nsOverflowContinuationTracker use Get/Set/RemovePropTableFrames
unconditionally though, so I guess that's why the overflow containers can't be
found on the principal list.

Comment 8

[Daniel Veditz [:dveditz]]

:mats -- since you've done some investigation already, should we assign this to you?

Comment 9

[Mats Palmgren (inactive)]

It looks like it requires non-trivial work on nsColumnSetFrame, so it would be better
if someone who knows that code takes it.

Comment 10

[Daniel Veditz [:dveditz]]

David: please find an appropriate owner for this bug in light of comment 9 if it's not you.

Comment 11

[Jet Villegas (inactive)]

Sending to sjohnson for a look. May be related to this changeset?

changeset:   83358:93b804e5f3f5
user:        Scott Johnson <[e-mail]>
date:        Sun Dec 25 23:25:59 2011 -0600
summary:     Bug 695222 - Implement column-fill part of CSS3 multicol spec. r=roc,dbaron

Comment 12

[Scott Johnson (:jwir3)]

Hm, so it looks like (from a bisect) that this is the first offending changeset:

changeset:   77842:731bbf32a6fd
user:        Ehsan Akhgari <[e-mail]>
date:        Wed May 11 19:53:34 2011 -0400
summary:     Bug 656130 - Part 1: Make sure that the absolute containing frame to be returned is actually marked as such in the frame tree; r=bzbarsky

I can look into it and see if I can determine what's causing this, and hopefully fix it, since I'm familiar with nsColumnSetFrame.

Comment 13

[(no longer active)]

Seems like nsColumnSetFrame doesn't know how to be an overflow container.  I can see two fixes for this bug, one is to teach it how to be one, the other is to stop calling FinishReflowWithAbsoluteFrames from nsColumnSetFrame::Reflow until that happens.  The second fix should be very simple and relatively safe for the branches.

Comment 14

[Scott Johnson (:jwir3)]

After giving this a bit of thought and speaking with Ehsan, I think I'm going to fix this bug using the second suggested fix in comment 13 (i.e. don't call FinishReflowWithAbsoluteFrames(), and instead replace the call to FinishAndStoreOverflow()). Then, I'll open a new bug to make nsColumnSetFrame an absolute container. 

Does this sound reasonable to everyone?

Comment 15

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

Yes.

Comment 16

[Scott Johnson (:jwir3)]

Attachment: b718516 - Removal of FinishReflowWithAbsoluteFrames

Comment 17

[(no longer active)]

Comment on attachment 595074 [details] [diff] [review]
b718516 - Removal of FinishReflowWithAbsoluteFrames

Please fix the indentation, r=me with that!

Comment 18

[Scott Johnson (:jwir3)]

Landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9062b3e04318

Also, I added a crashtest which is basically Nils' original testcase distilled into reftest form.

Comment 19

[Scott Johnson (:jwir3)]

Attachment: b718516 (with crashtest)

[Approval Request Comment]
Regression caused by (bug #): 656130
User impact if declined: Security vulnerability, although I'm not sure there's an exploit demonstrated yet.
Testing completed (on m-c, etc.): Not yet. Will be shortly.
Risk to taking this patch (and alternatives if risky): low, I believe
String changes made by this patch: None.

Comment 20

[Mats Palmgren (inactive)]

(In reply to Scott Johnson (:jwir3) from comment #18)
> Also, I added a crashtest which is basically Nils' original testcase
> distilled into reftest form.

FYI, we usually delay checking in tests for non-public bugs until the fix
has reached users on all relevant channels.

Comment 21

[Ed Morley [:emorley]]

Backed out of inbound for reftest failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=9062b3e04318
eg: https://tbpl.mozilla.org/php/getParsedLog.php?id=9161432&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c88465f4302

Comment 22

[Scott Johnson (:jwir3)]

(In reply to Mats Palmgren [:mats] from comment #20)
> (In reply to Scott Johnson (:jwir3) from comment #18)
> > Also, I added a crashtest which is basically Nils' original testcase
> > distilled into reftest form.
> 
> FYI, we usually delay checking in tests for non-public bugs until the fix
> has reached users on all relevant channels.

Oops, didn't realize that, sorry. I'll split it into a different patch so that it can be pushed up separately, seeing as this apparently got backed out due to reftest failures.

Comment 23

[Scott Johnson (:jwir3)]

Hm, so I didn't realize there were reftests testing that the abs-positioning worked in multi-col layout. Given that that is the case, I think I would rather make nsColumnSetFrame a full absolute container to fix this bug.

After speaking with mats, I think that comment 7 generally describes the problem. We think that making nsColumnSetFrame store its overflow containers on the main child list (i.e. fix the special behavior that nsColumnSetFrame does and make it do the same as the other frames).

That said, there's probably a reason that nsColumnSetFrame does what it does, possibly some uglyness that could trap me...

roc, do you know why nsColumnSetFrame was originally designed such that it didn't store NS_FRAME_IS_OVERFLOW_CONTAINER on the principal list, and perhaps have some suggestions to avoid potential pitfalls as I change this?

Comment 24

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

(In reply to Scott Johnson (:jwir3) from comment #23)
> roc, do you know why nsColumnSetFrame was originally designed such that it
> didn't store NS_FRAME_IS_OVERFLOW_CONTAINER on the principal list, and
> perhaps have some suggestions to avoid potential pitfalls as I change this?

It was designed long before overflow containers existed, so the current code probably just reflects our failure to handle columns correctly when we landed overflow containers.

Comment 25

[Scott Johnson (:jwir3)]

Attachment: b718516-2 (Without crashtest, with reftests disabled)

In the interest of getting this fixed ASAP, I'm putting up another patch that has the failing reftests disabled. I'd like a small sanity check, just to make sure I'm not doing something crazy.

Comment 26

[(no longer active)]

Comment on attachment 595797 [details] [diff] [review]
b718516-2 (Without crashtest, with reftests disabled)

Let's go with this.

Comment 27

[Scott Johnson (:jwir3)]

Sounds good. Also, for reference, the bug I created for making nsColumnSetFrame an overflow container is here:

https://bugzilla.mozilla.org/show_bug.cgi?id=724978

Comment 28

[Scott Johnson (:jwir3)]

Attachment: b718516-2 (Without crashtest, with only failing reftests disabled)

Re-enabled reftests that were passing.

Comment 29

[Scott Johnson (:jwir3)]

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/053d3cc103cb

Note that after this has been merged to m-c, it still needs to be pushed to beta, aurora, and probably release and esr-10 as well. Thus, we don't want to close this bug after it's been pushed to m-c.

Comment 30

[Scott Johnson (:jwir3)]

Attachment: b718516-test: Crashtest for this bug. [Don't push to inbound yet]

Don't push this to inbound or central until all releases have been updated with a fix for this bug.

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/053d3cc103cb

Comment 32

[Scott Johnson (:jwir3)]

Comment on attachment 596065 [details] [diff] [review]
b718516-2 (Without crashtest, with only failing reftests disabled)

[Approval Request Comment]
Regression caused by (bug #): 656130
User impact if declined: Security vulnerability, although I'm not sure there's an exploit demonstrated yet.
Testing completed (on m-c, etc.): Testing completed on m-c: https://tbpl.mozilla.org/?rev=d71dab82fff4
Risk to taking this patch (and alternatives if risky): low, I believe, because we're simply restoring previous functionality for one class (nsColumnSetFrame), until it can be made to work better with overflow containers.
String changes made by this patch: None.

Also, I'm not sure how to request approval for esr-10, but I think it will need to go into this branch as well.

Comment 33

[Alex Keybl [:akeybl]]

(In reply to Scott Johnson (:jwir3) from comment #32)
> Comment on attachment 596065 [details] [diff] [review]
> b718516-2 (Without crashtest, with only failing reftests disabled)
> 
> [Approval Request Comment]
> Regression caused by (bug #): 656130
> User impact if declined: Security vulnerability, although I'm not sure
> there's an exploit demonstrated yet.
> Testing completed (on m-c, etc.): Testing completed on m-c:
> https://tbpl.mozilla.org/?rev=d71dab82fff4
> Risk to taking this patch (and alternatives if risky): low, I believe,
> because we're simply restoring previous functionality for one class
> (nsColumnSetFrame), until it can be made to work better with overflow
> containers.
> String changes made by this patch: None.
> 
> Also, I'm not sure how to request approval for esr-10, but I think it will
> need to go into this branch as well.

Based upon the risk evaluation and the sg:crit nature of the bug, approving for Aurora 12 and Beta 11. Also tracking for ESR10. See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for info on landing there.

Comment 34

[Alex Keybl [:akeybl]]

Scott - please land on m-a/m-b ASAP. Please also land on mozilla-esr10 at the same time. For more information on the landing process, please see https://wiki.mozilla.org/Release_Management/ESR_Landing_Process

Comment 35

[Scott Johnson (:jwir3)]

Pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/ff20f55bc31b

Comment 36

[Scott Johnson (:jwir3)]

Pushed to beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/8921a6099974

Comment 37

[Scott Johnson (:jwir3)]

Pushed to esr10:
https://hg.mozilla.org/releases/mozilla-esr10/rev/726e2c223958

Comment 38

[Scott Johnson (:jwir3)]

Comment on attachment 596072 [details] [diff] [review]
b718516-test: Crashtest for this bug. [Don't push to inbound yet]

Should be able to push this test up to trunk now that all the channels have been updated with a fix...

Comment 39

[Mats Palmgren (inactive)]

Comment on attachment 596072 [details] [diff] [review]
b718516-test: Crashtest for this bug. [Don't push to inbound yet]

The test looks fine, but I think you should wait until the bug is public
before pushing it.  I'll leave that to dveditz to decide.

Comment 40

[Scott Johnson (:jwir3)]

(In reply to Mats Palmgren [:mats] from comment #39)
> Comment on attachment 596072 [details] [diff] [review]
> b718516-test: Crashtest for this bug. [Don't push to inbound yet]
> 
> The test looks fine, but I think you should wait until the bug is public
> before pushing it.  I'll leave that to dveditz to decide.

Sounds good. Thanks for looking at it!

Comment 41

[JP Rosevear [:jpr]]

Since this was pushed to aurora/beta without the crash test, can we mark as fixed for 11/12?

Comment 42

[Scott Johnson (:jwir3)]

(In reply to JP Rosevear [:jpr] from comment #41)
> Since this was pushed to aurora/beta without the crash test, can we mark as
> fixed for 11/12?

Yes. The crashtest will only be pushed to trunk.

Comment 43

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

As expected, this doesn't crash in 1.9.2.27 on Windows XP SP3.

Comment 44

[Daniel Veditz [:dveditz]]

The bug described here is fixed, closing bug. "Check in tests" has been moved to bug 733640.

Comment 45

[u279076]

Verified fixed in
 * Firefox 13.0a1 Nightly
 * Firefox 12.0a2 Aurora
 * Firefox 11.0b6
 * Firefox 10.0.3esr
using attached testcase

Comment 46

[Scott Johnson (:jwir3)]

Since the release has been completed, am I ok to check in the tests?

Comment 47

[Mats Palmgren (inactive)]

Crash tests landed in bug 733640.