30116 – WebCore::InsertLineBreakCommand::shouldUseBreakElement ReadAV@NULL

Berend-Jan Wever

Reported 2009-10-06 04:06:00 PDT

This HTML triggers a NULL pointer read access violation in Chromium ToT about 50% of the time, but does appear to affect Safari 4: <HTML> <BODY onload="go()"></BODY> <SCRIPT> function go() { document.designMode = "on"; document.execCommand("SelectAll"); document.execCommand("InsertLineBreak"); document.execCommand("Indent"); document.execCommand("insertparagraph"); } </SCRIPT> </HTML> Here's some potentially useful info: Attempt to read from a NULL pointer (+0x1C), instruction: 6c96e2b7 8b411c mov eax,dword ptr [ecx+1Ch] Registers: eax=003ce9f8 ebx=ffffffff ecx=00000000 edx=00000000 esi=0496c900 edi=0496c900 esp=003ce9ec ebp=003cea08 eip=6c96e2b7 Stack: ChildEBP RetAddr 003cea08 6c96e40d chrome_6bed0000!WebCore::InsertLineBreakCommand::shouldUseBreakElement(class WebCore::Position * insertionPos = 0x003ceb20)+0x17 003ceb30 6c6f8967 chrome_6bed0000!WebCore::InsertLineBreakCommand::doApply(void)+0x10d 003ceb40 6c8b66ec chrome_6bed0000!WebCore::EditCommand::apply(void)+0x67 003ceb58 6c96fa9b chrome_6bed0000!WebCore::CompositeEditCommand::applyCommandToComposite(class WTF::PassRefPtr<WebCore::EditCommand> cmd = class WTF::PassRefPtr<WebCore::EditCommand>)+0x1c 003cec58 6c6f8967 chrome_6bed0000!WebCore::InsertParagraphSeparatorCommand::doApply(void)+0xb6b 003cec68 6c8b66ec chrome_6bed0000!WebCore::EditCommand::apply(void)+0x67 003cec80 6c875b72 chrome_6bed0000!WebCore::CompositeEditCommand::applyCommandToComposite(class WTF::PassRefPtr<WebCore::EditCommand> cmd = class WTF::PassRefPtr<WebCore::EditCommand>)+0x1c 003cec90 6c6f8967 chrome_6bed0000!WebCore::TypingCommand::insertParagraphSeparator(void)+0x32 003ceca0 6c6f8c7e chrome_6bed0000!WebCore::EditCommand::apply(void)+0x67 003cecac 6c8777f7 chrome_6bed0000!WebCore::applyCommand(class WTF::PassRefPtr<WebCore::EditCommand> command = class WTF::PassRefPtr<WebCore::EditCommand>)+0xe 003cecc0 6c6f8fb1 chrome_6bed0000!WebCore::TypingCommand::insertParagraphSeparator(class WebCore::Document * document = 0x00f52d40)+0x77 003ceccc 6c6fa310 chrome_6bed0000!WebCore::executeInsertParagraph(class WebCore::Frame * frame = 0x0152a000, class WebCore::Event * __formal = 0x00000000, WebCore::EditorCommandSource __formal = CommandFromDOM (1), class WebCore::String * __formal = 0x003ced48)+0x11 003cecec 6c67ba17 chrome_6bed0000!WebCore::Editor::Command::execute(class WebCore::String * parameter = 0x003ced48, class WebCore::Event * triggeringEvent = 0x00000000)+0x90 003ced14 6c739da8 chrome_6bed0000!WebCore::Document::execCommand(class WebCore::String * commandName = 0x003ced38, bool userInterface = false, class WebCore::String * value = 0x003ced48)+0x57 003ced3c 6c429a88 chrome_6bed0000!WebCore::DocumentInternal::execCommandCallback(class v8::Arguments * args = 0x0469d2e0)+0xe8

Alexey Proskuryakov

Comment 1 2009-10-07 21:50:32 PDT

Stack trace from a semi-recent local build of Mac WebKit: #0 0x0217b481 in WebCore::Node::renderer at Node.h:384 #1 0x024dd1ed in WebCore::InsertLineBreakCommand::shouldUseBreakElement at InsertLineBreakCommand.cpp:86 #2 0x024dd397 in WebCore::InsertLineBreakCommand::doApply at InsertLineBreakCommand.cpp:104 #3 0x0234eff9 in WebCore::EditCommand::apply at EditCommand.cpp:91 #4 0x0214f329 in WebCore::CompositeEditCommand::applyCommandToComposite at CompositeEditCommand.cpp:99 #5 0x024e10a8 in WebCore::InsertParagraphSeparatorCommand::doApply at InsertParagraphSeparatorCommand.cpp:159 #6 0x0234eff9 in WebCore::EditCommand::apply at EditCommand.cpp:91 #7 0x0214f329 in WebCore::CompositeEditCommand::applyCommandToComposite at CompositeEditCommand.cpp:99 #8 0x02a6dbfb in WebCore::TypingCommand::insertParagraphSeparator at TypingCommand.cpp:377 #9 0x02a6e141 in WebCore::TypingCommand::doApply at TypingCommand.cpp:268 #10 0x0234eff9 in WebCore::EditCommand::apply at EditCommand.cpp:91 #11 0x0234f08d in WebCore::applyCommand at EditCommand.cpp:217 #12 0x02a6e3be in WebCore::TypingCommand::insertParagraphSeparator at TypingCommand.cpp:231 #13 0x0235ce85 in WebCore::executeInsertParagraph at EditorCommand.cpp:527 #14 0x0235b1eb in WebCore::Editor::Command::execute at EditorCommand.cpp:1504 #15 0x02262cbf in WebCore::Document::execCommand at Document.cpp:3324 #16 0x02565f6e in WebCore::jsDocumentPrototypeFunctionExecCommand at JSDocument.cpp:1870

Alexey Proskuryakov

Comment 2 2009-10-07 21:51:56 PDT

<rdar://problem/7285936>

Ryosuke Niwa

Comment 3 2009-12-17 17:28:44 PST

Cannot reproduce the bug with the reported HTML.

Lei Zhang

Comment 4 2010-04-30 14:40:37 PDT

The test case from http://code.google.com/p/chromium/issues/detail?id=37011 crashes the renderer with this stack trace using Chromium ToT @ r46031.

Ryosuke Niwa

Comment 5 2010-06-04 11:14:12 PDT

Created attachment 57897 [details] reduction Reduction steps: 1. Open the page 2. Trigger CR(13) keydown event inside textarea 3. WebKit crashes (TOT 60682) The exact problem is that selection is invalidated inside InsertLineBreak. So inside InsertLineBreakCommand::doApply(), caret is null and the first place in which it assumes non-null value (shouldUseBreakElement on line 103 of InsertLineBreakCommand.cpp) results in null-pointer access.

Tony Chang

Comment 6 2010-06-07 02:22:49 PDT

Created attachment 58003 [details] Patch

Tony Chang

Comment 7 2010-06-07 02:35:17 PDT

Ryosuke's analysis is correct. The textarea is hidden in keydown, but we try to use the visible position when inserting the line break. There is no visible position since the textarea is hidden, so we dereference a null pointer. This patch just does a null pointer check on the visible position. A few notes: - This means that when the textarea is hidden, we don't insert the linebreak because we bail out early. - This doesn't match what happens when you type any other character, which we can insert because it goes through a different code path that doesn't depend on visible positions. - I tried to make this code not depend on visible positions (pretty straight forward), but then you get a different behavior if you have a selection or not. Whenever there's a selection, we bail out in the selection.isNone() check. To try to stay consistent with this behavior, I had us just bail out rather than trying to insert the line break. - Firefox is able to insert the line break in this test case.

Ryosuke Niwa

Comment 8 2010-06-07 11:52:56 PDT

+jparent & +ojan since they're knowledgeable about how rich text edit should behave. +adele since she's knowledgeable about forms. (In reply to comment #7) > Ryosuke's analysis is correct. The textarea is hidden in keydown, but we try to use the visible position when inserting the line break. There is no visible position since the textarea is hidden, so we dereference a null pointer. This seems to be a yet another problem caused by VisiblePosition. Julie & Ojan, does it make sense for us to ignore text input if the textarea is hidden? > This patch just does a null pointer check on the visible position. I'm not sure if InsertLikeBreakCommand is the right place to deal with this problem. It seems like we should be stop propagating all text input commands or similar commands whenever VisiblePosition is null. > - Firefox is able to insert the line break in this test case. That's what I thought. Firefox doesn't canonicalize positions so this shouldn't be a problem for them. On the other hand, Internet Explorer won't add the line break in this test so new behavior matches MSIE.

Tony Chang

Comment 9 2010-06-07 17:16:46 PDT

Created attachment 58097 [details] Patch

Tony Chang

Comment 10 2010-06-07 17:19:29 PDT

Ok, I changed my mind. I think we should try to always allow the text input to happen. If the site wants to not allow the text input, they can preventDefault the event. This version of the patch allows a line break to be inserted if there is no selection and the textarea is hidden. It doesn't change the behavior that if there is a selection, pressing enter won't insert a line break but it will delete the selected text. This seems like a bug, but can be fixed separately (although it's not obvious to me how to do that).

Ojan Vafai

Comment 11 2010-06-08 15:19:56 PDT

Created attachment 58192 [details] Test case for typing into visibility:hidden element It's hard to write a testcase for this, but here's what I came up with. Basically, it's possible to have a focused, but visibility:hidden element. In Gecko, that element still receives key events and allows text input. In WebKit, it receives key events, but only does one text insertion. My opinion is that text insertion (include line-breaks) should work. If the element is receiving key events, it should also allow text insertion. I can also think of valid use-cases enabled by allowing text insertion in this case. Contrast this to Gecko's behavior if we do the same test, but use display:none instead of visibility:hidden. Gecko doesn't send any key events or do text insertion. Essentially it acts as if the element isn't focused. In WebKit, we still send it key events. Which, I think is a bug. Both browsers still list the textarea as document.activeElement, but that also seems like a bug. In short, except for the document.activeElement bug, I think Gecko's behavior is better and we should aim to move in that direction.

Ojan Vafai

Comment 12 2010-06-08 15:29:40 PDT

https://bugs.webkit.org/show_bug.cgi?id=40338 For the display:none case.

Ojan Vafai

Comment 13 2010-06-08 16:02:23 PDT

visibility:hidden case is bug 40342. This bug can focus on the crash.

Tony Chang

Comment 14 2010-06-08 16:35:17 PDT

Sounds good to me. That sounds like we want the second patch, right?

Ojan Vafai

Comment 15 2010-06-08 16:44:50 PDT

Comment on attachment 58097 [details] Patch Doesn't this make it so that we no longer canonicalize the position we're going to use when we hit enter? I wonder if a better (albeit harder) fix would be to make it so that "VisiblePosition caret(selection.visibleStart())" doesn't create a VisiblePosition with a null deepEquivalent. Specifically, VisiblePosition::canonicalPosition would need to not return null. If I understand this all correctly, we return null because we skip over visibility:hidden nodes when looking for the canonical position. This makes sense in general, but I don't think it makes sense when the rootEditableElement is the visibility:hidden node. Maybe we should change that logic. WDYT?

Tony Chang

Comment 16 2010-06-09 00:10:14 PDT

Created attachment 58216 [details] Patch

Tony Chang

Comment 17 2010-06-09 00:17:40 PDT

(In reply to comment #15) > (From update of attachment 58097 [details]) > Doesn't this make it so that we no longer canonicalize the position we're going to use when we hit enter? I'm not sure I understand, which canonicalization isn't happening? > I wonder if a better (albeit harder) fix would be to make it so that "VisiblePosition caret(selection.visibleStart())" doesn't create a VisiblePosition with a null deepEquivalent. Specifically, VisiblePosition::canonicalPosition would need to not return null. If I understand this all correctly, we return null because we skip over visibility:hidden nodes when looking for the canonical position. This makes sense in general, but I don't think it makes sense when the rootEditableElement is the visibility:hidden node. Maybe we should change that logic. WDYT? VisiblePosition::canonicalPosition returns null because PositionIterator checks for visibility. Changing this would be a big change which seems beyond the scope of this bug. But maybe you're asking about only changing visibleStart some of the time? That also seems tricky to implement (e.g. what happens if the rootEditableElement is visible but some child editable node is hidden)? I found another crasher except with contenteditable areas, so I've uploaded a new patch. If the goal of this bug is to just fix the crash, then perhaps we should just return early in both cases (or even return in EventHandler::defaultTextInputEventHandler.

Ojan Vafai

Comment 18 2010-06-09 16:08:01 PDT

(In reply to comment #17) > (In reply to comment #15) > > (From update of attachment 58097 [details] [details]) > > Doesn't this make it so that we no longer canonicalize the position we're going to use when we hit enter? > > I'm not sure I understand, which canonicalization isn't happening? Before this patch, pos pointed to the deepEquivalent (canonicalized node). After this patch, it just points to m_start, which may or may not be canonicalized. Right? > > I wonder if a better (albeit harder) fix would be to make it so that "VisiblePosition caret(selection.visibleStart())" doesn't create a VisiblePosition with a null deepEquivalent. Specifically, VisiblePosition::canonicalPosition would need to not return null. If I understand this all correctly, we return null because we skip over visibility:hidden nodes when looking for the canonical position. This makes sense in general, but I don't think it makes sense when the rootEditableElement is the visibility:hidden node. Maybe we should change that logic. WDYT? > > VisiblePosition::canonicalPosition returns null because PositionIterator checks for visibility. Changing this would be a big change which seems beyond the scope of this bug. But maybe you're asking about only changing visibleStart some of the time? That also seems tricky to implement (e.g. what happens if the rootEditableElement is visible but some child editable node is hidden)? I think it's OK to special-case the rootEditableElement here. If a child isn't visible, the typing will still go through just fine. It just won't go inside the visibility:hidden element. It'll go in one of it's siblings/parents/cousins. I think it would be sufficient for the visibility check in PositionIterator::isCandidate and Position::isCandidate to not return false there if the renderer is the renderer for the rootEditableElement, but I'm not sure. > I found another crasher except with contenteditable areas, so I've uploaded a new patch. This looks like we'll do the wrong thing (use a paragraph instead of a BR) if you hit enter inside a table that's not visible. That's much better than crashing though. Maybe add a FIXME pointing to bug 40342? > If the goal of this bug is to just fix the crash, then perhaps we should just return early in both cases (or even return in EventHandler::defaultTextInputEventHandler. I think that would be fine if you include a FIXME pointing to bug 40342. We should probably early return here though, not higher up in defaultTextInputEventHandler as we'll want to fix this someday.

Tony Chang

Comment 19 2010-06-09 18:36:00 PDT

Created attachment 58320 [details] Patch

Tony Chang

Comment 20 2010-06-09 18:45:30 PDT

Ojan, I can't tell if you're reviewing the patches since you're not changing the review flag. Inline comments on that patch are more helpful than just responding to the comments. (In reply to comment #18) > Before this patch, pos pointed to the deepEquivalent (canonicalized node). After this patch, it just points to m_start, which may or may not be canonicalized. Right? Sure, but what canonicalization needs to happen? I.e., do you have an example test case that this breaks? Since this is just plain text, I'm not sure we need to canonicalize here. > > VisiblePosition::canonicalPosition returns null because PositionIterator checks for visibility. Changing this would be a big change which seems beyond the scope of this bug. But maybe you're asking about only changing visibleStart some of the time? That also seems tricky to implement (e.g. what happens if the rootEditableElement is visible but some child editable node is hidden)? > > I think it's OK to special-case the rootEditableElement here. If a child isn't visible, the typing will still go through just fine. It just won't go inside the visibility:hidden element. It'll go in one of it's siblings/parents/cousins. I think it would be sufficient for the visibility check in PositionIterator::isCandidate and Position::isCandidate to not return false there if the renderer is the renderer for the rootEditableElement, but I'm not sure. I mean something like this: <div contenteditable> <p>foo bar</p> <p id="to-be-made-hidden"> | </p> </div> If the cursor is at |, and the keydown event makes "to-be-made-hidden" hidden, what should happen? Currently, if the node with the cursor disappears, the selection is lost. I think it would be weird if the cursor jumped to the foo bar paragraph. Anyway, shouldn't this be discussed on a different bug? > This looks like we'll do the wrong thing (use a paragraph instead of a BR) if you hit enter inside a table that's not visible. That's much better than crashing though. Maybe add a FIXME pointing to bug 40342? done.

Ojan Vafai

Comment 21 2010-06-10 17:16:43 PDT

> Ojan, I can't tell if you're reviewing the patches since you're not changing the review flag. It's not clear to me whether this patch is incorrect or correct, so I don't think it makes sense for me to r-/r+. Leaving it r? means another reviewer can come in and review. In the meantime, I'm trying to understand the code in question well enough to give an r-/r+. > Inline comments on that patch are more helpful than just responding to the comments. Not sure what you're asking for here. The code hasn't changed, so there's only comments to respond to. > > Before this patch, pos pointed to the deepEquivalent (canonicalized node). After this patch, it just points to m_start, which may or may not be canonicalized. Right? > > Sure, but what canonicalization needs to happen? I.e., do you have an example test case that this breaks? Since this is just plain text, I'm not sure we need to canonicalize here. Is this code really only hit in plaintext regions? Doesn't non-plaintext contentEditable hit this codepath as well? If it really is just for plain text, then I think we don't need to worry about canonicalization. If this code gets hit in rich-text, I don't have a clear example in my head where this would break. I think it would break if selection.start() is ever not canonicalized when it the selection gets set. Not sure if that's possible. By "break" here, I just mean that it would do something different than the old code. > Anyway, shouldn't this be discussed on a different bug? Sure, that discussion can happen on bug 40342.

Tony Chang

Comment 22 2010-06-10 18:00:33 PDT

(In reply to comment #21) > > > Before this patch, pos pointed to the deepEquivalent (canonicalized node). After this patch, it just points to m_start, which may or may not be canonicalized. Right? > > > > Sure, but what canonicalization needs to happen? I.e., do you have an example test case that this breaks? Since this is just plain text, I'm not sure we need to canonicalize here. > > Is this code really only hit in plaintext regions? Doesn't non-plaintext contentEditable hit this codepath as well? If it really is just for plain text, then I think we don't need to worry about canonicalization. Yes, the change to InsertLineBreakCommand is for plain text and the change to InsertParagraphSeparatorCommand is for rich text. Specifically, the decision is made here: http://trac.webkit.org/browser/trunk/WebCore/page/EventHandler.cpp#L2596 And TextEvent::isLineBreak() is set by the third parameter here: http://trac.webkit.org/browser/trunk/WebCore/editing/EditorCommand.cpp#L515 > Not sure what you're asking for here. The code hasn't changed, so there's only comments to respond to. I was unclear to me if the FIXMEs you mentioned in comment 18 were both about the same code or if you wanted 2 FIXMEs.

Tony Chang

Comment 23 2010-06-10 18:02:47 PDT

(In reply to comment #21) > Is this code really only hit in plaintext regions? Doesn't non-plaintext contentEditable hit this codepath as well? If it really is just for plain text, then I think we don't need to worry about canonicalization. Oh, I see, you're saying in other cases, we may need to canonicalize. Hmm, maybe, but it's not obvious to me that it is necessary without a test case.

Tony Chang

Comment 24 2010-06-10 18:04:20 PDT

(In reply to comment #23) > (In reply to comment #21) > > Is this code really only hit in plaintext regions? Doesn't non-plaintext contentEditable hit this codepath as well? If it really is just for plain text, then I think we don't need to worry about canonicalization. > > Oh, I see, you're saying in other cases, we may need to canonicalize. Hmm, maybe, but it's not obvious to me that it is necessary without a test case. And if we are concerned about that, then we should just go with the original patch that null checks |caret|. It won't insert a line, but that's bug 40342.

Ojan Vafai

Comment 25 2010-06-10 18:34:42 PDT

(In reply to comment #24) > (In reply to comment #23) > > (In reply to comment #21) > > > Is this code really only hit in plaintext regions? Doesn't non-plaintext contentEditable hit this codepath as well? If it really is just for plain text, then I think we don't need to worry about canonicalization. > > > > Oh, I see, you're saying in other cases, we may need to canonicalize. Hmm, maybe, but it's not obvious to me that it is necessary without a test case. Right. Unfortunately, this is a problem with the editing code in general. The test coverage isn't that great. So places where we make changes like this may cause regressions without breaking tests and the canonicalization code is really difficult to reason about. This code is not clearly wrong to me, but it does set off a red flag. I'm not sure what the correct path forward in cases like this are. My intuition is that we check this in as is and, if it causes a regression, it'll get reported and we can add a test + fix then. Adding Darin to this bug for his opinion on this. > And if we are concerned about that, then we should just go with the original patch that null checks |caret|. It won't insert a line, but that's bug 40342. I'm definitely OK with adding a null-check and a FIXME pointing to bug 40342. Not inserting the line-break here is wrong IMO, but it's obviously much better than crashing and definitely won't cause regressions.

Ojan Vafai

Comment 26 2010-07-01 12:07:37 PDT

Comment on attachment 58320 [details] Patch http://wkrietveld.appspot.com/30116/diff/2001/3003 File LayoutTests/editing/inserting/return-key-in-hidden-field.html (right): http://wkrietveld.appspot.com/30116/diff/2001/3003#newcode13 LayoutTests/editing/inserting/return-key-in-hidden-field.html:13: } Nit: technically by webkit style this (one-line if) shouldn't have curly braces. http://wkrietveld.appspot.com/30116/diff/2001/3003#newcode30 LayoutTests/editing/inserting/return-key-in-hidden-field.html:30: layoutTestController.dumpAsText(); 28-30 duplicates 20-22, was that intentional? I think you can just remove this. http://wkrietveld.appspot.com/30116/diff/2001/3004 File WebCore/ChangeLog (right): http://wkrietveld.appspot.com/30116/diff/2001/3004#newcode7 WebCore/ChangeLog:7: Maybe add a comment saying that this causes text insertions on visibility:hidden elements to get ignored and add a link to bug 40342? http://wkrietveld.appspot.com/30116/diff/2001/3005 File WebCore/editing/InsertLineBreakCommand.cpp (right): http://wkrietveld.appspot.com/30116/diff/2001/3005#newcode97 WebCore/editing/InsertLineBreakCommand.cpp:97: VisiblePosition caret(pos); I thought the conclusion was to make this a null-check with a comment pointing to bug 40342 instead?

Darin Adler

Comment 27 2010-07-01 12:10:51 PDT

I find these reitveld-created bugs.webkit.org comments difficult to read.

Ojan Vafai

Comment 28 2010-07-01 14:30:44 PDT

(In reply to comment #27) > I find these reitveld-created bugs.webkit.org comments difficult to read. Filed bug 41486. Hopefully we can improve it.

Tony Chang

Comment 29 2010-07-07 09:44:47 PDT

Created attachment 60745 [details] Patch

WebKit Commit Bot

Comment 30 2010-07-08 21:35:33 PDT

Comment on attachment 60745 [details] Patch Clearing flags on attachment: 60745 Committed r62889: <http://trac.webkit.org/changeset/62889>

WebKit Commit Bot

Comment 31 2010-07-08 21:35:41 PDT

All reviewed patches have been landed. Closing bug.

Csaba Osztrogonác

Comment 32 2010-07-09 00:29:59 PDT

Qt specific expected result landed in http://trac.webkit.org/changeset/62905. This test say it passes if doesn't crash. It passes, but Qt has an extra newline character in its expected file.

Tony Chang

Comment 33 2010-07-09 09:57:05 PDT

(In reply to comment #32) > Qt specific expected result landed in http://trac.webkit.org/changeset/62905. > > This test say it passes if doesn't crash. It passes, but > Qt has an extra newline character in its expected file. That looks right to me. Thanks for fixing, sorry I wasn't around when the change landed.

Attachments
reduction (486 bytes, text/html) 2010-06-04 11:14 PDT, Ryosuke Niwa	no flags	Details
Patch (3.49 KB, patch) 2010-06-07 02:22 PDT, Tony Chang	no flags	Details Formatted Diff Diff
Patch (3.60 KB, patch) 2010-06-07 17:16 PDT, Tony Chang	no flags	Details Formatted Diff Diff
Test case for typing into visibility:hidden element (870 bytes, text/html) 2010-06-08 15:19 PDT, Ojan Vafai	no flags	Details
Patch (4.96 KB, patch) 2010-06-09 00:10 PDT, Tony Chang	no flags	Details Formatted Diff Diff
Patch (5.14 KB, patch) 2010-06-09 18:36 PDT, Tony Chang	no flags	Details Formatted Diff Diff
Patch (5.27 KB, patch) 2010-07-07 09:44 PDT, Tony Chang	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.