362708 - Crash [@ nsGfxScrollFrameInner::GetActualScrollbarSizes] on print preview with branch builds with unminimised testcase

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

I'm attaching unminimised testcases here, these also crash on branch builds on print preview.
I filed bug 362707 for a case that only crashes trunk builds on print preview.
So I can minimise this unminised testcase for branch builds, if desired.

(filing it as security sensitive, just to be sure)

Comment 1

[Martijn Wargers (dead)]

Attachment: unminimised testcase

Comment 2

[Martijn Wargers (dead)]

Attachment: Partly minimised testcase

Comment 3

[Martijn Wargers (dead)]

Attachment: backtrace from a debug 1.8.1 branch build

#0  0x0577f1ee in nsGfxScrollFrameInner::GetActualScrollbarSizes (this=0x4c)
    at c:/mozilla181/mozilla/layout/generic/nsGfxScrollFrame.cpp:2511
#1  0x05c570bc in nsHTMLScrollFrame::GetActualScrollbarSizes (this=0x0)
    at c:/mozilla181/mozilla/layout/forms/../generic/nsGfxScrollFrame.h:294
#2  0x058b10c5 in nsTableRowGroupFrame::GetFirstRow (this=0x12277ce0)
    at c:/mozilla181/mozilla/layout/tables/nsTableRowGroupFrame.cpp:454
#3  0x058b059a in nsTableRowGroupFrame::InitRepeatedFrame (this=0x1227ece0,
    aPresContext=0x121aa678, aHeaderFooterFrame=0x12277ce0)
    at c:/mozilla181/mozilla/layout/tables/nsTableRowGroupFrame.cpp:173
etc

Comment 4

[Martijn Wargers (dead)]

Oh, apparently the partly minimised testcase also crashes the latest reflow branch build (I thought it didn't).

So the testcase I attached at bug 362707 was minimised using a regular trunk build, which is why it is now only crashing on print preview with regular trunk builds.

Comment 5

[Bernd]

I can reproduce the crash with the partly minimized testcase in my debug build, this is good.
I see a bunch of 
###!!! ASSERTION: Allowed only one anonymous view between frames: 'ancestorView
== view->GetParent()->GetParent()', file d:/moz_src/mozilla/layout/generic/nsCon
tainerFrame.cpp, line 272

before. Is it possible to reduce the testcase so that the assert fires?

Comment 6

[Martijn Wargers (dead)]

(In reply to comment #5)
> before. Is it possible to reduce the testcase so that the assert fires?

I don't get that assertion at all (not with branch or trunk) on print preview, I seem to crash directly.

Comment 7

[Daniel Veditz [:dveditz]]

This is definitely a potential security problem. Windows shut me down with a DEP violation, and the debugger shows that in nsTableRowGroupFrame::GetFirstRow the pointer to GetFirstFrame() is munged and we try to execute someone else's stuff.

The requirement to open Print Preview--not a normal, habitual browsing action--lowers this from critical to moderate. But given that the print-specific stuff is pretty removed in the stack there may be a way to construct a similarly broken frameset using print-like styles in a plain webpage. That would be critical.

Comment 8

[Bernd]

The table code has pretty strong pagination protection, thats the reason we see these crashes so late in the game. I am not sure how many crash path's are still open, the test cases in bug 362707 and bug 362724 look suspiciously similar, so my first guess is that they both identical and indicate a table view mishandling during pagination. My plan currently is to improve further the pagination protection at bug 362275. The next bug would be bug 362724 and then bug 362707. (thats the dream of the silver bullet). Once those are handled and this is still open a minimized test case with page-break properties would be cool.

Comment 9

[Mats Palmgren (inactive)]

Attachment: Frame dump

Comment 10

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

The frame constructor assumes that a table frame can only contain rowgroup
frames, but it can also be a rowgroup frame wrapped in a scroll frame.
This patch fixes this bug and bug 362707.
It doesn't fix bug 362724 - that crash looks a bit different.

Comment 11

[Bernd]

Just a quick question:

+      nsTableRowGroupFrame* rowGroupFrame = nsnull;
+      if (childFrame->GetType() == nsLayoutAtoms::tableRowGroupFrame) {
+        rowGroupFrame = NS_STATIC_CAST(nsTableRowGroupFrame*, childFrame);
+      }
+      else if (childFrame->GetType() == nsLayoutAtoms::scrollFrame) {
+        nsIFrame* firstChild = childFrame->GetFirstChild(nsnull);
+        rowGroupFrame = firstChild &&
+          firstChild->GetType() == nsLayoutAtoms::tableRowGroupFrame ?
+            NS_STATIC_CAST(nsTableRowGroupFrame*, firstChild) : nsnull;
+      }


Isn't this code equivalent to 

nsTableRowGroupFrame* rowGroupFrame = nsTableFrame::GetRowGroupFrame(childFrame);

Comment 12

[Mats Palmgren (inactive)]

Ah, indeed it is.  Assume it says so for now, I'll update the patch after
you have more comments...

Comment 13

[Bernd]

Comment on attachment 247771 [details] [diff] [review]
Patch rev. 1

the reminder looks good

Comment 14

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Addressing Bernd's comment 11

Comment 15

[Mats Palmgren (inactive)]

Checked in to trunk at 2006-12-07 19:44 PST

-> FIXED

Comment 16

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061208 Minefield/3.0a1
Both testcases don't crash anymore on print preview.
The Partly minimised testcase crashes Mozilla when closing print preview, but that's because the reflow branch has landed and has been filed as bug 362210.

Comment 17

[Jay Patel [:jay]]

Comment on attachment 247784 [details] [diff] [review]
Patch rev. 2

Approved for 1.8 branch, a=jay for drivers.

Comment 18

[Mats Palmgren (inactive)]

Checked in to MOZILLA_1_8_BRANCH at 2006-12-27 19:15 PST

Comment 19

[Marcia Knous [:marcia]]

verified fixed on the 1.8 branch using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2pre) Gecko/20070111 BonEcho/2.0.0.2pre. Test cases do not crash during print preview. Adding verified keyword for the 1.8 branch.

Comment 20

[Daniel Veditz [:dveditz]]

These testcases crash FF1.5.0.10 also.

Comment 21

[Daniel Veditz [:dveditz]]

Mats: do we need a 1.8.0-specific patch, or does this one apply?

Comment 22

[Mats Palmgren (inactive)]

Comment on attachment 247784 [details] [diff] [review]
Patch rev. 2

(In reply to comment #21)
> Mats: do we need a 1.8.0-specific patch, or does this one apply?

My local 1.8 branch patch works fine also on 1.8.0, tested
in my local 1.8.0 tree and it fixes the crash.
(there's a trivial difference to the trunk patch because the Init()
and SetInitialChildList() param lists have changed since, but
I didn't bother asking for new review).

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 247784 [details] [diff] [review]
Patch rev. 2

approved for 1.8.0 branch, a=dveditz for drivers

Comment 24

[Mats Palmgren (inactive)]

Checked in to MOZILLA_1_8_0_BRANCH at 2007-03-23 02:31 PST.

Comment 25

[Jay Patel [:jay]]

v.fixed on 1.8.0 branch with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.12pre) Gecko/20070426 Firefox/1.5.0.12pre