Bug 77077

Summary: Crash in DeleteSelectionCommand::handleGeneralDelete when attempting to delete the start block
Product: WebKit Reporter: Berend-Jan Wever <skylined>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, enrica, ojan, rniwa, tony
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows Vista   
Attachments:
Description Flags
Repro
none
fixes the crash
none
fixes the crash
none
Reverted some change
none
Reverted some change; add back change logs enrica: review+

Description Berend-Jan Wever 2012-01-26 01:15:37 PST
Created attachment 124076 [details]
Repro

Chromium:       http://code.google.com/p/chromium/issues/detail?id=111460
id:             chrome.dll!WebCore::caretMaxOffset ReadAV@NULL (16082e3239491f3c9d95ed01a263acfe)
description:    Attempt to read from unallocated NULL pointer+0x14 in chrome.dll!WebCore::caretMaxOffset
application:    Chromium 18.0.1011.0
stack:          chrome.dll!WebCore::caretMaxOffset
                chrome.dll!WebCore::DeleteSelectionCommand::handleGeneralDelete
                chrome.dll!WebCore::DeleteSelectionCommand::doApply
                chrome.dll!WebCore::CompositeEditCommand::applyCommandToComposite
                chrome.dll!WebCore::CompositeEditCommand::deleteSelection
                chrome.dll!WebCore::InsertParagraphSeparatorCommand::doApply
                chrome.dll!WebCore::CompositeEditCommand::applyCommandToComposite
                chrome.dll!WebCore::TypingCommand::insertParagraphSeparator
                chrome.dll!WebCore::CompositeEditCommand::apply
                chrome.dll!WebCore::applyCommand
                chrome.dll!WebCore::TypingCommand::insertParagraphSeparator
                chrome.dll!WebCore::executeInsertParagraph
                chrome.dll!WebCore::Editor::Command::execute
                chrome.dll!WebCore::Document::execCommand
                chrome.dll!WebCore::DocumentInternal::execCommandCallback

Repro:
><progress><script>
  document.designMode="on";
  document.execCommand("selectall");
  document.execCommand("justifycenter",false);
  document.body.removeChild(document.body.firstElementChild);
  document.execCommand("insertparagraph");
</script>
Comment 1 Ryosuke Niwa 2012-01-30 15:04:27 PST
Excellent! This is the repro I've been looking for a while.
Comment 2 Ryosuke Niwa 2012-01-30 16:37:49 PST
Created attachment 124627 [details]
fixes the crash
Comment 3 Ryosuke Niwa 2012-01-30 16:40:12 PST
Created attachment 124629 [details]
fixes the crash
Comment 4 Enrica Casucci 2012-01-30 16:50:52 PST
Comment on attachment 124629 [details]
fixes the crash

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

I'm not convinced that the fix is right. Could you please explain why the different results are ok? Thanks!

> LayoutTests/editing/deleting/delete-start-block.html:1
> +><progress><script src="../../resources/dump-as-markup.js"></script><script>

What is this?

> LayoutTests/editing/execCommand/delete-image-in-anchor-expected.txt:4
> +AFTER: <div style="text-align: center;"><br></div> <br>

This result doesn't look equivalent to me. Why is it ok to remove font and u tags here?

> LayoutTests/platform/mac/editing/deleting/delete-3608462-fix-expected.txt:-28
> -EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification

Isn't this an indication that something did not happen as expected?
Comment 5 Ryosuke Niwa 2012-01-30 16:57:34 PST
(In reply to comment #4)
> (From update of attachment 124629 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124629&action=review
> 
> I'm not convinced that the fix is right. Could you please explain why the different results are ok? Thanks!

It's okay to remove those styles after deleting the contents since the removed style stays in the typing style. Having said that, it's somehow not storing the typing style properly when I manually test it :( I'll investigate it.

> > LayoutTests/editing/deleting/delete-start-block.html:1
> > +><progress><script src="../../resources/dump-as-markup.js"></script><script>
> 
> What is this?

You mean ">" before <progress>? It's required as a part of the test.

> > LayoutTests/editing/execCommand/delete-image-in-anchor-expected.txt:4
> > +AFTER: <div style="text-align: center;"><br></div> <br>
> 
> This result doesn't look equivalent to me. Why is it ok to remove font and u tags here?

It is. There's some progression in that superfluous markups are removed but I need to check why the typing style isn't updated :(

> > LayoutTests/platform/mac/editing/deleting/delete-3608462-fix-expected.txt:-28
> > -EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
> 
> Isn't this an indication that something did not happen as expected?

Yeah, my patch DOES change the behavior of deletion so some behavioral changes are expected.
Comment 6 Ryosuke Niwa 2012-01-30 17:35:42 PST
Created attachment 124639 [details]
Reverted some change
Comment 7 Ryosuke Niwa 2012-01-30 17:37:24 PST
Apparently removing calls to deprecated* isn't non-trivial in this function. Reverted that change.
Comment 8 Ryosuke Niwa 2012-01-30 17:52:51 PST
Created attachment 124644 [details]
Reverted some change; add back change logs
Comment 9 Enrica Casucci 2012-01-31 10:15:59 PST
Comment on attachment 124644 [details]
Reverted some change; add back change logs

Looks good to me.
Comment 10 Ryosuke Niwa 2012-01-31 12:14:19 PST
Thanks for the review.
Comment 11 Ryosuke Niwa 2012-01-31 12:41:11 PST
Committed r106380: <http://trac.webkit.org/changeset/106380>