Last Comment Bug 309322 - Evil testcase using multiple display:table-caption causes crash if you are really determined [@ nsIFrame::HasView]
: Evil testcase using multiple display:table-caption causes crash if you are re...
Status: VERIFIED FIXED
[sg:critical]
: crash, fixed1.8.0.15, testcase, verified1.8.1.8
Product: Core
Classification: Components
Component: Layout: Tables (show other bugs)
: 1.0 Branch
: x86 Windows 2000
-- critical (vote)
: ---
Assigned To: Bernd
:
:
Mentors:
http://realclearpolitics.com/
Depends on: 292756 341858 348126 389924 859424
Blocks:
  Show dependency treegraph
 
Reported: 2005-09-20 08:01 PDT by Martijn Wargers [:mwargers]
Modified: 2013-10-13 05:37 PDT (History)
14 users (show)
dveditz: blocking1.8.1.8+
dveditz: wanted1.8.1.x+
dveditz: blocking1.8.0.14-
dveditz: blocking1.8.0.next+
dveditz: wanted1.8.0.x+
mats: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (1.64 KB, text/html)
2005-09-20 08:03 PDT, Martijn Wargers [:mwargers]
no flags Details
testcase2 (1.85 KB, text/html)
2005-09-20 08:10 PDT, Martijn Wargers [:mwargers]
no flags Details
testcase3 (1.38 KB, text/html)
2005-09-20 09:15 PDT, Martijn Wargers [:mwargers]
no flags Details
testcase4 (1.45 KB, text/html)
2005-11-06 05:49 PST, Martijn Wargers [:mwargers]
no flags Details
wip (19.44 KB, patch)
2005-11-12 05:10 PST, Bernd
no flags Details | Diff | Splinter Review
patch (21.51 KB, patch)
2005-11-19 12:07 PST, Bernd
no flags Details | Diff | Splinter Review
revised patch (20.31 KB, patch)
2005-11-23 13:08 PST, Bernd
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
stacktrace from testcase4 (3.27 KB, text/plain)
2005-11-26 18:09 PST, Adam Guthrie
no flags Details
Patch for 1.8 branch (20.29 KB, patch)
2007-07-09 17:09 PDT, Daniel Holbert [:dholbert]
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
1.8 branch version w/regression fixes (26.06 KB, patch)
2007-10-04 14:30 PDT, Daniel Veditz [:dveditz]
no flags Details | Diff | Splinter Review
w/regression fix for 1.8.0 (24.90 KB, patch)
2008-02-28 06:35 PST, Alexander Sack
caillon: approval1.8.0.next+
Details | Diff | Splinter Review

Description User image Martijn Wargers [:mwargers] 2005-09-20 08:01:45 PDT
See upcoming testcase (I hope to simplify it further some other day)
Not sure if it says anything, bug it doesn't crash Mozilla1.7.

Talkback ID: TB9544861Z
Comment 1 User image Martijn Wargers [:mwargers] 2005-09-20 08:03:20 PDT
Created attachment 196792 [details]
testcase
Comment 2 User image Martijn Wargers [:mwargers] 2005-09-20 08:05:14 PDT
Almost forgot to cc you, Bernd, on "evil testcase causing crashes (in table
code)" bugs.
Comment 3 User image Martijn Wargers [:mwargers] 2005-09-20 08:10:41 PDT
Created attachment 196793 [details]
testcase2

Hmm, previous testcase doesn't always crash.
This testcase is crashing easier, because of the larger amount of <span>'s.
Comment 4 User image Martijn Wargers [:mwargers] 2005-09-20 09:15:14 PDT
Created attachment 196799 [details]
testcase3

Ok, testcase isn't always crashing, either.
This testcase is more minimal.
When clicking on the button, it creates an extra bogus frame.
Comment 5 User image Bernd 2005-09-20 11:42:43 PDT
I see it crashing in my "current" debug build, but not with a recent opitmized
nightly. However I care more for the first than for the later.
Comment 6 User image Bernd 2005-10-14 23:20:03 PDT
It seems that with the patch for bug 311822 this does not crash anymore in my
debug build. It asserts however. Martijn could you test whith the next nightly
if the crash is gone?
Comment 7 User image Bernd 2005-10-16 00:51:02 PDT
wfm Gecko/20051015 Firefox/1.6a1
Comment 8 User image Martijn Wargers [:mwargers] 2005-10-16 12:18:56 PDT
Yes, testcase and testcase2 don't crash anymore, but testcase3 still creates an
extra bogus frame, which is kinda bad, isn't it?
Comment 9 User image Bernd 2005-10-16 21:26:45 PDT
Martijn, if it does not crash I would like to downgrade it. The solution for the
reminder is outlined in bug 292756.
Comment 10 User image Martijn Wargers [:mwargers] 2005-11-06 05:49:57 PST
Created attachment 201989 [details]
testcase4

Well, it can still be made to crash, by using an animated gif in the testcase. The crash happens when closing the tab/window.
Comment 11 User image Bernd 2005-11-12 05:10:38 PST
Created attachment 202793 [details] [diff] [review]
wip

it still asserts
Comment 13 User image David Baron :dbaron: ⌚️UTC-8 2005-11-19 11:46:02 PST
I had a patch (~/mozilla/diffs/ib-rewrite.*) to replace IsBlockLevel with IsBlockInside (block, list-item, table-cell, table-caption, inline-block) and IsBlockOutside (block, list-item, table).  It was a small part of something that never got checked in, though.  (I actually used IsModelBlock and IsRoleBlock, and hadn't yet added all those things to each.)

There are two different concepts of block:  things that are block-like on the inside and things that are block-like on the outside.  We often confuse them.

That said, I think that HTMLReflowState frameType code should probably just go away.  It's an extra layer we don't need.
Comment 14 User image David Baron :dbaron: ⌚️UTC-8 2005-11-19 11:46:44 PST
... the patch I mentioned is on bug 142585, fwiw.
Comment 15 User image Bernd 2005-11-19 12:07:13 PST
Created attachment 203652 [details] [diff] [review]
patch
Comment 16 User image Bernd 2005-11-19 12:13:07 PST
Oh, I did not see your comment when I attached the patch
Comment 17 User image Bernd 2005-11-20 12:35:22 PST
Shall I split the lifting of Davids code into a separate bug or incorporate it into this patch?
Comment 18 User image David Baron :dbaron: ⌚️UTC-8 2005-11-20 12:41:06 PST
That depends why you needed to make the change to IsBlockLevel, I think.  Given the current contents of that function, the change is wrong, since IsBlockLevel means IsOutsideBlock, not IsInsideBlock.  Was there a bug that change was fixing, or did it just look necessary?  If the former, then you may need to pull it into this patch (although separate is always easier to review); if the latter, it should definitely be separate.
Comment 19 User image Bernd 2005-11-20 12:51:11 PST
without the changes I hit the assert http://lxr.mozilla.org/seamonkey/source/layout/base/nsCSSFrameConstructor.cpp#9161

NS_ASSERTION(PR_FALSE, "no last inline frame"); 
Comment 20 User image Bernd 2005-11-20 12:52:25 PST
I will revisit the patch with comment 18 in mind.
Comment 21 User image Boris Zbarsky [:bz] (still a bit busy) 2005-11-20 13:11:41 PST
So do you still want review on the patch as attached?  Or should I wait?  I'm not likely to get to it for a week or so no matter what, but just so I know...
Comment 22 User image Bernd 2005-11-23 13:08:05 PST
Created attachment 204061 [details] [diff] [review]
revised patch

as David said the change to IsBlockLevel was wrong (oh it would be cool if I could parse his comments on first read).
The problem that I faced was the assert inside nsCSSFrameConstructor::NeedSpecialFrameReframe. And what the function does is: it looks for the child content and decides from the style context of the child whether it will behave like block outside. This is wrong when we hit a table related frame as it will be wrapped into pseudos till a frame with table display will be on the outer side visible.

I am not sure about the IsBlockLevel() call inside nsCSSFrameConstructor::ContentAppended so I did not touch it. But it looks suspicious to me.
Comment 23 User image Bernd 2005-11-24 23:02:01 PST
The comment about NeedSpecialFrameReframe is better described at https://bugzilla.mozilla.org/show_bug.cgi?id=307394#c0
Comment 24 User image Adam Guthrie 2005-11-26 18:09:00 PST
Created attachment 204257 [details]
stacktrace from testcase4
Comment 25 User image Boris Zbarsky [:bz] (still a bit busy) 2005-11-27 16:38:35 PST
Comment on attachment 204061 [details] [diff] [review]
revised patch

r+sr=bzbarsky
Comment 26 User image Bernd 2005-12-03 09:06:00 PST
I am pretty reluctant to go with this for branch approval. Fix checked in
Comment 27 User image Stephen Donner [:stephend] 2005-12-04 15:33:42 PST
Verified FIXED using trunk build of SeaMonkey 2005-12-04-10 on Windows XP with all 4 testcases--no crash.
Comment 28 User image Daniel Veditz [:dveditz] 2007-03-20 23:55:54 PDT
This affects the 1.8 branch as well, and operations on deleted objects are potential security problems. This should be fixed on the branch(es) as well (and the regression fixes).
Comment 29 User image Bernd 2007-03-21 10:40:18 PDT
See comment 26, this is not low risk. It caused multiple regressions. I don't have a working environment to build & test branches. Neither do I have time to do this is in a reasonable time frame (3-4 weeks).
Comment 30 User image Daniel Veditz [:dveditz] 2007-03-21 15:51:51 PDT
leaving deleted-object crashers lying around isn't exactly low-risk either :-(
Comment 31 User image Daniel Veditz [:dveditz] 2007-03-26 15:27:08 PDT
dbaron: is this one you could help shepherd into the 1.8 branch?
Comment 32 User image Daniel Veditz [:dveditz] 2007-04-25 13:55:39 PDT
We should land this early next time instead, the regression risk is more than we want in FF2.0.0.4 if we're trying to make that a target for major upgrade.
Comment 33 User image Daniel Holbert [:dholbert] 2007-07-09 17:09:44 PDT
Created attachment 271586 [details] [diff] [review]
Patch for 1.8 branch

This is a version of "revised patch" (attachment 204061 [details] [diff] [review]), updated to apply to the 1.8 branch.  I had to modify the patch to accommodate 2 hunks that the original patch didn't work for out-of-the-box, due to changes in the 1.8 branch:

- nsCSSFrameConstructor.cpp:11210
     1.0 branch just calls CreateContinuingFrame, discarding return val
     1.8 branch stores return val of CreateContinuingFrame, and has a check for failure.
 (also minor differences in context after)
 
 - nsTableOuterFrame.cpp:206
     (minor change) -- 1.8 branch added 2 assertions in the contextual region of this hunk.  Original patch actually works here, using a fuzz factor.

My new patch preserves the new code from 1.8 branch in these regions.

My patch also fixes the only testcase which I could get to show buggy behavior, testcase #3.  (the other tescases seem to work for me without this patch)
Comment 34 User image Boris Zbarsky [:bz] (still a bit busy) 2007-07-09 23:18:28 PDT
Comment on attachment 271586 [details] [diff] [review]
Patch for 1.8 branch

Looks good.
Comment 35 User image Boris Zbarsky [:bz] (still a bit busy) 2007-07-29 07:50:47 PDT
Note that this patch causes bug 389924.  We probably want to either post a branch fix in that bug or just roll the fix into the patch.
Comment 36 User image Daniel Veditz [:dveditz] 2007-10-04 10:30:22 PDT
This patch applied fine to the branch, but had trouble merging the regression fixes (particularly bug 389924 which uses code added in the Reflow Branch landing). Think we'll have to wait to get this tied up safely.
Comment 37 User image Daniel Veditz [:dveditz] 2007-10-04 14:30:57 PDT
Created attachment 283610 [details] [diff] [review]
1.8 branch version w/regression fixes

This combines the above patch with the regression fixes for bug 389924 and bug 341858 (which in turn fixes bug 348126 and bug 343270)
Comment 38 User image Daniel Veditz [:dveditz] 2007-10-04 14:47:01 PDT
fix checked into 1.8 branch
Comment 39 User image Tony Chung [:tchung] 2007-10-04 22:14:43 PDT
Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.8) Gecko/20071004 Firefox/2.0.0.8. ID:2007100415.   Testcases fail to crash.  
Comment 40 User image Alexander Sack 2008-02-28 06:35:51 PST
Created attachment 306268 [details] [diff] [review]
w/regression fix for 1.8.0

caillon, this is "1.8 branch version w/regression fixes" diffed against 1.8.0.
Comment 41 User image Christopher Aillon (sabbatical, not receiving bugmail) 2008-03-19 13:23:04 PDT
Comment on attachment 306268 [details] [diff] [review]
w/regression fix for 1.8.0

a=caillon for 1.8.0.15
Comment 42 User image Christopher Aillon (sabbatical, not receiving bugmail) 2008-03-20 09:05:07 PDT
fixed on the 1.8.0 branch
Comment 43 User image Mats Palmgren (:mats) 2013-01-25 09:45:14 PST
Added crashtests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3834a081d85d
Comment 44 User image Ryan VanderMeulen [:RyanVM] 2013-01-26 16:51:21 PST
https://hg.mozilla.org/mozilla-central/rev/3834a081d85d

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