403148 - Crash [@ nsTextControlFrame::SetValue] with <input type=file> and contentEditable

Status: RESOLVED WORKSFORME

(Core :: DOM: Core & HTML, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Loading the testcase crashes Firefox.

Setting contentEditable on the span causes the file upload control to disappear (!).  Then, turning the file upload control into a normal input crashes:

Thread 0 Crashed:
0   nsTextControlFrame::SetValue (nsTextControlFrame.cpp:2675)
1   nsFileControlFrame::SetFormProperty (nsFileControlFrame.cpp:546)
2   nsHTMLInputElement::SetFileName (nsHTMLInputElement.cpp:849)
3   nsHTMLInputElement::ParseAttribute (nsHTMLInputElement.cpp:2009)
4   nsGenericElement::SetAttr (nsGenericElement.cpp:3569)
5   nsGenericHTMLElement::SetAttr (nsGenericHTMLElement.cpp:1400)
6   nsGenericHTMLElement::SetAttribute (nsGenericHTMLElement.cpp:330)
7   nsHTMLInputElement::SetAttribute (nsHTMLInputElement.cpp:167)
8   NS_InvokeByIndex_P + 98 (xptcinvoke_unixish_x86.cpp:179)
...

Comment 1

[Boris Zbarsky [:bzbarsky]]

So the problem here is that setting that contentEditable=true loads contenteditable.css, and one of the rules in there changes the style on the anonymous inputs inside the file input from "-moz-user-modify: disabled" to "-moz-user-modify: none".  Which triggers a reframe, so we try to reframe anonymous content. Not only does that fail miserably, but we end up with a dangling pointer to the (now dead) text control frame in the file control frame, so we crash when we try to use that pointer (as here).

Is contenteditable.css really supposed to apply to native anonymous content?

Comment 2

[Mike Schroepfer]

Marking blocking and setting p2 (since it is a crash).  Bz if you think differently please adjust...

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

My patch for Bug 389324 – "[contenteditable="true"] does a bad job in contenteditable.css" prevents this crash, so I'm marking Bug 389324 as blocking this one, and raising it to P2.

Comment 4

[Jesse Ruderman]

We should not be relying on !important rules in "extra" UA stylesheets such as contenteditable.css to prevent crashes.  See bug 347355 comment 15 for some reasons.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #3)
> My patch for Bug 389324 – "[contenteditable="true"] does a bad job in
> contenteditable.css" prevents this crash, so I'm marking Bug 389324 as blocking
> this one, and raising it to P2.
> 

Ok, removing dependency and returning priority of bug 389324 to its previous P4 value.

Comment 6

[Martijn Wargers (dead)]

The patch in bug 389324 still affects the testcase in this bug, so I think the dependancy is justified, fwiw.

Comment 7

[Johnny Stenback (:jst)]

Not going to block on this, but we're likely to take a fix if one shows up (before or after release).

Comment 8

[Jesse Ruderman]

The testcase no longer causes a crash for me.  Boris, do the problems you explained in comment 1 still exist?

Comment 9

[Boris Zbarsky [:bzbarsky]]

I would think so, yes.  I don't have time to dig to make sure right now.

Comment 10

[Jesse Ruderman]

ok, well, i filed bug 505619 for that.

*this* bug is WFM and will soon be in-testsuite+

Comment 11

[Jesse Ruderman]

Test added: http://hg.mozilla.org/mozilla-central/rev/91cb26be77bc