[reflow branch] Crash [@ PresShell::ProcessReflowCommands] with unminimised testcase that uses a lot of iframes

VERIFIED FIXED

Status

()

Core
Layout
--
critical
VERIFIED FIXED
12 years ago
7 years ago

People

(Reporter: Martijn Wargers (zombie), Assigned: mats)

Tracking

({crash, regression})

Trunk
crash, regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: post 1.8-branch, crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

12 years ago
Created attachment 250853 [details]
testcase

See testcase. Download the testcase to your computer, unzip it, then open the file named 'iframecrash1.htm'.
I usually crash within 10 seconds after opening. If it hasn't crashed after 30 seconds, try a reload.

I'm pretty sure this is a regression, I'm not seeing this with the latest 1.8.x build.
I've tried to come up with a regression range, and I think (not completely sure, though) it regressed between 2006-12-07 and 2006-12-08:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-01-07+04&maxdate=2008-01-08+06&cvsroot=%2Fcvsroot

Talkback ID: TB28146788W
0x00000000
PresShell::ProcessReflowCommands  [mozilla\layout\base\nspresshell.cpp, line 6179]
PresShell::FlushPendingNotifications  [mozilla\layout\base\nspresshell.cpp, line 4799]
nsDocument::FlushPendingNotifications  [mozilla\content\base\src\nsdocument.cpp, line 4424]
nsHTMLDocument::FlushPendingNotifications  [mozilla\content\html\document\src\nshtmldocument.cpp, line 1296]
nsDocShell::GetPositionAndSize  [mozilla\docshell\base\nsdocshell.cpp, line 3564]
nsDocShell::SetupNewViewer  [mozilla\docshell\base\nsdocshell.cpp, line 6019]

If desired, I can try and come up with a more minimal testcase (which will probably be a painful process, because it doesn't seem to crash in a consistent way).

I'm filing this as security sensitive for the moment, because the testcase uses all kinds of 'evil' code.
(Assignee)

Comment 1

12 years ago
Patch coming up...
Assignee: nobody → mats.palmgren
OS: Windows XP → All
Hardware: PC → All
(Assignee)

Comment 2

12 years ago
When we hit the "wasDirty lied" assertion we will append the frame again
so we have two or more entries in mDirtyRoots for 'f'. If the frame is
destroyed while there is more than one entry we will have entries for
the destroyed frame in mDirtyRoots because PresShell::NotifyDestroyingFrame
only removes one entry.
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsPresShell.cpp&rev=3.954&root=/cvsroot&mark=3488-3491#3479
Blocks: 300030
Summary: Crash [@ PresShell::ProcessReflowCommands] with unminimised testcase that uses a lot of iframes → [reflow branch] Crash [@ PresShell::ProcessReflowCommands] with unminimised testcase that uses a lot of iframes
(Assignee)

Comment 3

12 years ago
Created attachment 250903 [details] [diff] [review]
Patch rev. 1

This patch assumes we only want one entry per frame in mDirtyRoots.
An alternative would be to allow that and make NotifyDestroyingFrame
handle multiple entries instead.
Attachment #250903 - Flags: superreview?(dbaron)
Attachment #250903 - Flags: review?(dbaron)
I suppose we should probably do this; I think the main cause of this assertion is some stuff the scrollbars do in the middle of reflow.

That said, you don't need the IndexOf call; just do the RemoveElement loop, and put the assertion in the loop body.
(Assignee)

Comment 5

12 years ago
Created attachment 254531 [details] [diff] [review]
Patch rev. 2
Attachment #250903 - Attachment is obsolete: true
Attachment #254531 - Flags: superreview?(dbaron)
Attachment #254531 - Flags: review?(dbaron)
Attachment #250903 - Flags: superreview?(dbaron)
Attachment #250903 - Flags: review?(dbaron)
Comment on attachment 254531 [details] [diff] [review]
Patch rev. 2

Probably nice to have braces around the loop body, in case somebody makes the macro not-quite-well-behaved.  r+sr=dbaron
Attachment #254531 - Flags: superreview?(dbaron)
Attachment #254531 - Flags: superreview+
Attachment #254531 - Flags: review?(dbaron)
Attachment #254531 - Flags: review+
(Assignee)

Comment 7

12 years ago
Nit fixed.
Checked in to trunk at 2007-02-22 18:29 PST.

-> FIXED
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Reporter)

Comment 8

12 years ago
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070228 Minefield/3.0a3pre
(the testcase tends to hang Firefox for quite a time, though)
Status: RESOLVED → VERIFIED
Verified not to affect the 1.8 branch (Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.4pre) Gecko/20070322 BonEcho/2.0.0.4pre).
Whiteboard: post 1.8-branch
Group: security

Updated

11 years ago
Flags: in-testsuite?
Crash Signature: [@ PresShell::ProcessReflowCommands]
You need to log in before you can comment on or make changes to this bug.