Last Comment Bug 362708 - Crash [@ nsGfxScrollFrameInner::GetActualScrollbarSizes] on print preview with branch builds with unminimised testcase
: Crash [@ nsGfxScrollFrameInner::GetActualScrollbarSizes] on print preview wit...
Status: VERIFIED FIXED
[sg:moderate?]
: crash, verified1.8.0.12, verified1.8.1.2
Product: Core
Classification: Components
Component: Layout (show other bugs)
: 1.8 Branch
: All All
: -- critical (vote)
: ---
Assigned To: Mats Palmgren (:mats)
:
: Jet Villegas (:jet)
Mentors:
Depends on: 363726
Blocks: 362707
  Show dependency treegraph
 
Reported: 2006-12-04 01:06 PST by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2011-06-13 10:01 PDT (History)
5 users (show)
jaymoz: blocking1.8.1.2-
dveditz: blocking1.8.0.12+
dveditz: wanted1.8.0.x+
dveditz: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
unminimised testcase (19.57 KB, text/html)
2006-12-04 01:07 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Partly minimised testcase (3.17 KB, text/html)
2006-12-04 01:08 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
backtrace from a debug 1.8.1 branch build (12.59 KB, text/plain)
2006-12-04 01:09 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Frame dump (38.52 KB, text/html)
2006-12-06 20:04 PST, Mats Palmgren (:mats)
no flags Details
Patch rev. 1 (4.73 KB, patch)
2006-12-06 20:06 PST, Mats Palmgren (:mats)
bernd_mozilla: review+
Details | Diff | Splinter Review
Patch rev. 2 (4.30 KB, patch)
2006-12-06 22:52 PST, Mats Palmgren (:mats)
roc: superreview+
jaymoz: approval1.8.1.2+
dveditz: approval1.8.0.12+
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2006-12-04 01:06:19 PST
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 [:mwargers] (not working for Mozilla) 2006-12-04 01:07:55 PST
Created attachment 247397 [details]
unminimised testcase
Comment 2 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-12-04 01:08:43 PST
Created attachment 247398 [details]
Partly minimised testcase
Comment 3 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-12-04 01:09:42 PST
Created attachment 247399 [details]
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 [:mwargers] (not working for Mozilla) 2006-12-04 01:18:34 PST
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 2006-12-04 21:58:50 PST
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 [:mwargers] (not working for Mozilla) 2006-12-05 03:24:19 PST
(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] 2006-12-06 10:54:40 PST
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 2006-12-06 11:57:06 PST
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 (:mats) 2006-12-06 20:04:00 PST
Created attachment 247770 [details]
Frame dump
Comment 10 Mats Palmgren (:mats) 2006-12-06 20:06:27 PST
Created attachment 247771 [details] [diff] [review]
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 2006-12-06 21:36:43 PST
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 (:mats) 2006-12-06 21:57:26 PST
Ah, indeed it is.  Assume it says so for now, I'll update the patch after
you have more comments...
Comment 13 Bernd 2006-12-06 22:33:48 PST
Comment on attachment 247771 [details] [diff] [review]
Patch rev. 1

the reminder looks good
Comment 14 Mats Palmgren (:mats) 2006-12-06 22:52:39 PST
Created attachment 247784 [details] [diff] [review]
Patch rev. 2

Addressing Bernd's comment 11
Comment 15 Mats Palmgren (:mats) 2006-12-07 20:43:58 PST
Checked in to trunk at 2006-12-07 19:44 PST

-> FIXED
Comment 16 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-12-08 06:42:42 PST
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] 2006-12-20 15:44:54 PST
Comment on attachment 247784 [details] [diff] [review]
Patch rev. 2

Approved for 1.8 branch, a=jay for drivers.
Comment 18 Mats Palmgren (:mats) 2006-12-27 20:06:37 PST
Checked in to MOZILLA_1_8_BRANCH at 2006-12-27 19:15 PST
Comment 19 Marcia Knous [:marcia - use ni] 2007-01-11 17:47:59 PST
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] 2007-02-22 03:38:50 PST
These testcases crash FF1.5.0.10 also.
Comment 21 Daniel Veditz [:dveditz] 2007-03-14 10:54:46 PDT
Mats: do we need a 1.8.0-specific patch, or does this one apply?
Comment 22 Mats Palmgren (:mats) 2007-03-14 11:19:21 PDT
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] 2007-03-16 14:50:26 PDT
Comment on attachment 247784 [details] [diff] [review]
Patch rev. 2

approved for 1.8.0 branch, a=dveditz for drivers
Comment 24 Mats Palmgren (:mats) 2007-03-23 04:39:12 PDT
Checked in to MOZILLA_1_8_0_BRANCH at 2007-03-23 02:31 PST.
Comment 25 Jay Patel [:jay] 2007-04-26 14:12:27 PDT
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

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