Bug 47808

Summary: chrome.dll!WebCore::RangeBoundaryPoint::toPosition ReadAV@NULL (cf0d0f28bc56f2591cc74f71b46036ea)
Product: WebKit Reporter: Berend-Jan Wever <skylined>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Major CC: adele, darin, enrica, eric, jparent, ojan, rniwa, tony
Priority: P1 Keywords: HasReduction
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
fixes the crash darin: review+

Berend-Jan Wever
Reported 2010-10-18 03:32:33 PDT
Repro: <html><head><script> function go() { document.execCommand("selectall"); document.designMode="on"; document.execCommand("InsertLineBreak"); document.execCommand("insertimage"); document.execCommand("InsertOrderedList"); document.execCommand("inserthtml", false, "z"); document.execCommand("InsertHorizontalRule"); document.execCommand("selectall"); document.execCommand("createlink", false, "z"); document.execCommand("insertunorderedlist"); } </script></head><body onload="go();"></body></html> stack: chrome.dll!WebCore::RangeBoundaryPoint::toPosition chrome.dll!WebCore::InsertListCommand::doApply chrome.dll!WebCore::EditCommand::apply chrome.dll!WebCore::applyCommand chrome.dll!WebCore::executeInsertUnorderedList chrome.dll!WebCore::Editor::Command::execute chrome.dll!WebCore::Document::execCommand chrome.dll!WebCore::DocumentInternal::execCommandCallback ...
Attachments
fixes the crash (6.82 KB, patch)
2010-11-30 18:56 PST, Ryosuke Niwa 		darin: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1 2010-11-30 18:56:26 PST
Created attachment 75242 [details] fixes the crash
Tony Chang
Comment 2 2010-12-01 10:04:13 PST
Comment on attachment 75242 [details] fixes the crash View in context: https://bugs.webkit.org/attachment.cgi?id=75242&action=review > WebCore/editing/InsertListCommand.cpp:163 > + if (!lastSelectionRange) > + return; Do you think it is likely for this to regress again? This check doesn't seem to buy us much. > LayoutTests/editing/execCommand/switch-multiple-list-items-crash.html:1 > +<html><head><script> Nit: Can you add some text to this test (i.e., shouldn't crash and should print PASS)?
Ryosuke Niwa
Comment 3 2010-12-01 10:44:56 PST
http://crbug.com/59557 http://crbug.com/64692 This is a blocker for Chromium milestone 9.
Ryosuke Niwa
Comment 4 2010-12-01 11:00:57 PST
Comment on attachment 75242 [details] fixes the crash View in context: https://bugs.webkit.org/attachment.cgi?id=75242&action=review >> WebCore/editing/InsertListCommand.cpp:163 >> + return; > > Do you think it is likely for this to regress again? This check doesn't seem to buy us much. I'm sure there are other bugs in moveParagraph (which uses DeleteSelectionCommand, ReplaceSelectionCommand, etc...) and moveParagraphWithClones, and exiting early is better than crashing since a user won't lose data. Because the user can observe that not all paragraphs are listified or unlistified, they should be able to report bugs if this assertion hits. >> LayoutTests/editing/execCommand/switch-multiple-list-items-crash.html:1 >> +<html><head><script> > > Nit: Can you add some text to this test (i.e., shouldn't crash and should print PASS)? I thought doing so will stop reproducing the crash but it seems like it's not affected. Will add some explanation.
Tony Chang
Comment 5 2010-12-01 11:05:31 PST
(In reply to comment #4) > (From update of attachment 75242 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=75242&action=review > > >> WebCore/editing/InsertListCommand.cpp:163 > >> + return; > > > > Do you think it is likely for this to regress again? This check doesn't seem to buy us much. > > I'm sure there are other bugs in moveParagraph (which uses DeleteSelectionCommand, ReplaceSelectionCommand, etc...) and moveParagraphWithClones, and exiting early is better than crashing since a user won't lose data. Because the user can observe that not all paragraphs are listified or unlistified, they should be able to report bugs if this assertion hits. Won't we only get bug reports for debug builds (and if someone decides to file the bug)? In the wild, we only get reports of the app crashes. I'm worried that this is papering over future bugs.
Enrica Casucci
Comment 6 2010-12-01 11:07:21 PST
The change looks good to me
Darin Adler
Comment 7 2010-12-01 11:09:14 PST
Comment on attachment 75242 [details] fixes the crash View in context: https://bugs.webkit.org/attachment.cgi?id=75242&action=review >>>> WebCore/editing/InsertListCommand.cpp:163 >>>> + return; >>> >>> Do you think it is likely for this to regress again? This check doesn't seem to buy us much. >> >> I'm sure there are other bugs in moveParagraph (which uses DeleteSelectionCommand, ReplaceSelectionCommand, etc...) and moveParagraphWithClones, and exiting early is better than crashing since a user won't lose data. Because the user can observe that not all paragraphs are listified or unlistified, they should be able to report bugs if this assertion hits. > > Won't we only get bug reports for debug builds (and if someone decides to file the bug)? In the wild, we only get reports of the app crashes. I'm worried that this is papering over future bugs. I think that “papering over” is OK in this case.
Ryosuke Niwa
Comment 8 2010-12-01 11:10:53 PST
(In reply to comment #5) > Won't we only get bug reports for debug builds (and if someone decides to file the bug)? In the wild, we only get reports of the app crashes. I'm worried that this is papering over future bugs. Sure but I don't want to make debugging easier at the cost of user experience. We also have similar sanity checks & early exits in ApplyBlockElementCommand::formatSelection, which is used by FormatBlockCommand and IndentOutdentCommand.
Ryosuke Niwa
Comment 9 2010-12-01 11:11:36 PST
Thanks for the review, Darin & Tony. I'll add the comment Tony mentioned and land.
Ryosuke Niwa
Comment 10 2010-12-01 11:51:09 PST
Committed r73052: <http://trac.webkit.org/changeset/73052>
Note You need to log in before you can comment on or make changes to this bug.