Bug 57755 - REGRESSION(r81328): Null pointer crash in canAppendNewLineFeed when selection isn't inside an editable element
Summary: REGRESSION(r81328): Null pointer crash in canAppendNewLineFeed when selection...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows Vista
: P1 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-04 07:57 PDT by Berend-Jan Wever
Modified: 2011-04-06 01:52 PDT (History)
7 users (show)

See Also:


Attachments
Repro (224 bytes, text/html)
2011-04-04 07:57 PDT, Berend-Jan Wever
no flags Details
Patch (4.02 KB, patch)
2011-04-05 19:44 PDT, Naoki Takano
no flags Details | Formatted Diff | Diff
Patch (3.63 KB, patch)
2011-04-05 23:44 PDT, Naoki Takano
no flags Details | Formatted Diff | Diff
Patch (3.66 KB, patch)
2011-04-05 23:50 PDT, Naoki Takano
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Berend-Jan Wever 2011-04-04 07:57:10 PDT
Created attachment 88058 [details]
Repro

Chromium: http://code.google.com/p/chromium/issues/detail?id=78320
"selection.rootEditableElement()" can return NULL, something that "canAppendNewLineFeed" does not handle:

static bool canAppendNewLineFeed(const VisibleSelection& selection)
{
    ExceptionCode ec = 0;
    RefPtr<BeforeTextInsertedEvent> event = BeforeTextInsertedEvent::create(String("\n"));
    selection.rootEditableElement()->dispatchEvent(event, ec); // CRASH
    return event->text().length();
}

Repro:
<body onload="go()">
<script>
  function go() {
    document.designMode="on";
    document.write("</");
    document.getSelection().addRange(document.createRange());
    document.execCommand("InsertLineBreak");
  }
</script>

id:             chrome.dll!WebCore::EventTarget::dispatchEvent ReadAV@NULL (6c815dec64b94a3dde8974a25da7f213)
description:    Attempt to read from unallocated NULL pointer in chrome.dll!WebCore::EventTarget::dispatchEvent
application:    Chromium 12.0.725.0
stack:          chrome.dll!WebCore::EventTarget::dispatchEvent
                chrome.dll!WebCore::canAppendNewLineFeed
                chrome.dll!WebCore::TypingCommand::insertLineBreak
                chrome.dll!WebCore::EditCommand::apply
                chrome.dll!WebCore::applyCommand
                chrome.dll!WebCore::TypingCommand::insertLineBreak
                chrome.dll!WebCore::executeInsertLineBreak
                chrome.dll!WebCore::Editor::Command::execute
                chrome.dll!WebCore::Document::execCommand
                chrome.dll!WebCore::DocumentInternal::execCommandCallback
                chrome.dll!v8::internal::HandleApiCallHelper<...>
                chrome.dll!v8::internal::Builtin_HandleApiCall
                chrome.dll!v8::internal::Invoke
                chrome.dll!v8::internal::Execution::Call
                ...
Comment 1 Ryosuke Niwa 2011-04-05 05:02:28 PDT
Naoki, could you fix this crash?
Comment 2 Naoki Takano 2011-04-05 09:03:13 PDT
Ok, I'll look into it.
Comment 3 Naoki Takano 2011-04-05 18:27:23 PDT
I can reproduce ToT.
Comment 4 Naoki Takano 2011-04-05 19:44:05 PDT
Created attachment 88353 [details]
Patch
Comment 5 Naoki Takano 2011-04-05 19:44:26 PDT
Comment on attachment 88353 [details]
Patch

Please review.
Comment 6 Ryosuke Niwa 2011-04-05 23:22:21 PDT
Comment on attachment 88353 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88353&action=review

> LayoutTests/editing/execCommand/insert-line-break-onload.html:1
> +<body onload="go()">

Missing DOCTYPE.

> LayoutTests/editing/execCommand/insert-line-break-onload.html:2
> +Test InsertLineBreak is called correctly without any exception.

Mn... I'd expect that adding this line would stop reproducing the crash.  Did you make sure this test case cause a crash?

> LayoutTests/editing/execCommand/insert-line-break-onload.html:10
> +    var result = "Calling InsertLineBreak succeeded without any exception.";

I'd just print 'PASS' with the test description because that's what other tests do.

> LayoutTests/editing/execCommand/insert-line-break-onload.html:15
> +    } else
> +        alert(result);

Why do you need an alert here?  I'd expect for document.write to work here as well.

> Source/WebCore/editing/TypingCommand.cpp:52
> +    if (Node* node = selection.rootEditableElement()) {

We normally do an early exit in these cases.
Comment 7 Naoki Takano 2011-04-05 23:33:25 PDT
Thank you for your review.

(In reply to comment #6)
> > LayoutTests/editing/execCommand/insert-line-break-onload.html:2
> > +Test InsertLineBreak is called correctly without any exception.
> 
> Mn... I'd expect that adding this line would stop reproducing the crash.  Did you make sure this test case cause a crash?
Yes, it crashes.

> > LayoutTests/editing/execCommand/insert-line-break-onload.html:15
> > +    } else
> > +        alert(result);
> 
> Why do you need an alert here?  I'd expect for document.write to work here as well.
Because I referred to insert-line-break-no-scroll.html
Anyway I'll change it to use document.write().
Comment 8 Naoki Takano 2011-04-05 23:44:56 PDT
Created attachment 88376 [details]
Patch
Comment 9 Naoki Takano 2011-04-05 23:45:16 PDT
Comment on attachment 88376 [details]
Patch

Please review again.
Comment 10 Naoki Takano 2011-04-05 23:47:09 PDT
Comment on attachment 88376 [details]
Patch

Oops, I forgot to change one more thing...
Comment 11 Naoki Takano 2011-04-05 23:50:36 PDT
Created attachment 88377 [details]
Patch
Comment 12 Naoki Takano 2011-04-05 23:50:53 PDT
Comment on attachment 88377 [details]
Patch

Ok, please review again.
Comment 13 WebKit Commit Bot 2011-04-06 00:22:47 PDT
Comment on attachment 88377 [details]
Patch

Clearing flags on attachment: 88377

Committed r83026: <http://trac.webkit.org/changeset/83026>
Comment 14 WebKit Commit Bot 2011-04-06 00:22:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 WebKit Review Bot 2011-04-06 01:52:57 PDT
http://trac.webkit.org/changeset/83026 might have broken GTK Linux 32-bit Debug