Last Comment Bug 337419 - [FIX] Crash with float, columns, and a feed [@ nsFontMetricsMac::Init] [@ nsUnicodeFontMappingMac::GetFontID] [@ nsIFrame::AddStateBits] [@ nsTextStyle::nsTextStyle]
: [FIX] Crash with float, columns, and a feed [@ nsFontMetricsMac::Init] [@ nsU...
Status: RESOLVED FIXED
[sg:critical]
: crash, testcase, verified1.8.0.7, verified1.8.1
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on: 350046
Blocks: stirdom
  Show dependency treegraph
 
Reported: 2006-05-10 06:01 PDT by Jesse Ruderman
Modified: 2011-06-13 10:01 PDT (History)
10 users (show)
mtschrep: blocking1.8.1+
dveditz: blocking1.8.0.7+
jruderman: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (321 bytes, text/html)
2006-05-10 06:01 PDT, Jesse Ruderman
no flags Details
A stack trace with nsTextStyle::nsTextStyle on top (4.17 KB, text/plain)
2006-06-03 01:16 PDT, Jesse Ruderman
no flags Details
Valgrind log (7.50 KB, text/plain)
2006-06-03 01:30 PDT, Jesse Ruderman
no flags Details
Patch A, rev. 1 (21.10 KB, patch)
2006-08-21 17:03 PDT, Mats Palmgren (:mats)
roc: review+
roc: superreview+
dveditz: approval1.8.0.7+
mtschrep: approval1.8.1+
Details | Diff | Review
Patch B, rev. 1 (9.97 KB, patch)
2006-08-21 17:07 PDT, Mats Palmgren (:mats)
roc: review+
roc: superreview+
Details | Diff | Review

Description Jesse Ruderman 2006-05-10 06:01:08 PDT
This testcase causes a crash with a varying signature and sometimes with a random address on top.
Comment 1 Jesse Ruderman 2006-05-10 06:01:47 PDT
Created attachment 221578 [details]
testcase
Comment 2 Jesse Ruderman 2006-05-10 12:11:10 PDT
I'm curious why it matters that the page has a feed.
Comment 3 Jesse Ruderman 2006-06-03 00:56:10 PDT
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 2006-06-03 01:16:14 PDT
Created attachment 224297 [details]
A stack trace with nsTextStyle::nsTextStyle on top
Comment 5 Jesse Ruderman 2006-06-03 01:29:33 PDT
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 2006-06-03 01:30:27 PDT
Created attachment 224298 [details]
Valgrind log
Comment 7 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-06-05 15:08:31 PDT
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 2006-06-21 09:26:29 PDT
Still crashes on trunk, despite recent fixes for column-related crashes.
Comment 9 Jesse Ruderman 2006-08-17 22:04:12 PDT
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 2006-08-17 22:04:35 PDT
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 (:mats) 2006-08-21 17:03:55 PDT
Created attachment 234872 [details] [diff] [review]
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 (:mats) 2006-08-21 17:07:19 PDT
Created attachment 234873 [details] [diff] [review]
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 [:bz] (Out June 25-July 6) 2006-08-21 17:16:10 PDT
roc should probably just review all of these...
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-08-21 17:24:50 PDT
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) (Exited; email my personal email if necessary) 2006-08-21 17:41:48 PDT
Comment on attachment 234873 [details] [diff] [review]
Patch B, rev. 1

looks good
Comment 16 Mats Palmgren (:mats) 2006-08-21 19:22:23 PDT
(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 (:mats) 2006-08-23 23:30:49 PDT
"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 2006-08-25 10:49:06 PDT
Comment on attachment 234872 [details] [diff] [review]
Patch A, rev. 1

 a=schrep for drivers
Comment 19 Daniel Veditz [:dveditz] 2006-08-25 11:23:45 PDT
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 (:mats) 2006-08-25 14:44:48 PDT
Patch A: checked into to MOZILLA_1_8_BRANCH at 2006-08-25 13:21 PDT.
Comment 21 Mats Palmgren (:mats) 2006-08-25 17:24:03 PDT
Patch A: checked in to MOZILLA_1_8_0_BRANCH at 2006-08-25 15:36 PDT.
Comment 22 Jay Patel [:jay] 2006-09-07 12:37:36 PDT
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 2007-12-15 14:39:01 PST
Crashtest checked in.

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