Closed Bug 309322 Opened 15 years ago Closed 14 years ago

Evil testcase using multiple display:table-caption causes crash if you are really determined [@ nsIFrame::HasView]

Categories

(Core :: Layout: Tables, defect, critical)

1.0 Branch
x86
Windows 2000
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: martijn.martijn, Assigned: bernd_mozilla)

References

(Depends on 1 open bug, )

Details

(4 keywords, Whiteboard: [sg:critical])

Crash Data

Attachments

(9 files, 2 obsolete files)

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
Attached file testcase
Almost forgot to cc you, Bernd, on "evil testcase causing crashes (in table
code)" bugs.
Attached file testcase2
Hmm, previous testcase doesn't always crash.
This testcase is crashing easier, because of the larger amount of <span>'s.
Attached file testcase3
Ok, testcase isn't always crashing, either.
This testcase is more minimal.
When clicking on the button, it creates an extra bogus frame.
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.
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?
wfm Gecko/20051015 Firefox/1.6a1
Yes, testcase and testcase2 don't crash anymore, but testcase3 still creates an
extra bogus frame, which is kinda bad, isn't it?
Martijn, if it does not crash I would like to downgrade it. The solution for the
reminder is outlined in bug 292756.
Severity: critical → normal
Depends on: 292756
Summary: Evil testcase using multiple display:table-caption causes crash → Evil testcase using multiple display:table-caption causes asserts
Attached file 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.
Severity: normal → critical
Keywords: crash
Summary: Evil testcase using multiple display:table-caption causes asserts → Evil testcase using multiple display:table-caption causes crash if you are really determined
Attached patch wip (obsolete) — Splinter Review
it still asserts
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.
... the patch I mentioned is on bug 142585, fwiw.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → bernd_mozilla
Attachment #202793 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #203652 - Flags: superreview?(bzbarsky)
Attachment #203652 - Flags: review?(bzbarsky)
Oh, I did not see your comment when I attached the patch
Shall I split the lifting of Davids code into a separate bug or incorporate it into this patch?
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.
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"); 
I will revisit the patch with comment 18 in mind.
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...
Attachment #203652 - Flags: superreview?(bzbarsky)
Attachment #203652 - Flags: review?(bzbarsky)
Attached patch revised patchSplinter Review
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.
Attachment #203652 - Attachment is obsolete: true
Attachment #204061 - Flags: superreview?(bzbarsky)
Attachment #204061 - Flags: review?(bzbarsky)
The comment about NeedSpecialFrameReframe is better described at https://bugzilla.mozilla.org/show_bug.cgi?id=307394#c0
Version: Trunk → 1.0 Branch
Summary: Evil testcase using multiple display:table-caption causes crash if you are really determined → Evil testcase using multiple display:table-caption causes crash if you are really determined [@ nsIFrame::HasView]
Comment on attachment 204061 [details] [diff] [review]
revised patch

r+sr=bzbarsky
Attachment #204061 - Flags: superreview?(bzbarsky)
Attachment #204061 - Flags: superreview+
Attachment #204061 - Flags: review?(bzbarsky)
Attachment #204061 - Flags: review+
I am pretty reluctant to go with this for branch approval. Fix checked in
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Verified FIXED using trunk build of SeaMonkey 2005-12-04-10 on Windows XP with all 4 testcases--no crash.
Status: RESOLVED → VERIFIED
Depends on: 341858
Depends on: 348126
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).
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
Whiteboard: [sg:critical]
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).
leaving deleted-object crashers lying around isn't exactly low-risk either :-(
Group: security
dbaron: is this one you could help shepherd into the 1.8 branch?
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.12?
Flags: blocking1.8.0.12+
Whiteboard: [sg:critical] → [sg:critical] need branch owner
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.
Flags: blocking1.8.1.5+
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.13+
Flags: blocking1.8.0.12+
Flags: blocking1.8.1.5+ → blocking1.8.1.6+
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)
Attachment #271586 - Flags: review?(bzbarsky)
Comment on attachment 271586 [details] [diff] [review]
Patch for 1.8 branch

Looks good.
Attachment #271586 - Flags: superreview+
Attachment #271586 - Flags: review?(bzbarsky)
Attachment #271586 - Flags: review+
Keywords: checkin-needed
Whiteboard: [sg:critical] need branch owner → [sg:critical] need branch checkins
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.
Flags: blocking1.8.0.13+ → blocking1.8.0.14+
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.
Flags: blocking1.8.1.8+ → blocking1.8.1.9+
Flags: blocking1.8.1.9+ → blocking1.8.1.8+
This combines the above patch with the regression fixes for bug 389924 and bug 341858 (which in turn fixes bug 348126 and bug 343270)
fix checked into 1.8 branch
Whiteboard: [sg:critical] need branch checkins → [sg:critical]
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.  
Group: security
Flags: in-testsuite?
Flags: blocking1.8.0.15+
Flags: blocking1.8.0.14-
Flags: blocking1.8.0.14+
caillon, this is "1.8 branch version w/regression fixes" diffed against 1.8.0.
Attachment #306268 - Flags: approval1.8.0.15?
Comment on attachment 306268 [details] [diff] [review]
w/regression fix for 1.8.0

a=caillon for 1.8.0.15
Attachment #306268 - Flags: approval1.8.0.15? → approval1.8.0.15+
fixed on the 1.8.0 branch
Keywords: fixed1.8.0.15
Crash Signature: [@ nsIFrame::HasView]
Added crashtests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3834a081d85d
Flags: in-testsuite? → in-testsuite+
Depends on: 859424
You need to log in before you can comment on or make changes to this bug.