337419 - [FIX] Crash with float, columns, and a feed [@ nsFontMetricsMac::Init] [@ nsUnicodeFontMappingMac::GetFontID] [@ nsIFrame::AddStateBits] [@ nsTextStyle::nsTextStyle]

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

This testcase causes a crash with a varying signature and sometimes with a random address on top.

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Jesse Ruderman]

I'm curious why it matters that the page has a feed.

Comment 3

[Jesse Ruderman]

Still crashes a trunk nightly on Mac, Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060602 Minefield/3.0a1.

Does not crash on Windows.

Comment 4

[Jesse Ruderman]

Attachment: A stack trace with nsTextStyle::nsTextStyle on top

Comment 5

[Jesse Ruderman]

Also crashes on Linux, at least using a build modified for use with Valgrind.  I'll attach a Valgrind log shortly.

Comment 6

[Jesse Ruderman]

Attachment: Valgrind log

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This looks like a bit of a mess -- CreateContinuingFrame often doesn't set its out parameter when it returns failure, but callers check the otherwise uninitialized out parameter rather than the nsresult.

It's also worth figuring out why it's failing, though.

Comment 8

[Jesse Ruderman]

Still crashes on trunk, despite recent fixes for column-related crashes.

Comment 9

[Jesse Ruderman]

Now it crashes even without the feed, and fires this assertion shortly before crashing:

###!!! ASSERTION: unexpected frame type: 'PR_FALSE', file /Users/admin/trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 11152

Comment 10

[Jesse Ruderman]

CCing Mats, who fixed bug 348688, in the hope that he'll be interested in fixing this float-related crash as well.

Comment 11

[Mats Palmgren (inactive)]

Attachment: Patch A, rev. 1

1. Make all CreateContinuingFrame() callers check the returned error code.
This patch fixes the crash but the float is not rendered.
I think it's a safe fix for branches.

The reason we fail to render the float is that the out-fo-flow is a
nsHTMLScrollFrame which we can't create a continuation for.
So with this patch we still get these two assertions:
###!!! ASSERTION: unexpected frame type: 'Not Reached', file nsCSSFrameConstructor.cpp, line 11185
###!!! ASSERTION: reflow dirty lines failed: 'NS_SUCCEEDED(rv)', file nsBlockFrame.cpp, line 916

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsBlockReflowState.cpp&rev=3.514&root=/cvsroot&mark=654-661#623
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsBlockFrame.cpp&rev=3.787&root=/cvsroot&mark=4231,4240,4242-4243#4230
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsBlockFrame.cpp&rev=3.787&root=/cvsroot&mark=4318,4322#4317
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsHTMLContainerFrame.cpp&rev=3.217&root=/cvsroot&mark=338,351-352#337
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1251&root=/cvsroot&mark=11119-11132,11152-11153#11118

Comment 12

[Mats Palmgren (inactive)]

Attachment: Patch B, rev. 1

1. Make scroll frames non-splittable.
2. Make a placeholder splittable only if the out-of-flow is.
3. If a float placeholder is not splittable then place it immediately
   instead of failing to reflow it because it can't be split.

This patch makes us render the testcase without any assertions
(together with Patch A), but is probably more risky.


BTW, maybe we should change:
  NS_IMETHOD IsSplittable(nsSplittableType& aIsSplittable) const;
to
  nsSplittableType GetSplittableType() const;

It's only used within layout/ and always return NS_OK as far as I can see.

Comment 13

[Boris Zbarsky [:bzbarsky]]

roc should probably just review all of these...

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 234872 [details] [diff] [review]
Patch A, rev. 1

+  nsresult rv = aPresContext.PresShell()->FrameConstructor()->
     CreateContinuingFrame(&aPresContext, &aRowFrame, this, aContRowFrame);
-  if (!*aContRowFrame) return;
+  if (NS_FAILED(rv)) {
+    return;
+  }

Set *aContRowFrame to nsnull before this return, and check callers to make sure they check that.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 234873 [details] [diff] [review]
Patch B, rev. 1

looks good

Comment 16

[Mats Palmgren (inactive)]

(In reply to comment #14)
> Set *aContRowFrame to nsnull before this return, and check callers
> to make sure they check that.

Good point.  Patch A with the additional null assignment checked in
to trunk at 2006-08-21 18:33 PDT. (I'll hold Patch B for 24h to get nightlies)

Comment 17

[Mats Palmgren (inactive)]

"Patch B" checked in to trunk at 2006-08-23 22:22 PDT.

Filed bug 349973 on changing IsSplittable() signature.

-> FIXED

Comment 18

[Mike Schroepfer]

Comment on attachment 234872 [details] [diff] [review]
Patch A, rev. 1

 a=schrep for drivers

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 234872 [details] [diff] [review]
Patch A, rev. 1

approved for 1.8.0 branch, a=dveditz for drivers

Comment 20

[Mats Palmgren (inactive)]

Patch A: checked into to MOZILLA_1_8_BRANCH at 2006-08-25 13:21 PDT.

Comment 21

[Mats Palmgren (inactive)]

Patch A: checked in to MOZILLA_1_8_0_BRANCH at 2006-08-25 15:36 PDT.

Comment 22

[Jay Patel [:jay]]

v.fixed on both 1.8.0 and 1.8.1 branches with:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.7) Gecko/20060906 Firefox/1.5.0.7
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1b2) Gecko/20060907 BonEcho/2.0b2

Comment 23

[Jesse Ruderman]

Crashtest checked in.