Closed Bug 362708 Opened 18 years ago Closed 18 years ago

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

Categories

(Core :: Layout, defect)

1.8 Branch
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: martijn.martijn, Assigned: MatsPalmgren_bugz)

References

Details

(Keywords: crash, verified1.8.0.12, verified1.8.1.2, Whiteboard: [sg:moderate?])

Crash Data

Attachments

(5 files, 1 obsolete file)

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)
Attached file unminimised testcase
#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
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.
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?
(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.

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.
Whiteboard: [sg:moderate?]
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.
Assignee: nobody → bernd_mozilla
Attached file Frame dump
Attached patch Patch rev. 1 (obsolete) — Splinter Review
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.
Attachment #247771 - Flags: review?(bernd_mozilla)
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);

Ah, indeed it is.  Assume it says so for now, I'll update the patch after
you have more comments...
Comment on attachment 247771 [details] [diff] [review]
Patch rev. 1

the reminder looks good
Attachment #247771 - Flags: review?(bernd_mozilla) → review+
Assignee: bernd_mozilla → mats.palmgren
OS: Windows XP → All
Hardware: PC → All
Attached patch Patch rev. 2Splinter Review
Addressing Bernd's comment 11
Attachment #247784 - Flags: superreview?(roc)
Attachment #247784 - Flags: superreview?(roc) → superreview+
Checked in to trunk at 2006-12-07 19:44 PST

-> FIXED
Status: NEW → RESOLVED
Closed: 18 years ago
Flags: blocking1.8.1.2?
Resolution: --- → FIXED
Attachment #247784 - Flags: approval1.8.1.2?
Attachment #247771 - Attachment is obsolete: true
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.
Status: RESOLVED → VERIFIED
Blocks: 362707
Depends on: 363726
Depends on: 363729
No longer depends on: 363729
Comment on attachment 247784 [details] [diff] [review]
Patch rev. 2

Approved for 1.8 branch, a=jay for drivers.
Attachment #247784 - Flags: approval1.8.1.2? → approval1.8.1.2+
Flags: blocking1.8.1.2? → blocking1.8.1.2-
Checked in to MOZILLA_1_8_BRANCH at 2006-12-27 19:15 PST
Keywords: fixed1.8.1.2
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.
These testcases crash FF1.5.0.10 also.
Flags: wanted1.8.0.x+
Flags: blocking1.8.0.11?
Whiteboard: [sg:moderate?] → [sg:moderate?] wait for 1.8.0 branch fix
Mats: do we need a 1.8.0-specific patch, or does this one apply?
Flags: blocking1.8.0.12? → blocking1.8.0.12+
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).
Attachment #247784 - Flags: approval1.8.0.11?
Attachment #247784 - Flags: approval1.8.0.11? → approval1.8.0.12?
Comment on attachment 247784 [details] [diff] [review]
Patch rev. 2

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #247784 - Flags: approval1.8.0.12? → approval1.8.0.12+
Checked in to MOZILLA_1_8_0_BRANCH at 2007-03-23 02:31 PST.
Keywords: fixed1.8.0.12
Whiteboard: [sg:moderate?] wait for 1.8.0 branch fix → [sg:moderate?]
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
Group: security
Flags: in-testsuite?
Crash Signature: [@ nsGfxScrollFrameInner::GetActualScrollbarSizes]
You need to log in before you can comment on or make changes to this bug.