Bug 77077 – Crash in DeleteSelectionCommand::handleGeneralDelete when attempting to delete the start block

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Berend-Jan Wever]

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>

[Ryosuke Niwa]

Excellent! This is the repro I've been looking for a while.

[Ryosuke Niwa]

Created attachment 124627 [details]
fixes the crash

[Ryosuke Niwa]

Created attachment 124629 [details]
fixes the crash

[Enrica Casucci]

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?

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Created attachment 124639 [details]
Reverted some change

[Ryosuke Niwa]

Apparently removing calls to deprecated* isn't non-trivial in this function. Reverted that change.

[Ryosuke Niwa]

Created attachment 124644 [details]
Reverted some change; add back change logs

[Enrica Casucci]

Comment on attachment 124644 [details]
Reverted some change; add back change logs

Looks good to me.

[Ryosuke Niwa]

Thanks for the review.

[Ryosuke Niwa]

Committed r106380: <http://trac.webkit.org/changeset/106380>