Bug 32823 - Various designmode="on"/"off" & execCommand("Undo") NULL pointer crashes
Summary: Various designmode="on"/"off" & execCommand("Undo") NULL pointer crashes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Ryosuke Niwa
URL: http://skypher.com/SkyLined/Repro/Web...
Keywords: InRadar
: 32424 32425 32822 33049 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-12-21 07:17 PST by Berend-Jan Wever
Modified: 2011-03-15 10:07 PDT (History)
10 users (show)

See Also:


Attachments
Repro - WebCore::caretMaxOffset ReadAV@NULL (5785e692fef72522c3ee976bff525d98) (474 bytes, text/html)
2009-12-21 07:17 PST, Berend-Jan Wever
no flags Details
reduced test case with comments (519 bytes, text/html)
2009-12-30 09:33 PST, Mike Moretti
no flags Details
Repro - WebCore::Node::hasTagName ReadAV@NULL (106220ce302426f16b811b1cc735e87a) (277 bytes, text/html)
2010-07-27 02:41 PDT, Berend-Jan Wever
no flags Details
Repro - WebCore::ApplyStyleCommand::applyBlockStyle ReadAV@NULL (eba56e4a5c0f11cedaba7aea0468d82a) (506 bytes, text/html)
2010-07-27 02:47 PDT, Berend-Jan Wever
no flags Details
Repro - WebCore::ReplaceSelectionCommand::doApply ReadAV@NULL (badacdea771d24a7e25e9ab3859da7a2) (277 bytes, text/html)
2010-07-27 02:48 PDT, Berend-Jan Wever
no flags Details
Repro - WebCore::positionInParentBeforeNode ReadAV@NULL (98882320ea2b731f10f353b23849bd6a) (1.24 KB, text/html)
2010-07-27 02:53 PDT, Berend-Jan Wever
no flags Details
Repro - WebCore::ApplyStyleCommand::removeInlineStyle ReadAV@NULL (18fa82a60d48c7965db66b916a546328) (357 bytes, text/html)
2010-07-27 03:02 PDT, Berend-Jan Wever
no flags Details
Repro - WebCore::executeToggleStyleInList ReadAV@NULL (95ebfffa464da44f33912d9597442825) (354 bytes, text/html)
2010-07-27 03:04 PDT, Berend-Jan Wever
no flags Details
fixes the crash (20.76 KB, patch)
2010-07-27 15:24 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (18.01 KB, patch)
2010-08-03 17:22 PDT, Ryosuke Niwa
darin: 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 2009-12-21 07:17:57 PST
Created attachment 45327 [details]
Repro - WebCore::caretMaxOffset ReadAV@NULL (5785e692fef72522c3ee976bff525d98)

Repro:
<BODY></BODY>
<SCRIPT>
  document.execCommand("SelectAll","");
  document.designMode= "on";
  document.execCommand("InsertHorizontalRule","");
  document.execCommand("CreateLink",false, 171);
  document.execCommand("InsertorderedList",false,undefined);
  document.execCommand("Unlink",5);
  document.execCommand("Indent",false,"");
  document.designMode="";
  document.execCommand("Undo",false,"");
  document.designMode="on";
  document.execCommand("ForeColor",false,3);
</SCRIPT>
Comment 1 Berend-Jan Wever 2009-12-30 03:24:12 PST
I think the below is a very similar bug, probably same root cause:
WebCore::highestAncestor ReadAV@NULL (6fd56a0a3c08931c7b81ea35e70a582a)
<BODY></BODY>
<SCRIPT>
  document.designMode="on";
  document.execCommand("selectall",Infinity);
  document.execCommand("subscript",false-5);
  document.execCommand("Indent",false,1);
  document.designMode="";
  document.execCommand("Undo","");
  document.designMode="on";
  document.execCommand("JustifyNone",false,"");
</SCRIPT>
I cannot seem to reproduce it in recent builds of Chromium, just reporting in case.
Comment 2 Mike Moretti 2009-12-30 09:33:12 PST
Created attachment 45668 [details]
reduced test case with comments

I've been looking into this bug and have a question.  What is the expected functionality of execCommand when there is no "contenteditable" field and "designmode" is off?  Should it do anything at all in this case?  Should only certain commands be allowed to execute?  I know there is a command table with specific "enabled" checks specified in it, but is it correct for the undo case?  enabledUndo seems to only call "canUndo" on the frame; shouldn't it also be checking if enabledInEditableText?  I.e., should undo be disabled for the case when there is no editable field or we're not in designmode (as in this test case), or does it need to function (and the crash really need to be fixed)?
Comment 3 Berend-Jan Wever 2009-12-31 02:25:30 PST
*** Bug 32822 has been marked as a duplicate of this bug. ***
Comment 4 Berend-Jan Wever 2009-12-31 02:32:09 PST
*** Bug 32424 has been marked as a duplicate of this bug. ***
Comment 5 Berend-Jan Wever 2009-12-31 02:32:19 PST
*** Bug 32425 has been marked as a duplicate of this bug. ***
Comment 6 Berend-Jan Wever 2009-12-31 02:32:39 PST
*** Bug 33049 has been marked as a duplicate of this bug. ***
Comment 7 Berend-Jan Wever 2010-03-05 07:16:18 PST
Appears to be fixed - I cannot repro in Chrome 4.0.249.89 unknown (38071)
Comment 8 Berend-Jan Wever 2010-03-12 09:12:50 PST
It appears this is not completely fixed; I am finding new examples such as this one:

<BODY></BODY>
<SCRIPT>
  document.designMode = "on";
  document.execCommand("selectall");
  document.execCommand("InsertParagraph");
  document.execCommand("inserthorizontalrule");
  document.designMode = "";
  document.execCommand("Undo");
  document.designMode = "on";
  document.execCommand("Strikethrough");
</SCRIPT>

Causes:
id:          WebCore::executeToggleStyleInList ReadAV@NULL (95ebfffa464da44f33912d9597442825)
description: Attempt to read from NULL pointer in WebCore::executeToggleStyleInList
stack:       WebCore::executeToggleStyleInList
             WebCore::executeStrikethrough
             WebCore::Editor::Command::execute
             WebCore::Document::execCommand
             WebCore::DocumentInternal::execCommandCallback
             v8::internal::HandleApiCallHelper<0>
             v8::internal::Builtin_HandleApiCall
             v8::internal::Invoke
             v8::internal::Execution::Call
             v8::Script::Run
             WebCore::V8Proxy::runScript
             WebCore::V8Proxy::evaluate
             WebCore::ScriptController::evaluate
             WebCore::ScriptController::executeScript
             WebCore::ScriptController::executeScript
             WebCore::ScriptController::executeIfJavaScriptURL
             WebCore::FrameLoader::changeLocation
             WebCore::RedirectScheduler::timerFired
             WebCore::Timer<WebCore::RedirectScheduler>::fired
             WebCore::ThreadTimers::sharedTimerFiredInternal
             WTF::ThreadSpecific<WebCore::ThreadGlobalData>::operator WebCore::ThreadGlobalData *
             MessageLoop::RunTask
             MessageLoop::DoWork
             base::MessagePumpDefault::Run
             MessageLoop::RunInternal
             MessageLoop::Run
             RendererMain
             ChromeMain
             MainDllLoader::Launch
             wWinMain
             __tmainCRTStartup
             BaseProcessStart+0x23
Comment 9 Berend-Jan Wever 2010-05-04 01:21:48 PDT
Forgot to update the status in previous comment :(
Comment 10 Berend-Jan Wever 2010-07-05 03:44:51 PDT
I just filed bug 41601, which is probably a duplicate of this and indicates this may be a security issue.
Comment 11 Berend-Jan Wever 2010-07-27 02:41:56 PDT
Created attachment 62665 [details]
Repro - WebCore::Node::hasTagName ReadAV@NULL (106220ce302426f16b811b1cc735e87a)

I'm going to be uploading repros for all crashes I find with this general structure:

designmode = on
execCommand...
designmode = off
execCommand(undo)
designmode = on
execCommand...

I expect they are all manifestations of similar issues and that the code needs an overhaul, given the number of bugs I am finding. (But I cannot exclude that this may all be caused by only one bug in the code).
Comment 12 Berend-Jan Wever 2010-07-27 02:47:19 PDT
Created attachment 62666 [details]
Repro - WebCore::ApplyStyleCommand::applyBlockStyle ReadAV@NULL (eba56e4a5c0f11cedaba7aea0468d82a)
Comment 13 Berend-Jan Wever 2010-07-27 02:48:17 PDT
Created attachment 62667 [details]
Repro - WebCore::ReplaceSelectionCommand::doApply ReadAV@NULL (badacdea771d24a7e25e9ab3859da7a2)
Comment 14 Berend-Jan Wever 2010-07-27 02:53:35 PDT
Created attachment 62668 [details]
Repro - WebCore::positionInParentBeforeNode ReadAV@NULL (98882320ea2b731f10f353b23849bd6a)
Comment 15 Berend-Jan Wever 2010-07-27 03:02:59 PDT
Created attachment 62669 [details]
Repro - WebCore::ApplyStyleCommand::removeInlineStyle ReadAV@NULL (18fa82a60d48c7965db66b916a546328)
Comment 16 Berend-Jan Wever 2010-07-27 03:04:35 PDT
Created attachment 62670 [details]
Repro - WebCore::executeToggleStyleInList ReadAV@NULL (95ebfffa464da44f33912d9597442825)
Comment 17 Roland Steiner 2010-07-27 03:08:47 PDT
[CC'ing rniwa for added editing-fu.]

I had a brief look at the bug previously - the fundamental problem is what Mike Moretti described in comment #2. The problem is that execCommand("undo") is prevented, which results in an inconsistent state.

As Mike wrote, the question is how to best handle this case (and also the case where the contentEditable attribute is being modified).
Comment 18 Darin Adler 2010-07-27 10:01:03 PDT
Lets not overstate the case here. I think we can fix these all in a pretty straightforward manner without an "overhaul".
Comment 19 Darin Adler 2010-07-27 10:01:16 PDT
But having all in a single bug report may be impractical.
Comment 20 Ryosuke Niwa 2010-07-27 11:54:41 PDT
Every reproduction steps until comment #11 do not crash on TOT.
Comment 21 Darin Adler 2010-07-27 11:58:35 PDT
So it seems we have crash logs here, but we are not clear on which reproduction steps go with those logs. There’s a chance this is all fixed already on TOT, but we can’t be sure until we have a particular set of steps to reproduce that definitely cause a crash.

Having all these crashes in one bug report is indeed causing confusion and making it hard to narrow this down, so we probably need to make a simpler bug report with one set of steps to reproduce and one crash.

If these do indeed have security implications, then this bug should be in the security component instead of in the open.
Comment 22 Ryosuke Niwa 2010-07-27 13:22:47 PDT
It seems like the crash in #11 is caused by our updating selection after undo becauese the nodes pointed by the selection are not in the document.
Comment 23 Ryosuke Niwa 2010-07-27 14:47:05 PDT
#12 doesn't reproduce, but #11, #13-16 are the same type of crashes. They're all caused by changeSelectionAfterCommand incorrectly updating the selection.  A patch coming.
Comment 24 Ryosuke Niwa 2010-07-27 15:24:01 PDT
Created attachment 62756 [details]
fixes the crash

Fixed the crash by adding a check in changeSelectionAfterCommand.  The fix is simple and small.  I included all tests that crash TOT.
Comment 25 Berend-Jan Wever 2010-07-28 08:14:51 PDT
@Darin:
* Sorry for the confusion: I am NOT suggesting we NEED to overhaul the code. I am hoping it is one bug in the code
  triggered in various ways. However, considering how many crashes I am finding, I am worried that the code may need
  a little bit extra attention. So, I was trying to suggest that the code is reviewed to make sure it is in good 
  overall shape before closing this bug.
* About all the repros: If this is indeed only one bug in the code, we would know when a fix stops all repros from
  crashing. If it turns out that a fix does not fix all crashes, you can eitehr take the repros that still trigger
  crashes and move them to a new bug, or continue to fix the code until all crashes are gone.
  Because I do not know how many bugs there actually are in the code, I have opted to put all repros in one bug: it is
  the easiest way to make sure we do not lose track of any crashes, so all of them get fixed.
* Everything I uploaded that starts with "Repro - " is what I would call a repro: each contains HTML and JavaScript
  that triggers a crash for me in latest Chromium. I'm not sure what you mean by "crash log".
* I have been fuzzing this for months and only found NULL pointers. I am not saying there is a security issue hiding
  somewhere in the code, but I have failed to find any, hence marked as non-security.

@Ryosuke:
* I assume this was indeed one bug that could be triggered in various ways, causing the NULL pointers to happen in
  various other parts of the code and that this is indeed not a security issue?
* I also assuem that your fix prevents all repros from crashing in one fell swoop?
Comment 26 Ryosuke Niwa 2010-07-28 09:59:02 PDT
On second thought, I don't think my approach was right here.  Let me explain my thoughts as I rely comment #25.

(In reply to comment #25)
> @Darin:
> * Sorry for the confusion: I am NOT suggesting we NEED to overhaul the code. I am hoping it is one bug in the code
>   triggered in various ways. However, considering how many crashes I am finding, I am worried that the code may need
>   a little bit extra attention. So, I was trying to suggest that the code is reviewed to make sure it is in good 
>   overall shape before closing this bug.

Indeed, all bugs were triggered by undo restoring selection incorrectly.  Namely, it was setting the start or the end of selection to a node that's not attached to the document but solely kept alive for undo/redo purposes.  And we probably need to come up with a way to cleanly deal with undo/redo inconsistency as well as JavaScript interference during executing composite editing commands.

> * About all the repros: If this is indeed only one bug in the code, we would know when a fix stops all repros from
>   crashing. If it turns out that a fix does not fix all crashes, you can eitehr take the repros that still trigger
>   crashes and move them to a new bug, or continue to fix the code until all crashes are gone.
>   Because I do not know how many bugs there actually are in the code, I have opted to put all repros in one bug: it is
>   the easiest way to make sure we do not lose track of any crashes, so all of them get fixed.

So here's a thing.  This bug report addresses multiple bugs simultaneously by its own nature because the crashes we are seeing here are caused because
1. Undo restores selection incorrectly to detached nodes. (This does not cause crash by itself).
2. Some editing command can't handle orphaned selections and crashes.

My patch certainly fix the problem 1 but does not fix problem 2. What we really need to do here is to go look at each test, and find a way to reproduce the same problem without using undo, e.g. by creating orphaned selection artificially.  I'm not sure if this is possible yet, so I'll investigate into this.

> * Everything I uploaded that starts with "Repro - " is what I would call a repro: each contains HTML and JavaScript
>   that triggers a crash for me in latest Chromium. I'm not sure what you mean by "crash log".

I think he meant the stack trace. 

> * I have been fuzzing this for months and only found NULL pointers. I am not saying there is a security issue hiding
>   somewhere in the code, but I have failed to find any, hence marked as non-security.

Indeed, there's no security implications as far as I know.

> @Ryosuke:
> * I assume this was indeed one bug that could be triggered in various ways, causing the NULL pointers to happen in
>   various other parts of the code and that this is indeed not a security issue?

Yes, the details explained above.

> * I also assuem that your fix prevents all repros from crashing in one fell swoop?

So my patch has pros and cons.  My patch on one hand fixes the crashes by solving the problem 1 but also hides problem 2.  But editing commands shouldn't be crashing regardless.  So I'll try to come up with tests that cause the same kind of crash for each case, and then prepare a separate test that ensures that undo doesn't restore selection incorrectly.
Comment 27 Ryosuke Niwa 2010-07-28 10:01:43 PDT
Ugh... I misread your comment.

> * I assume this was indeed one bug that could be triggered in various ways, causing the NULL pointers to happen in
>   various other parts of the code and that this is indeed not a security issue?

No, the opposite is true.  There was one non-crash bug that triggered various crash bugs.
Comment 28 Ryosuke Niwa 2010-07-28 10:23:22 PDT
On another second thought, if either end of selection points to a detached node, it doesn't make much sense to call almost any editing command. So maybe my fix is okay after all.

Adding more editing people to see if anyone can comment on this point.
Comment 29 Darin Adler 2010-07-28 11:00:01 PDT
(In reply to comment #28)
> On another second thought, if either end of selection points to a detached node, it doesn't make much sense to call almost any editing command. So maybe my fix is okay after all.

Yes, that makes sense. The editing command layer can just check for this type of selection and not do any command at all. That should get rid of the crashes, and then we may still have bugs where selection gets into this state, but they should be non-crashing issues.
Comment 30 Ryosuke Niwa 2010-07-28 11:18:46 PDT
(In reply to comment #29)
> (In reply to comment #28)
> > On another second thought, if either end of selection points to a detached node, it doesn't make much sense to call almost any editing command. So maybe my fix is okay after all.
> 
> Yes, that makes sense. The editing command layer can just check for this type of selection and not do any command at all. That should get rid of the crashes, and then we may still have bugs where selection gets into this state, but they should be non-crashing issues.

So do you want me to add two fixes here?  One in restoring selection and another in EditCommand to prevent calling any command if selection is orphaned.  But I feel like that might break some commands because some simple commands don't refer to selection, and some composite commands may call these simple commands before restoring the selection properly.  What do you think?
Comment 31 Darin Adler 2010-07-28 11:26:17 PDT
I don’t think a fix in EditCommand is necessarily a good idea. The individual commands should probably have the fixes instead. I want the check to be as close as possible to the code that relies on assumption in the check, not in some higher level place making assumptions about all editing commands.

However, I do think that there could be two types of fixes.

1) Mechanical fixes that prevent the code from crashing in a way that’s clearly correct and closely related to the code that would crash, but may give suboptimal editing behavior.

2) Higher level fixes that give more logical results in strange cases where the low level fixes might lead to unexpected behavior and we can do better.
Comment 32 Ryosuke Niwa 2010-07-28 11:46:04 PDT
(In reply to comment #31)
> I don’t think a fix in EditCommand is necessarily a good idea. The individual commands should probably have the fixes instead.

We're on the same page.

> I want the check to be as close as possible to the code that relies on assumption in the check, not in some higher level place making assumptions about all editing commands.

Yeah, I'd like that too.

> However, I do think that there could be two types of fixes.
> 
> 1) Mechanical fixes that prevent the code from crashing in a way that’s clearly correct and closely related to the code that would crash, but may give suboptimal editing behavior.

For example #11 crash happens after http://trac.webkit.org/browser/trunk/WebCore/editing/ReplaceSelectionCommand.cpp#824 because DeleteSelection clears up the selection and #825 obtains a null visible position.  This clearly is a problem but I'm not sure if it's the scope of this bug to fix this kind of bug.  We might want to file a separate bug or wait until some test in which this condition holds (i.e. selection existed before the call to DeleteSelection but became null afterwards) by putting ASSERT here for now.

> 2) Higher level fixes that give more logical results in strange cases where the low level fixes might lead to unexpected behavior and we can do better.

By higher level fixes, you mean something like I did in my patch to prevent undo from restoring bad selection?  Or you mean that exiting early at the beginning of each command that relies on selection?
Comment 33 Ryosuke Niwa 2010-08-03 16:53:49 PDT
SkyLined, could you unmark this bug as a security-related?  As far as I'm concerned, there are no "exploitable" crashes here.
Comment 34 Ryosuke Niwa 2010-08-03 17:22:39 PDT
Created attachment 63394 [details]
Patch
Comment 35 Ryosuke Niwa 2010-08-03 17:25:43 PDT
Darin, I added check for various editing commands.  But it turns out that at least 2 tests I'm adding crashes inside the changeSelectionAfterCommand when it tries to create a range to verify the selection so my change in changeSelectionAfterCommand is still necessary to fix this bug.
Comment 36 Darin Adler 2010-08-24 12:49:03 PDT
Comment on attachment 63394 [details]
Patch

> +    if (!m_selectionToDelete.isRange() || m_selectionToDelete.start().isOrphan() || m_selectionToDelete.end().isOrphan())

I'd like to see a helper function for this so we don't have to repeat these three checks every time. One version for the isNone and another for the isRange variant I guess.
Comment 37 Ryosuke Niwa 2010-08-25 12:16:40 PDT
Thanks for the review, Darin.  I really appreciate it.

(In reply to comment #36)
> (From update of attachment 63394 [details])
> > +    if (!m_selectionToDelete.isRange() || m_selectionToDelete.start().isOrphan() || m_selectionToDelete.end().isOrphan())
> 
> I'd like to see a helper function for this so we don't have to repeat these three checks every time. One version for the isNone and another for the isRange variant I guess.

Will add isNonOrphanedRange and isNonOrphanedCaretOrRange to VisibleSelection.
Comment 38 Ryosuke Niwa 2010-08-25 12:19:40 PDT
Committed r66032: <http://trac.webkit.org/changeset/66032>
Comment 39 David Kilzer (:ddkilzer) 2011-03-15 10:07:23 PDT
<rdar://problem/9135704>