Last Comment Bug 496011 - Crash [@ nsTextControlFrame::CalcIntrinsicSize] with execCommand inserthtml {ib} and undo
: Crash [@ nsTextControlFrame::CalcIntrinsicSize] with execCommand inserthtml {...
Status: RESOLVED FIXED
[sg:critical?]
: crash, regression, testcase, verified1.9.2
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: x86 Mac OS X
: -- critical (vote)
: mozilla1.9.3a1
Assigned To: Timothy Nikkel (:tnikkel)
:
Mentors:
Depends on: 558663
Blocks: stirdom 336383
  Show dependency treegraph
 
Reported: 2009-06-02 14:27 PDT by Jesse Ruderman
Modified: 2011-06-13 10:01 PDT (History)
12 users (show)
dveditz: wanted1.9.0.x-
tnikkel: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
needed
.2-fixed
needed
.9-fixed


Attachments
testcase (crashes Firefox when loaded) (433 bytes, application/xhtml+xml)
2009-06-02 14:27 PDT, Jesse Ruderman
no flags Details
patch to make the frame constructor deal with recreating frames for native anonymous content (1.36 KB, patch)
2010-01-16 00:18 PST, Timothy Nikkel (:tnikkel)
bzbarsky: review-
Details | Diff | Review
patch v2 (1.43 KB, patch)
2010-01-27 02:27 PST, Timothy Nikkel (:tnikkel)
bzbarsky: review+
mbeltzner: approval1.9.2.2+
mbeltzner: approval1.9.1.9+
Details | Diff | Review

Description Jesse Ruderman 2009-06-02 14:27:23 PDT
Created attachment 381150 [details]
testcase (crashes Firefox when loaded)

Null deref [@ nsTextControlFrame::CalcIntrinsicSize]
Comment 1 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-01-04 13:36:54 PST
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?
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-01-04 14:22:29 PST
(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.
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-01-04 14:31:08 PST
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.
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-01-04 15:00:20 PST
(In reply to comment #3)
> Ah, ok.  But we fixed that, right?  Given that, is this a reasonable thing to
> try?

Yes and yes.
Comment 5 Timothy Nikkel (:tnikkel) 2010-01-16 00:18:55 PST
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.
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-01-21 15:16:18 PST
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....
Comment 7 Timothy Nikkel (:tnikkel) 2010-01-27 02:27:35 PST
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.
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-01-27 09:48:17 PST
> 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 9 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-01-27 09:48:28 PST
Comment on attachment 423771 [details] [diff] [review]
patch v2

r=bzbarsky
Comment 10 Timothy Nikkel (:tnikkel) 2010-01-27 13:13:29 PST
(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.
Comment 11 Timothy Nikkel (:tnikkel) 2010-01-27 13:24:13 PST
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?
Comment 12 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-01-27 13:48:49 PST
> Or there any other cases where we might try to reconstruct a child of a leaf
> frame 

Not really, no.
Comment 13 Timothy Nikkel (:tnikkel) 2010-02-03 22:47:29 PST
http://hg.mozilla.org/mozilla-central/rev/30ef9a2cc88d
Comment 14 Timothy Nikkel (:tnikkel) 2010-02-03 22:49:54 PST
After baking on trunk this is something we might want on the branches.
Comment 15 Mike Beltzner [:beltzner, not reading bugmail] 2010-02-10 12:58:20 PST
Sure, nominate the patch when it's ready to land.
Comment 16 Mike Beltzner [:beltzner, not reading bugmail] 2010-03-05 10:53:36 PST
Comment on attachment 423771 [details] [diff] [review]
patch v2

a=beltzner for 1.9.2.2 and 1.9.1.9
Comment 17 Timothy Nikkel (:tnikkel) 2010-03-05 15:13:36 PST
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fc8c7b0411e3
Comment 18 Timothy Nikkel (:tnikkel) 2010-03-05 15:15:28 PST
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/6f4d006b87f8
Comment 19 [On PTO until 6/29] 2010-03-15 17:06:36 PDT
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.
Comment 20 Daniel Veditz [:dveditz] 2010-03-28 22:14:57 PDT
This testcase does not crash 1.9.0.19
Comment 21 :Ehsan Akhgari (busy, don't ask for review please) 2010-04-15 12:24:33 PDT
(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.
Comment 22 Timothy Nikkel (:tnikkel) 2010-04-15 12:32:23 PDT
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.
Comment 23 :Ehsan Akhgari (busy, don't ask for review please) 2010-04-15 15:27:45 PDT
(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.
Comment 24 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-04-15 19:28:24 PDT
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.
Comment 25 Timothy Nikkel (:tnikkel) 2010-06-27 17:23:37 PDT
Added crashtest
http://hg.mozilla.org/mozilla-central/rev/1c374190ca92
Comment 26 :Ehsan Akhgari (busy, don't ask for review please) 2010-07-13 19:09:22 PDT
Note that this changeset might be relevant to this bug:

http://hg.mozilla.org/mozilla-central/rev/2a99ed409a38
Comment 27 Timothy Nikkel (:tnikkel) 2010-07-13 19:10:12 PDT
Indeed, I think that part 3 of bug 289384 (the changeset above) fixes the editor bug here.

Note You need to log in before you can comment on or make changes to this bug.