Bug 77077 - Crash in DeleteSelectionCommand::handleGeneralDelete when attempting to delete the start block
Summary: Crash in DeleteSelectionCommand::handleGeneralDelete when attempting to delet...
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: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-26 01:15 PST by Berend-Jan Wever
Modified: 2012-01-31 12:41 PST (History)
5 users (show)

See Also:


Attachments
Repro (246 bytes, text/html)
2012-01-26 01:15 PST, Berend-Jan Wever
no flags Details
fixes the crash (7.30 KB, patch)
2012-01-30 16:37 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
fixes the crash (6.12 KB, patch)
2012-01-30 16:40 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Reverted some change (1.82 KB, patch)
2012-01-30 17:35 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Reverted some change; add back change logs (3.53 KB, patch)
2012-01-30 17:52 PST, Ryosuke Niwa
enrica: review+
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 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>