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

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
Editor
--
critical
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: tnikkel)

Tracking

(Blocks: 2 bugs, 4 keywords)

Trunk
mozilla1.9.3a1
x86
Mac OS X
crash, regression, testcase, verified1.9.2
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.0.x -
in-testsuite +

Firefox Tracking Flags

(blocking1.9.2 needed, status1.9.2 .2-fixed, blocking1.9.1 needed, status1.9.1 .9-fixed)

Details

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

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
Created attachment 381150 [details]
testcase (crashes Firefox when loaded)

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

Updated

8 years ago
Whiteboard: [sg:critical?]
(Assignee)

Comment 5

8 years ago
Created attachment 421987 [details] [diff] [review]
patch to make the frame constructor deal with recreating frames for native anonymous content

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-
(Assignee)

Comment 7

7 years ago
Created attachment 423771 [details] [diff] [review]
patch v2

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+
(Assignee)

Comment 10

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

Comment 11

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

Comment 13

7 years ago
http://hg.mozilla.org/mozilla-central/rev/30ef9a2cc88d
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
(Assignee)

Comment 14

7 years ago
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
status1.9.1: --- → wanted
status1.9.2: --- → wanted
(Assignee)

Updated

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

Comment 17

7 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fc8c7b0411e3
status1.9.1: wanted → .9-fixed
(Assignee)

Comment 18

7 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/6f4d006b87f8
status1.9.2: wanted → .2-fixed
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
(Assignee)

Updated

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

Comment 22

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

Comment 25

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

Comment 27

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