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

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

Hmm, previous testcase doesn't always crash. This testcase is crashing easier, because of the larger amount of <span>'s.

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.

Well, it can still be made to crash, by using an animated gif in the testcase. The crash happens when closing the tab/window.

it still asserts

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.

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...

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.

The comment about NeedSpecialFrameReframe is better described at https://bugzilla.mozilla.org/show_bug.cgi?id=307394#c0

Comment on attachment 204061 [details] [diff] [review] revised patch r+sr=bzbarsky

I am pretty reluctant to go with this for branch approval. Fix checked in

Verified FIXED using trunk build of SeaMonkey 2005-12-04-10 on Windows XP with all 4 testcases--no crash.

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).

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 :-(

dbaron: is this one you could help shepherd into the 1.8 branch?

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.

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 on attachment 271586 [details] [diff] [review] Patch for 1.8 branch Looks good.

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.

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.

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

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.

caillon, this is "1.8 branch version w/regression fixes" diffed against 1.8.0.

Comment on attachment 306268 [details] [diff] [review] w/regression fix for 1.8.0 a=caillon for 1.8.0.15

fixed on the 1.8.0 branch

Added crashtests: https://hg.mozilla.org/integration/mozilla-inbound/rev/3834a081d85d