The default bug view has changed. See this FAQ.

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

VERIFIED FIXED

Status

()

Core
Layout
--
critical
VERIFIED FIXED
11 years ago
6 years ago

People

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

Tracking

({crash, verified1.8.0.12, verified1.8.1.2})

1.8 Branch
crash, verified1.8.0.12, verified1.8.1.2
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.2 -
blocking1.8.0.12 +
wanted1.8.0.x +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate?], crash signature)

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

11 years ago
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)
(Reporter)

Comment 1

11 years ago
Created attachment 247397 [details]
unminimised testcase
(Reporter)

Comment 2

11 years ago
Created attachment 247398 [details]
Partly minimised testcase
(Reporter)

Comment 3

11 years ago
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
(Reporter)

Comment 4

11 years ago
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

11 years ago
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?
(Reporter)

Comment 6

11 years ago
(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?]

Comment 8

11 years ago
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
(Assignee)

Comment 9

11 years ago
Created attachment 247770 [details]
Frame dump
(Assignee)

Comment 10

11 years ago
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.
Attachment #247771 - Flags: review?(bernd_mozilla)

Comment 11

11 years ago
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);

(Assignee)

Comment 12

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

Comment 13

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

the reminder looks good
Attachment #247771 - Flags: review?(bernd_mozilla) → review+
(Assignee)

Updated

11 years ago
Assignee: bernd_mozilla → mats.palmgren
OS: Windows XP → All
Hardware: PC → All
(Assignee)

Comment 14

11 years ago
Created attachment 247784 [details] [diff] [review]
Patch rev. 2

Addressing Bernd's comment 11
Attachment #247784 - Flags: superreview?(roc)
Attachment #247784 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 15

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

-> FIXED
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Flags: blocking1.8.1.2?
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Attachment #247784 - Flags: approval1.8.1.2?
(Assignee)

Updated

10 years ago
Attachment #247771 - Attachment is obsolete: true
(Reporter)

Comment 16

10 years ago
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
(Reporter)

Updated

10 years ago
Blocks: 362707
(Reporter)

Updated

10 years ago
Depends on: 363726
(Reporter)

Updated

10 years ago
Depends on: 363729
(Reporter)

Updated

10 years ago
No longer depends on: 363729

Comment 17

10 years ago
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+

Updated

10 years ago
Flags: blocking1.8.1.2? → blocking1.8.1.2-
(Assignee)

Comment 18

10 years ago
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.
Keywords: fixed1.8.1.2 → verified1.8.1.2
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+
(Assignee)

Comment 22

10 years ago
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+
(Assignee)

Comment 24

10 years ago
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?]

Comment 25

10 years ago
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
Keywords: fixed1.8.0.12 → verified1.8.0.12
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.