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>
Excellent! This is the repro I've been looking for a while.
Created attachment 124627 [details] fixes the crash
Created attachment 124629 [details] fixes the crash
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?
(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.
Created attachment 124639 [details] Reverted some change
Apparently removing calls to deprecated* isn't non-trivial in this function. Reverted that change.
Created attachment 124644 [details] Reverted some change; add back change logs
Comment on attachment 124644 [details] Reverted some change; add back change logs Looks good to me.
Thanks for the review.
Committed r106380: <http://trac.webkit.org/changeset/106380>