Last Comment Bug 718516 - crash @xul!nsOverflowContinuationTracker::SetUpListWalker
: crash @xul!nsOverflowContinuationTracker::SetUpListWalker
Status: VERIFIED FIXED
[sg:critical][test check-in moved to ...
: regression, reproducible, testcase
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla13
Assigned To: Scott Johnson (:jwir3)
:
:
Mentors:
Depends on: 724978 736072 839871
Blocks: 656130 733640
  Show dependency treegraph
 
Reported: 2012-01-16 14:12 PST by Nils
Modified: 2013-05-13 13:39 PDT (History)
14 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wontfix
+
verified
+
verified
+
verified
11+
verified
unaffected


Attachments
testcase (crashes trunk) (1.71 KB, text/html)
2012-01-16 14:12 PST, Nils
no flags Details
stack + frame dump for the asserion (12.13 KB, text/html)
2012-01-24 07:38 PST, Mats Palmgren (:mats)
no flags Details
b718516 - Removal of FinishReflowWithAbsoluteFrames (1.42 KB, patch)
2012-02-07 10:06 PST, Scott Johnson (:jwir3)
ehsan: review+
Details | Diff | Splinter Review
b718516 (with crashtest) (1.43 KB, patch)
2012-02-07 15:34 PST, Scott Johnson (:jwir3)
no flags Details | Diff | Splinter Review
b718516-2 (Without crashtest, with reftests disabled) (4.00 KB, patch)
2012-02-09 10:10 PST, Scott Johnson (:jwir3)
ehsan: review+
Details | Diff | Splinter Review
b718516-2 (Without crashtest, with only failing reftests disabled) (3.78 KB, patch)
2012-02-10 09:10 PST, Scott Johnson (:jwir3)
jaywir3: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑release-
Details | Diff | Splinter Review
b718516-test: Crashtest for this bug. [Don't push to inbound yet] (3.93 KB, patch)
2012-02-10 09:28 PST, Scott Johnson (:jwir3)
mats: review+
Details | Diff | Splinter Review

Description Nils 2012-01-16 14:12:12 PST
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.
Comment 1 Ludovic Hirlimann [:Usul] 2012-01-17 00:10:23 PST
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] 2012-01-18 16:50:08 PST
Jet: please find someone on the layout team to investigate this security bug.
Comment 3 Daniel Veditz [:dveditz] 2012-01-18 16:51:52 PST
abillings crashed in beta as well (Fx10)
Comment 4 Daniel Veditz [:dveditz] 2012-01-18 16:53:49 PST
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] 2012-01-18 16:54:19 PST
Firefox 10b5 crash report for this: https://crash-stats.mozilla.com/report/index/bp-00adaa18-705c-4587-b9bb-9a4982120119
Comment 6 Mats Palmgren (:mats) 2012-01-24 07:38:17 PST
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
Comment 7 Mats Palmgren (:mats) 2012-01-24 07:52:08 PST
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] 2012-01-26 13:43:38 PST
:mats -- since you've done some investigation already, should we assign this to you?
Comment 9 Mats Palmgren (:mats) 2012-01-26 15:23:12 PST
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] 2012-02-02 13:34:34 PST
David: please find an appropriate owner for this bug in light of comment 9 if it's not you.
Comment 11 Jet Villegas (:jet) 2012-02-02 18:07:51 PST
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
Comment 12 Scott Johnson (:jwir3) 2012-02-03 16:15:21 PST
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.
Comment 13 :Ehsan Akhgari 2012-02-06 12:17:23 PST
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) 2012-02-06 16:11:37 PST
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) 2012-02-06 18:56:30 PST
Yes.
Comment 16 Scott Johnson (:jwir3) 2012-02-07 10:06:53 PST
Created attachment 595074 [details] [diff] [review]
b718516 - Removal of FinishReflowWithAbsoluteFrames
Comment 17 :Ehsan Akhgari 2012-02-07 11:16:11 PST
Comment on attachment 595074 [details] [diff] [review]
b718516 - Removal of FinishReflowWithAbsoluteFrames

Please fix the indentation, r=me with that!
Comment 18 Scott Johnson (:jwir3) 2012-02-07 15:32:52 PST
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) 2012-02-07 15:34:59 PST
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.
Comment 20 Mats Palmgren (:mats) 2012-02-07 15:41:00 PST
(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 22 Scott Johnson (:jwir3) 2012-02-07 19:46:20 PST
(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) 2012-02-08 09:59:36 PST
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) 2012-02-08 13:59:11 PST
(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) 2012-02-09 10:10:53 PST
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.
Comment 26 :Ehsan Akhgari 2012-02-09 19:38:16 PST
Comment on attachment 595797 [details] [diff] [review]
b718516-2 (Without crashtest, with reftests disabled)

Let's go with this.
Comment 27 Scott Johnson (:jwir3) 2012-02-10 08:17:47 PST
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) 2012-02-10 09:10:24 PST
Created attachment 596065 [details] [diff] [review]
b718516-2 (Without crashtest, with only failing reftests disabled)

Re-enabled reftests that were passing.
Comment 29 Scott Johnson (:jwir3) 2012-02-10 09:12:25 PST
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) 2012-02-10 09:28:55 PST
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.
Comment 31 Ed Morley [:emorley] 2012-02-10 19:41:30 PST
https://hg.mozilla.org/mozilla-central/rev/053d3cc103cb
Comment 32 Scott Johnson (:jwir3) 2012-02-11 10:42:18 PST
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] 2012-02-14 17:42:42 PST
(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] 2012-02-16 15:42:09 PST
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) 2012-02-17 10:21:56 PST
Pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/ff20f55bc31b
Comment 36 Scott Johnson (:jwir3) 2012-02-17 10:52:58 PST
Pushed to beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/8921a6099974
Comment 37 Scott Johnson (:jwir3) 2012-02-17 12:20:55 PST
Pushed to esr10:
https://hg.mozilla.org/releases/mozilla-esr10/rev/726e2c223958
Comment 38 Scott Johnson (:jwir3) 2012-02-20 15:44:55 PST
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 (:mats) 2012-02-20 17:17:20 PST
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) 2012-02-20 17:18:11 PST
(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] 2012-02-27 05:37:31 PST
Since this was pushed to aurora/beta without the crash test, can we mark as fixed for 11/12?
Comment 42 Scott Johnson (:jwir3) 2012-02-27 09:09:15 PST
(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] 2012-03-01 17:47:47 PST
As expected, this doesn't crash in 1.9.2.27 on Windows XP SP3.
Comment 44 Daniel Veditz [:dveditz] 2012-03-07 09:42:04 PST
The bug described here is fixed, closing bug. "Check in tests" has been moved to bug 733640.
Comment 45 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-08 13:02:38 PST
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) 2012-03-15 12:35:16 PDT
Since the release has been completed, am I ok to check in the tests?
Comment 47 Mats Palmgren (:mats) 2013-05-13 13:39:30 PDT
Crash tests landed in bug 733640.

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