Bug 8145 - REGRESSION: Pasting text from TextEdit with a bold word into text field results in crash
Summary: REGRESSION: Pasting text from TextEdit with a bold word into text field resul...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P1 Normal
Assignee: Justin Garcia
URL:
Keywords: InRadar, Regression
: 8149 (view as bug list)
Depends on:
Blocks:
 
Reported: 2006-04-02 15:20 PDT by Mark Rowe (bdash)
Modified: 2006-04-07 11:43 PDT (History)
3 users (show)

See Also:


Attachments
Crash log. (21.65 KB, text/plain)
2006-04-02 15:22 PDT, Mark Rowe (bdash)
no flags Details
Rough patch (1.48 KB, patch)
2006-04-02 17:18 PDT, Mark Rowe (bdash)
darin: review-
Details | Formatted Diff | Diff
patch, changelog and layout tests (23.70 KB, patch)
2006-04-06 16:13 PDT, Justin Garcia
no flags Details | Formatted Diff | Diff
patch, changelog and layout tests (23.89 KB, patch)
2006-04-06 16:20 PDT, Justin Garcia
no flags Details | Formatted Diff | Diff
patch, changelog and layout tests (23.94 KB, patch)
2006-04-06 21:08 PDT, Justin Garcia
justin.garcia: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Rowe (bdash) 2006-04-02 15:20:41 PDT
Steps to reproduce:
1) Open TextEdit
2) Type a few words of text
3) Select a word and make it bold
4) Copy all of the text
5) Open http://www.google.com/ and paste the text into the search field
6) <crash>

The crash log is attached.
Comment 1 Mark Rowe (bdash) 2006-04-02 15:22:14 PDT
Created attachment 7470 [details]
Crash log.
Comment 2 Darin Adler 2006-04-02 17:17:44 PDT
Crash happens because a node is nil. This happens because of this code in ReplaceSelectionCommand::doApply:

702             } else if (m_lastNodeInserted && !m_fragment.isBlockFlow(refNode)) {
703                 Position pos = visiblePos.next().deepEquivalent().downstream();
704                 insertNodeAtAndUpdateNodesInserted(refNode, pos.node(), pos.offset());

visiblePos.next() is null.
Comment 3 Mark Rowe (bdash) 2006-04-02 17:18:13 PDT
Created attachment 7471 [details]
Rough patch

The patch simply adds a NULL check before the call to insertNodeAtAndUpdateNodesInserted.  I don't really understand the code in question so I am in no way confident that it is the right fix, however it prevents the crash and doesn't appear to change any other behaviour.
Comment 4 Darin Adler 2006-04-02 17:56:21 PDT
Comment on attachment 7471 [details]
Rough patch

I believe this is wrong and will result in the pasted text not being inserted at all!

The right thing is to find a way to insert after visiblePos instead of before visiblePos.next() in this case.
Comment 5 Darin Adler 2006-04-02 18:53:50 PDT
Justin should probably handle this one himself.
Comment 6 Mark Rowe (bdash) 2006-04-02 20:23:28 PDT
The pasted text is definitely inserted correctly with that patch, but I'm more than willing to let Justin handle this.
Comment 7 Maciej Stachowiak 2006-04-02 23:25:45 PDT
These are all text field regressions so they should all be P1.
Comment 8 Darin Adler 2006-04-04 09:19:19 PDT
*** Bug 8149 has been marked as a duplicate of this bug. ***
Comment 9 Darin Adler 2006-04-04 09:20:16 PDT
Same as <rdar://problem/4499431>.
Comment 10 Justin Garcia 2006-04-06 16:13:27 PDT
Created attachment 7545 [details]
patch, changelog and layout tests
Comment 11 Justin Garcia 2006-04-06 16:20:37 PDT
Created attachment 7546 [details]
patch, changelog and layout tests
Comment 12 Justin Garcia 2006-04-06 17:49:01 PDT
Comment on attachment 7546 [details]
patch, changelog and layout tests

dave reviewed this in person, and he's going to look it over and do a final review tonight.
Comment 13 Justin Garcia 2006-04-06 21:08:04 PDT
Created attachment 7552 [details]
patch, changelog and layout tests

One more revision of my changelog and code comments.
Comment 14 Justin Garcia 2006-04-07 10:59:54 PDT
Comment on attachment 7552 [details]
patch, changelog and layout tests

dave reviewed this