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] (not working for Mozilla)
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] (not working for Mozilla)
no flags Details
testcase2 (1.85 KB, text/html)
2005-09-20 08:10 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
testcase3 (1.38 KB, text/html)
2005-09-20 09:15 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
testcase4 (1.45 KB, text/html)
2005-11-06 05:49 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
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 Martijn Wargers [:mwargers] (not working for Mozilla) 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 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-09-20 08:03:20 PDT
Created attachment 196792 [details]
testcase
Comment 2 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-09-20 08:05:14 PDT
Almost forgot to cc you, Bernd, on "evil testcase causing crashes (in table
code)" bugs.
Comment 3 Martijn Wargers [:mwargers] (not working for Mozilla) 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 Martijn Wargers [:mwargers] (not working for Mozilla) 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 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 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 Bernd 2005-10-16 00:51:02 PDT
wfm Gecko/20051015 Firefox/1.6a1
Comment 8 Martijn Wargers [:mwargers] (not working for Mozilla) 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 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 Martijn Wargers [:mwargers] (not working for Mozilla) 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 Bernd 2005-11-12 05:10:38 PST
Created attachment 202793 [details] [diff] [review]
wip

it still asserts
Comment 13 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 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 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2005-11-19 11:46:44 PST
... the patch I mentioned is on bug 142585, fwiw.
Comment 15 Bernd 2005-11-19 12:07:13 PST
Created attachment 203652 [details] [diff] [review]
patch
Comment 16 Bernd 2005-11-19 12:13:07 PST
Oh, I did not see your comment when I attached the patch
Comment 17 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 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 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 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 Bernd 2005-11-20 12:52:25 PST
I will revisit the patch with comment 18 in mind.
Comment 21 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 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 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 Adam Guthrie 2005-11-26 18:09:00 PST
Created attachment 204257 [details]
stacktrace from testcase4
Comment 25 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 Bernd 2005-12-03 09:06:00 PST
I am pretty reluctant to go with this for branch approval. Fix checked in
Comment 27 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 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 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 Daniel Veditz [:dveditz] 2007-03-21 15:51:51 PDT
leaving deleted-object crashers lying around isn't exactly low-risk either :-(
Comment 31 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 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 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 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 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 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 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 Daniel Veditz [:dveditz] 2007-10-04 14:47:01 PDT
fix checked into 1.8 branch
Comment 39 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 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 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 Christopher Aillon (sabbatical, not receiving bugmail) 2008-03-20 09:05:07 PDT
fixed on the 1.8.0 branch
Comment 43 Mats Palmgren (:mats) 2013-01-25 09:45:14 PST
Added crashtests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3834a081d85d
Comment 44 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.