Closed Bug 306911 Opened 19 years ago Closed 19 years ago

Infinite recursion crash with nested <xul:listitem>s [@ nsCOMPtr_base::assign_from_qi] [@nsGrid::GetScrollBox]

Categories

(Core :: XUL, defect)

PowerPC
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: dbaron)

References

Details

(Keywords: crash, testcase, Whiteboard: [sg:dos][patch] caused 317480)

Crash Data

Attachments

(6 files, 1 obsolete file)

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050901
Firefox/1.6a1

The testcase crashes while the status bar counter shows "21".

Filing as security-sensitive because the unsimplified testcase contains code
from bug 306663.
Attached file testcase (not reduced) (obsolete) —
Blocks: stirdom
This crash happens on the Gecko 1.8 branch too.
Hmm, my Windows MSVC build got to 38 and hung, while my Linux GTK1 build got to
46 and crashed. Grid code sucks :-(
Attached file reduced testcase
This crash happens due to infinite recursion among these 5 functions:

nsGridRowLeafLayout::GetMaxSize(nsIFrame*, nsBoxLayoutState&, nsSize&) + 136
nsBoxFrame::GetMaxSize(nsBoxLayoutState&, nsSize&) + 204
nsGridCell::GetMaxSize(nsBoxLayoutState&, nsSize&) + 160
nsGrid::GetMaxRowHeight(nsBoxLayoutState&, int, int&, int) + 460
nsGrid::GetMaxRowSize(nsBoxLayoutState&, int, nsSize&, int) + 88
nsGridRowLeafLayout::GetMaxSize(nsIFrame*, nsBoxLayoutState&, nsSize&) + 136
...
Keywords: testcase
Summary: StirDOM/XUL crash [@ nsCOMPtr_base::assign_from_qi] [@nsGrid::GetScrollBox] → Infinite recursion crash with nested <xul:listitem>s [@ nsCOMPtr_base::assign_from_qi] [@nsGrid::GetScrollBox]
This patch should make the eventual fix easier to understand.  (I think there are multiple ways to fix this and we should do both of them -- both making the grid code a little more careful about what it considers the parent, and making the frame construction code a little more careful, somehow.)
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Attachment #203463 - Flags: superreview?(bzbarsky)
Attachment #203463 - Flags: review?(bzbarsky)
Comment on attachment 203463 [details] [diff] [review]
comments and a little code cleanup (checked in)

r+sr=bzbarsky
Attachment #203463 - Flags: superreview?(bzbarsky)
Attachment #203463 - Flags: superreview+
Attachment #203463 - Flags: review?(bzbarsky)
Attachment #203463 - Flags: review+
Comment on attachment 203463 [details] [diff] [review]
comments and a little code cleanup (checked in)

Checked in to trunk.
Attachment #203463 - Attachment description: comments and a little code cleanup → comments and a little code cleanup (checked in)
This makes GetParentGridPart only return an appropriate parent, i.e., one that is part of a correctly nested grid structure.  In theory this should mean that we treat incorrectly nested grid structures as independent grid-like things, i.e., treat them just as they would be treated if they weren't anywhere near other grid parts.
Attachment #203893 - Flags: superreview?(bzbarsky)
Attachment #203893 - Flags: review?(bzbarsky)
Attachment #203893 - Flags: superreview?(bzbarsky)
Attachment #203893 - Flags: superreview-
Attachment #203893 - Flags: review?(bzbarsky)
Attachment #203893 - Flags: review-
Attachment #203893 - Attachment is obsolete: true
Attachment #203894 - Flags: superreview?(bzbarsky)
Attachment #203894 - Flags: review?(bzbarsky)
Attachment #203894 - Flags: superreview?(bzbarsky)
Attachment #203894 - Flags: superreview+
Attachment #203894 - Flags: review?(bzbarsky)
Attachment #203894 - Flags: review+
Fix checked in to trunk.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
This patch broke the mail compose window. See Bug 317480 for more details.
Depends on: 317480
Attached file additional testcases
Here are two additional testcases:  note that this patch changed the behavior for the first and means that we do render content when the grid group is omitted; I'm not sure if that's a good thing or a bad thing, though.
Flags: testcase+
It's OK to leave this out of the 1.8 branches? If we need it please use the blocking/approval flags to nominate for those releases. If we do need it, would we need to worry about the change to nsIGridPart, or is that internal enough that the API change is branch-safe?
Whiteboard: [patch] → [sg:nse][patch] reveals 306663, caused 317480
*** Bug 359786 has been marked as a duplicate of this bug. ***
dbaron, any thoughs on what to do for the 1.8 branches for this?  sounds like we would need to pick up this fix and the fix for 317480 if it went on the branch.
Flags: wanted1.8.1.x?
Flags: wanted1.8.0.x?
Flags: in-testsuite+ → in-testsuite?
This crash has been reported independently at bug 388304
Flags: wanted1.8.1.x? → wanted1.8.1.x+
Blocks: 388304
Crashtest is in.
Flags: in-testsuite? → in-testsuite+
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
Group: core-security
Whiteboard: [sg:nse][patch] reveals 306663, caused 317480 → [sg:dos][patch] caused 317480
Crash Signature: [@ nsCOMPtr_base::assign_from_qi] [@nsGrid::GetScrollBox]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: