Note: There are a few cases of duplicates in user autocompletion which are being worked on.

crash @xul!nsOverflowContinuationTracker::SetUpListWalker

VERIFIED FIXED in Firefox 11

Status

()

Core
Layout
--
critical
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: Nils, Assigned: jwir3)

Tracking

({regression, reproducible, testcase})

Trunk
mozilla13
regression, reproducible, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox10- wontfix, firefox11+ verified, firefox12+ verified, firefox13+ verified, firefox-esr1011+ verified, status1.9.2 unaffected)

Details

(Whiteboard: [sg:critical][test check-in moved to bug 733640][qa!], crash signature)

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 589002 [details]
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.
Attachment #589002 - Attachment mime type: text/plain → text/html
Also crashes Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:11.0a2) Gecko/20120116 Firefox/11.0a2
Status: UNCONFIRMED → NEW
Crash Signature: [@ nsOverflowContinuationTracker::nsOverflowContinuationTracker ]
Ever confirmed: true
Keywords: reproducible, testcase
OS: Windows 7 → All
Hardware: x86 → All
Jet: please find someone on the layout team to investigate this security bug.
Assignee: nobody → jet
status-firefox11: --- → affected
status-firefox12: --- → affected
status-firefox9: --- → unaffected
tracking-firefox11: --- → +
tracking-firefox12: --- → +
Keywords: regression
Whiteboard: [sg:critical]
status-firefox10: --- → affected
abillings crashed in beta as well (Fx10)
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).
Keywords: regressionwindow-wanted
Firefox 10b5 crash report for this: https://crash-stats.mozilla.com/report/index/bp-00adaa18-705c-4587-b9bb-9a4982120119
status-firefox10: affected → ---
status-firefox11: affected → ---
status-firefox12: affected → ---
status-firefox9: unaffected → ---
tracking-firefox11: + → ---
tracking-firefox12: + → ---
status-firefox10: --- → affected
status-firefox11: --- → affected
status-firefox12: --- → affected
tracking-firefox10: --- → -
tracking-firefox11: --- → +
tracking-firefox12: --- → +
Created attachment 591088 [details]
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
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.
:mats -- since you've done some investigation already, should we assign this to you?
It looks like it requires non-trivial work on nsColumnSetFrame, so it would be better
if someone who knows that code takes it.
David: please find an appropriate owner for this bug in light of comment 9 if it's not you.
Assignee: jet → dbaron
status-firefox-esr10: --- → affected
status-firefox13: --- → affected
tracking-firefox-esr10: --- → ?
tracking-firefox13: --- → +

Comment 11

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

changeset:   83358:93b804e5f3f5
user:        Scott Johnson <sjohnson@mozilla.com>
date:        Sun Dec 25 23:25:59 2011 -0600
summary:     Bug 695222 - Implement column-fill part of CSS3 multicol spec. r=roc,dbaron
Assignee: dbaron → sjohnson
(Assignee)

Comment 12

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

changeset:   77842:731bbf32a6fd
user:        Ehsan Akhgari <ehsan@mozilla.com>
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.
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.
(Assignee)

Comment 14

6 years ago
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?
Yes.
(Assignee)

Comment 16

6 years ago
Created attachment 595074 [details] [diff] [review]
b718516 - Removal of FinishReflowWithAbsoluteFrames
Attachment #595074 - Flags: review?(ehsan)
Comment on attachment 595074 [details] [diff] [review]
b718516 - Removal of FinishReflowWithAbsoluteFrames

Please fix the indentation, r=me with that!
Attachment #595074 - Flags: review?(ehsan) → review+
(Assignee)

Comment 18

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

Updated

6 years ago
Target Milestone: --- → mozilla13
(Assignee)

Comment 19

6 years ago
Created attachment 595223 [details] [diff] [review]
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.
Attachment #595074 - Attachment is obsolete: true
Attachment #595223 - Flags: approval-mozilla-beta?
Attachment #595223 - Flags: approval-mozilla-aurora?
(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.
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
Target Milestone: mozilla13 → ---
(Assignee)

Comment 22

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

Comment 23

6 years ago
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?
Target Milestone: --- → mozilla13
(Assignee)

Updated

6 years ago
Target Milestone: mozilla13 → ---
(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.
(Assignee)

Comment 25

6 years ago
Created attachment 595797 [details] [diff] [review]
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.
Attachment #595223 - Attachment is obsolete: true
Attachment #595223 - Flags: approval-mozilla-beta?
Attachment #595223 - Flags: approval-mozilla-aurora?
Attachment #595797 - Flags: review?(ehsan)
Comment on attachment 595797 [details] [diff] [review]
b718516-2 (Without crashtest, with reftests disabled)

Let's go with this.
Attachment #595797 - Flags: review?(ehsan) → review+
(Assignee)

Comment 27

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

Comment 28

6 years ago
Created attachment 596065 [details] [diff] [review]
b718516-2 (Without crashtest, with only failing reftests disabled)

Re-enabled reftests that were passing.
Attachment #595797 - Attachment is obsolete: true
Attachment #596065 - Flags: review+
(Assignee)

Comment 29

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

Updated

6 years ago
Whiteboard: [sg:critical] → [sg:critical] [don't close after pushing to m-c]
(Assignee)

Comment 30

6 years ago
Created attachment 596072 [details] [diff] [review]
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.
(Assignee)

Updated

6 years ago
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/053d3cc103cb
status-firefox13: affected → fixed
(Assignee)

Comment 32

6 years ago
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.
Attachment #596065 - Flags: approval-mozilla-release?
Attachment #596065 - Flags: approval-mozilla-beta?
Attachment #596065 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

6 years ago
Keywords: regressionwindow-wanted
(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.
tracking-firefox-esr10: ? → 10+

Updated

6 years ago
Attachment #596065 - Flags: approval-mozilla-release?
Attachment #596065 - Flags: approval-mozilla-release-
Attachment #596065 - Flags: approval-mozilla-beta?
Attachment #596065 - Flags: approval-mozilla-beta+
Attachment #596065 - Flags: approval-mozilla-aurora?
Attachment #596065 - Flags: approval-mozilla-aurora+

Updated

6 years ago
tracking-firefox-esr10: 10+ → 11+
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
(Assignee)

Comment 35

6 years ago
Pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/ff20f55bc31b
(Assignee)

Comment 36

6 years ago
Pushed to beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/8921a6099974
(Assignee)

Comment 37

6 years ago
Pushed to esr10:
https://hg.mozilla.org/releases/mozilla-esr10/rev/726e2c223958
(Assignee)

Comment 38

6 years ago
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...
Attachment #596072 - Flags: review?(matspal)
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.
Attachment #596072 - Flags: review?(matspal) → review+
(Assignee)

Comment 40

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

6 years ago
Since this was pushed to aurora/beta without the crash test, can we mark as fixed for 11/12?
(Assignee)

Comment 42

6 years ago
(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.
status-firefox-esr10: affected → fixed
status-firefox11: affected → fixed
status-firefox12: affected → fixed
As expected, this doesn't crash in 1.9.2.27 on Windows XP SP3.
status1.9.2: --- → unaffected
Blocks: 656130
Flags: in-testsuite?
Blocks: 733640
The bug described here is fixed, closing bug. "Check in tests" has been moved to bug 733640.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox10: affected → wontfix
Resolution: --- → FIXED
Whiteboard: [sg:critical] [don't close after pushing to m-c] → [sg:critical] [test check-in moved to bug 733640]
Verified fixed in
 * Firefox 13.0a1 Nightly
 * Firefox 12.0a2 Aurora
 * Firefox 11.0b6
 * Firefox 10.0.3esr
using attached testcase
Status: RESOLVED → VERIFIED
status-firefox-esr10: fixed → verified
status-firefox11: fixed → verified
status-firefox12: fixed → verified
status-firefox13: fixed → verified
Whiteboard: [sg:critical] [test check-in moved to bug 733640] → [sg:critical][test check-in moved to bug 733640][qa!]

Updated

5 years ago
Depends on: 736072
(Assignee)

Comment 46

5 years ago
Since the release has been completed, am I ok to check in the tests?
Group: core-security
Depends on: 724978
Depends on: 839871
Crash tests landed in bug 733640.
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.