295531 - Crash in [@ nsHTMLEditor::InsertHTMLWithContext] inserting a space because it assumes that there is at least one node to paste

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

This function, which is called from ther Insert -> HTML dialog box in composer,
assumes in a couple of places that there is at least one node to paste. The
result is that if there isn't, it attempts to dereference nodeList[0], and
crashes for its efforts. I'm not entirely sure of why it makes this assumption,
but I have a patch that fixes it to not crash if there are no nodes to paste.

STEPS TO REPRODUCE:
1. Start composer
2. Click on Insert -> HTML
3. Enter ony a space
4. Click "Insert"

EXPECTED RESULTS: Nothing happens (or a space is inserted).
ACTUAL RESULTS: An assertion (followed by a crash).

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: bail early if there is nothing to paste

This patch fixes the crash/assertion by bailing early if there is not, in fact,
anything to paste.

I'm not certain this is the right approach, but it seems to make sense in this
context.

Comment 2

[Kathleen :Brade]

Comment on attachment 184545 [details] [diff] [review]
bail early if there is nothing to paste

r=brade

I'm curious what data is causing there to be no nodes.	Is the editor being
asked to insert an empty string or ?

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 184545 [details] [diff] [review]
bail early if there is nothing to paste

Editor is being asked to insert a space. I guess that at some point, spaces get
stripped out, leading to the crash.

Simon, can you sr?

Comment 4

[timeless]

what if i want to insert html &"&endash; :)

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 184545 [details] [diff] [review]
bail early if there is nothing to paste

After some more debugging, I no longer like this patch. The issue I was seeing
was _another_ htmlparser bug. I want to add an assertion though.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: add an assertion

It's worth adding an assertion, IMO, since we're making the assumption later,
anyway.

Comment 7

[Frank Wein [:mcsmurf]]

Comment on attachment 184631 [details] [diff] [review]
add an assertion

crasher fix

Comment 8

[Frank Wein [:mcsmurf]]

Comment on attachment 184631 [details] [diff] [review]
add an assertion

oh nevermind this actually adds an assertion only ;)

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 184631 [details] [diff] [review]
add an assertion

This assertion has already helped track down two dataloss problems in the HTML
parser, so it should probably go into 1.8b3

Comment 10

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 184631 [details] [diff] [review]
add an assertion

a=shaver

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Assertion checked in.

Comment 12

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2)
Gecko/20050602 on Windows XP Seamonkey trunk using comment 0 as my testcase.

No crash, and inserting spaces (by themselves) does nothing, which is expected.