Closed Bug 64670 Opened 24 years ago Closed 24 years ago

{ib} replaced out-of-flow inline element with view crashes browser

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla0.9

People

(Reporter: waterson, Assigned: waterson)

Details

(Keywords: crash)

Attachments

(4 files)

Whadda mouthful! This is a follow-up to bug 57026. Specifically, a replaced inline element that is not in the normal flow (e.g., the <object> in a relatively positioned <span>, below) <span style="position:relative"> <object> <p><form><input type="text"></input></form></p> </object> </span> will crash the browser. What happens is 1. the <object> is initially created as an inline frame 2. we realize that we can't render the object 3. we replace the <object> with the <p>, converting a inline to a block This runs through nsCSSFrameConstructor::SplitToContainingBlock(), which isn't clever enough to migrate the <input> element's view properly.
Attached file test case
Status: NEW → ASSIGNED
Keywords: crash
Priority: -- → P2
Target Milestone: --- → mozilla0.8
Moving to mozilla0.9: this case is now *extremely* rare. Might become more important if we get rid of WipeContainingBlock() in favor of SplitToContainingBlock().
Target Milestone: mozilla0.8 → mozilla0.9
Summary: replaced out-of-flow inline element with view crashes browser → {ib} replaced out-of-flow inline element with view crashes browser
Attached patch proposed fixSplinter Review
The above patch fixes the following: 1. A comma where there should've been a semi-colon. (Ack! C rules!) 2. I was mixing up the replaced frame and its parent when invoking SplitToContainingBlock(). This is the third and fifth chunks in nsCSSFrameConstructor.cpp 3. SplitToContainingBlock() now sanity checks its arguments, and handles cases where aLeftInlineChild or aRightInlineChild may be null. 4. SplitToContainingBlock() now is aware of frames that have views, and reparents accordingly. 5. Fix nsObjectFrame to only stifle key events if the plugin instance was successfully instantiated (otherwise, you can't replace the object frame with an <input>, e.g.!) saari, double-check me on that one... Fishing for some r= lovin'...
Chris, thanks for the mondo-comment on SplitToContainingBlock() - it is very clear but, uh, kinda complicated (I didn't know you were such the ASCII-artist). I think it is interesting that we never check if parentFrame is null in CantRenderReplacedElement - at least an assertion? My only other comment is that the indenting in the object frame stuff appears to be wrong (you added a new if() but did not indent what now is inside that block). [s]r=attinasi
Thanks for looking at this. Coupla questions... - I copied the comment from way back when I was figuring this out. Is it too gaudy? I could extract it and just refer to it if that's better. - I'll add a check for null pointer; I think this case is safe because of other, earlier out-of-memory conditions, but what the heck. - It's a ``diff -wu'', so whitespace differences aren't shown. I can give you the full-on diff if that's better...
Chris, the big ol' honkin' comment is fine, it is just the concept that is complicated I think. But I actually like that the details are right there with the code. The null ptr check will be good, or as assertion (can a frame have a null parent in that context? - I doubt it, so probably just assert). Finally, the diff is fine, I didn't realize that whitespace changes could be ignored like that - that is a sweet option. Thanks for the replies.
The view creation/reparenting logic in the patch looks fine to me. r=kmcclusk@netscape.com
The objectframe changes look good to me.
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Marking verified in the March 23rd build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: