The default bug view has changed. See this FAQ.

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

VERIFIED FIXED

Status

()

Core
Layout: Tables
--
critical
VERIFIED FIXED
12 years ago
4 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: Bernd)

Tracking

(Depends on: 1 bug, 4 keywords)

1.0 Branch
x86
Windows 2000
crash, fixed1.8.0.15, testcase, verified1.8.1.8
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.8 +
wanted1.8.1.x +
blocking1.8.0.14 -
blocking1.8.0.next +
wanted1.8.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical], crash signature, URL)

Attachments

(9 attachments, 2 obsolete attachments)

(Reporter)

Description

12 years ago
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
(Reporter)

Comment 1

12 years ago
Created attachment 196792 [details]
testcase
(Reporter)

Comment 2

12 years ago
Almost forgot to cc you, Bernd, on "evil testcase causing crashes (in table
code)" bugs.
(Reporter)

Comment 3

12 years ago
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.
(Reporter)

Comment 4

12 years ago
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.
(Assignee)

Comment 5

12 years ago
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.
(Assignee)

Comment 6

12 years ago
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?
(Assignee)

Comment 7

12 years ago
wfm Gecko/20051015 Firefox/1.6a1
(Reporter)

Comment 8

12 years ago
Yes, testcase and testcase2 don't crash anymore, but testcase3 still creates an
extra bogus frame, which is kinda bad, isn't it?
(Assignee)

Comment 9

12 years ago
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
(Reporter)

Comment 10

12 years ago
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.
(Assignee)

Updated

12 years ago
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
(Assignee)

Comment 11

12 years ago
Created attachment 202793 [details] [diff] [review]
wip

it still asserts
(Assignee)

Comment 12

12 years ago
why does 
http://lxr.mozilla.org/seamonkey/source/layout/style/nsStyleStruct.h#823
and
http://lxr.mozilla.org/mozilla/source/layout/generic/nsHTMLReflowState.cpp#490

differ?
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.
(Assignee)

Comment 15

12 years ago
Created attachment 203652 [details] [diff] [review]
patch
Assignee: nobody → bernd_mozilla
Attachment #202793 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #203652 - Flags: superreview?(bzbarsky)
Attachment #203652 - Flags: review?(bzbarsky)
(Assignee)

Comment 16

12 years ago
Oh, I did not see your comment when I attached the patch
(Assignee)

Comment 17

12 years ago
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.
(Assignee)

Comment 19

12 years ago
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"); 
(Assignee)

Comment 20

12 years ago
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...
(Assignee)

Updated

12 years ago
Attachment #203652 - Flags: superreview?(bzbarsky)
Attachment #203652 - Flags: review?(bzbarsky)
(Assignee)

Comment 22

12 years ago
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.
Attachment #203652 - Attachment is obsolete: true
Attachment #204061 - Flags: superreview?(bzbarsky)
Attachment #204061 - Flags: review?(bzbarsky)
(Assignee)

Comment 23

12 years ago
The comment about NeedSpecialFrameReframe is better described at https://bugzilla.mozilla.org/show_bug.cgi?id=307394#c0
Version: Trunk → 1.0 Branch

Updated

12 years ago
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 24

12 years ago
Created attachment 204257 [details]
stacktrace from testcase4
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+
(Assignee)

Comment 26

12 years ago
I am pretty reluctant to go with this for branch approval. Fix checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 12 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
(Reporter)

Updated

11 years ago
Depends on: 341858
(Reporter)

Updated

11 years ago
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]
(Assignee)

Comment 29

10 years ago
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+
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)
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
Depends on: 389924
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+
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)
fix checked into 1.8 branch
Keywords: checkin-needed → fixed1.8.1.8
Whiteboard: [sg:critical] need branch checkins → [sg:critical]

Comment 39

10 years ago
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.  
Keywords: fixed1.8.1.8 → verified1.8.1.8
Group: security
Flags: in-testsuite?
Flags: blocking1.8.0.15+
Flags: blocking1.8.0.14-
Flags: blocking1.8.0.14+

Comment 40

9 years ago
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.
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+
https://hg.mozilla.org/mozilla-central/rev/3834a081d85d
Depends on: 859424
You need to log in before you can comment on or make changes to this bug.