Closed
Bug 718516
Opened 12 years ago
Closed 12 years ago
crash @xul!nsOverflowContinuationTracker::SetUpListWalker
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
VERIFIED
FIXED
mozilla13
People
(Reporter: nils, Assigned: jwir3)
References
Details
(Keywords: regression, reproducible, testcase, Whiteboard: [sg:critical][test check-in moved to bug 733640][qa!])
Crash Data
Attachments
(4 files, 3 obsolete files)
1.71 KB,
text/html
|
Details | |
12.13 KB,
text/html
|
Details | |
3.78 KB,
patch
|
jwir3
:
review+
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
akeybl
:
approval-mozilla-release-
|
Details | Diff | Splinter Review |
3.93 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•12 years ago
|
Attachment #589002 -
Attachment mime type: text/plain → text/html
Comment 1•12 years ago
|
||
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
Comment 2•12 years ago
|
||
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]
Updated•12 years ago
|
status-firefox10:
--- → affected
Comment 3•12 years ago
|
||
abillings crashed in beta as well (Fx10)
Comment 4•12 years ago
|
||
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
Comment 5•12 years ago
|
||
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:
+ → ---
Updated•12 years ago
|
status-firefox10:
--- → affected
status-firefox11:
--- → affected
status-firefox12:
--- → affected
tracking-firefox10:
--- → -
tracking-firefox11:
--- → +
tracking-firefox12:
--- → +
Comment 6•12 years ago
|
||
###!!! 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•12 years ago
|
||
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•12 years ago
|
||
:mats -- since you've done some investigation already, should we assign this to you?
Comment 9•12 years ago
|
||
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•12 years ago
|
||
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•12 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•12 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.
Comment 13•12 years ago
|
||
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•12 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•12 years ago
|
||
Attachment #595074 -
Flags: review?(ehsan)
Comment 17•12 years ago
|
||
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•12 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•12 years ago
|
Target Milestone: --- → mozilla13
Assignee | ||
Comment 19•12 years ago
|
||
[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?
Comment 20•12 years ago
|
||
(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•12 years ago
|
||
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•12 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•12 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•12 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•12 years ago
|
||
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 26•12 years ago
|
||
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•12 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•12 years ago
|
||
Re-enabled reftests that were passing.
Attachment #595797 -
Attachment is obsolete: true
Attachment #596065 -
Flags: review+
Assignee | ||
Comment 29•12 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•12 years ago
|
Whiteboard: [sg:critical] → [sg:critical] [don't close after pushing to m-c]
Assignee | ||
Comment 30•12 years ago
|
||
Don't push this to inbound or central until all releases have been updated with a fix for this bug.
Assignee | ||
Updated•12 years ago
|
Target Milestone: --- → mozilla13
Assignee | ||
Comment 32•12 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•12 years ago
|
Keywords: regressionwindow-wanted
Comment 33•12 years ago
|
||
(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.
Updated•12 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•12 years ago
|
Comment 34•12 years ago
|
||
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•12 years ago
|
||
Pushed to aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/ff20f55bc31b
Assignee | ||
Comment 36•12 years ago
|
||
Pushed to beta: https://hg.mozilla.org/releases/mozilla-beta/rev/8921a6099974
Assignee | ||
Comment 37•12 years ago
|
||
Pushed to esr10: https://hg.mozilla.org/releases/mozilla-esr10/rev/726e2c223958
Assignee | ||
Comment 38•12 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 39•12 years ago
|
||
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•12 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•12 years ago
|
||
Since this was pushed to aurora/beta without the crash test, can we mark as fixed for 11/12?
Assignee | ||
Comment 42•12 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.
Comment 43•12 years ago
|
||
As expected, this doesn't crash in 1.9.2.27 on Windows XP SP3.
status1.9.2:
--- → unaffected
Comment 44•12 years ago
|
||
The bug described here is fixed, closing bug. "Check in tests" has been moved to bug 733640.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical] [don't close after pushing to m-c] → [sg:critical] [test check-in moved to bug 733640]
Comment 45•12 years ago
|
||
Verified fixed in * Firefox 13.0a1 Nightly * Firefox 12.0a2 Aurora * Firefox 11.0b6 * Firefox 10.0.3esr using attached testcase
Status: RESOLVED → VERIFIED
Whiteboard: [sg:critical] [test check-in moved to bug 733640] → [sg:critical][test check-in moved to bug 733640][qa!]
Assignee | ||
Comment 46•12 years ago
|
||
Since the release has been completed, am I ok to check in the tests?
Updated•12 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•