Closed Bug 496011 Opened 15 years ago Closed 15 years ago

Crash [@ nsTextControlFrame::CalcIntrinsicSize] with execCommand inserthtml {ib} and undo

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.3a1
Tracking Flags:
Tracking Status
blocking1.9.2 --- needed
status1.9.2 --- .2-fixed
blocking1.9.1 --- needed
status1.9.1 --- .9-fixed

People

(Reporter: jruderman, Assigned: tnikkel)

References

Details

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

Crash Data

Attachments

(2 files, 1 obsolete file)

testcase (crashes Firefox when loaded)
433 bytes, application/xhtml+xml
Details
patch v2
1.43 KB, patch
bzbarsky
: review+
beltzner
: approval1.9.2.2+
beltzner
: approval1.9.1.9+
Details | Diff | Splinter Review 
Null deref [@ nsTextControlFrame::CalcIntrinsicSize]
The nsTextControlFrame has no mFirstChild.

And the reason for that is that for some reason those execCommand's end up operating on the kids of the textarea.  In particular, the insertHTML calls actually mess with the anonymous DOM inside the textarea, so that after the insertHTML call the frame tree actually has the ib split for that span-containing-a-div inside the textarea's anonymous block.  Then undo removes the ib split, which tries to recreate the anonymous block, which fails but leaves the textarea in a bad state.

We could at the very least reconstruct the textarea when we go to reconstruct that anonymous block.  roc, I think we've talked about that sort of thing before; do you recall why we didn't do it?

But apart from that, why is the HTML editor on the <body> messing with the anonymous internal DOM in the textarea?
Group: core-security
(In reply to comment #1)
> We could at the very least reconstruct the textarea when we go to reconstruct
> that anonymous block.  roc, I think we've talked about that sort of thing
> before; do you recall why we didn't do it?

In the case I was looking at, reconstruction of the anonymous block was being triggered by the initialization of the textarea frame itself. So propagating that reconstruction up to the textarea would have triggered an infinite loop.
Ah, ok.  But we fixed that, right?  Given that, is this a reasonable thing to try?

I still think we need to fix the editor bug here, though.
(In reply to comment #3)
> Ah, ok.  But we fixed that, right?  Given that, is this a reasonable thing to
> try?

Yes and yes.
Whiteboard: [sg:critical?]
Just get the first non-native anonymous ancestor and recreate that instead.
Attachment #421987 - Flags: review?(bzbarsky)
Comment on attachment 421987 [details] [diff] [review]
patch to make the frame constructor deal with recreating frames for native anonymous content

I don't think this is the right thing to do.  In particular, this can easily confuse things like video controls, I think...

A better solution, maybe, would be to have MaybeRecreateContainerForFrameRemoval recreate the parent if the frame's content is the root of a native anonymous subtree.  We could do that in RecreateFramesForContent instead, but doing it in MaybeRecreateContainerForFrameRemoval should cover the case of someone calling ContentRemoved directly to, right?  I _think_ we want to reconstruct the ancestor in that case....
Attachment #421987 - Flags: review?(bzbarsky) → review-
Attached patch patch v2 — — Splinter Review 
patch v2

Although I'm not totally sure why we would want to reconstruct the parent the root of a native anonymous subtree is removed.
Assignee: nobody → tnikkel
Attachment #421987 - Attachment is obsolete: true
Attachment #423771 - Flags: review?(bzbarsky)
> Although I'm not totally sure why we would want to reconstruct the parent the
> root of a native anonymous subtree is removed.

Because in general, the parent expects its native anon content to be around (as here) and if that frame is gone the parent will be _very_ confused.
Comment on attachment 423771 [details] [diff] [review]
patch v2

r=bzbarsky
Attachment #423771 - Flags: review?(bzbarsky) → review+
(In reply to comment #8)
> Because in general, the parent expects its native anon content to be around (as
> here) and if that frame is gone the parent will be _very_ confused.

Yeah ok, I was thinking that the only thing that would try to do ContentRemoved on the root of a native anonymous subtree would be the owner frame, so that the owner gets rid of its native anonymous content purposefully for some reason, and then we reconstruct the owner and it creates the native anonymous content again.
Or there any other cases where we might try to reconstruct a child of a leaf frame (the reason the reconstruct fails in this testcase)? Might we want to do something similar for children of leaf frames at some point?
> Or there any other cases where we might try to reconstruct a child of a leaf
> frame 

Not really, no.
http://hg.mozilla.org/mozilla-central/rev/30ef9a2cc88d
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
After baking on trunk this is something we might want on the branches.
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
Sure, nominate the patch when it's ready to land.
blocking1.9.1: ? → needed
blocking1.9.2: ? → needed
Attachment #423771 - Flags: approval1.9.2.2?
Attachment #423771 - Flags: approval1.9.1.9?
Comment on attachment 423771 [details] [diff] [review]
patch v2

a=beltzner for 1.9.2.2 and 1.9.1.9
Attachment #423771 - Flags: approval1.9.2.2?
Attachment #423771 - Flags: approval1.9.2.2+
Attachment #423771 - Flags: approval1.9.1.9?
Attachment #423771 - Flags: approval1.9.1.9+
Verified fixed for 1.9.2 using testcase with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.2pre) Gecko/20100312 Namoroka/3.6.2pre. 1.9.2.0 crashes with the testcase.

I can't verify this with the testcase for 1.9.1 because 1.9.1.8 doesn't crash with the attached testcase. Has anyone seen this crash on 1.9.1 or have suggestions for reproducing the crash? I tried both debug and retail builds.
Keywords: verified1.9.2
This testcase does not crash 1.9.0.19
Flags: wanted1.9.0.x-
Keywords: regression
Depends on: 558663
(In reply to comment #1)
> And the reason for that is that for some reason those execCommand's end up
> operating on the kids of the textarea.  In particular, the insertHTML calls
> actually mess with the anonymous DOM inside the textarea, so that after the
> insertHTML call the frame tree actually has the ib split for that
> span-containing-a-div inside the textarea's anonymous block.  Then undo removes
> the ib split, which tries to recreate the anonymous block, which fails but
> leaves the textarea in a bad state.

What is the expected thing to happen here?  Reading the code, as far as I can tell, the inserthtml command first deletes the selection if it's not collapsed.  Doing so in this case will boil down to a bunch of RemoveChild calls on the body element.  The RemoveChild on the textarea should also lead to the span element being removed, right?  And it should also remove textarea's native anonymous subtree.  I'm kind of confused on what's happening here out of ordinary.

BTW, I'm not sure what ib splits are, so that might explain why I don't understand the issue hre.
The ib-split is the <span><div></div></span>. An ib-split is any inline element that contains a block element. The block ends up splitting the inline.
(In reply to comment #22)
> The ib-split is the <span><div></div></span>. An ib-split is any inline element
> that contains a block element. The block ends up splitting the inline.

Thanks for the explanation.  But that still doesn't help me understand comment 1 completely.
Ehsan, what happens here is that the insertHTML call inserts the <span><div/></span> gunk into the anonymous content subtree inside the textarea (presumably because that's where the selection is positioned?).  The fact that this happens seems like a bug in insertHTML to me.
Group: core-security
Added crashtest
http://hg.mozilla.org/mozilla-central/rev/1c374190ca92
Flags: in-testsuite? → in-testsuite+
Note that this changeset might be relevant to this bug:

http://hg.mozilla.org/mozilla-central/rev/2a99ed409a38
Indeed, I think that part 3 of bug 289384 (the changeset above) fixes the editor bug here.
Crash Signature: [@ nsTextControlFrame::CalcIntrinsicSize]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: