Bug 30116 - WebCore::InsertLineBreakCommand::shouldUseBreakElement ReadAV@NULL
Summary: WebCore::InsertLineBreakCommand::shouldUseBreakElement ReadAV@NULL
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: Nobody
URL: http://skypher.com/SkyLined/Repro/Web...
Keywords: GoogleBug, InRadar
Depends on:
Blocks:
 
Reported: 2009-10-06 04:06 PDT by Berend-Jan Wever
Modified: 2010-07-09 09:57 PDT (History)
11 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Berend-Jan Wever 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
Comment 1 Alexey Proskuryakov 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
Comment 2 Alexey Proskuryakov 2009-10-07 21:51:56 PDT
<rdar://problem/7285936>
Comment 3 Ryosuke Niwa 2009-12-17 17:28:44 PST
Cannot reproduce the bug with the reported HTML.
Comment 4 Lei Zhang 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.
Comment 5 Ryosuke Niwa 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.
Comment 6 Tony Chang 2010-06-07 02:22:49 PDT
Created attachment 58003 [details]
Patch
Comment 7 Tony Chang 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.
Comment 8 Ryosuke Niwa 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.
Comment 9 Tony Chang 2010-06-07 17:16:46 PDT
Created attachment 58097 [details]
Patch
Comment 10 Tony Chang 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).
Comment 11 Ojan Vafai 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.
Comment 12 Ojan Vafai 2010-06-08 15:29:40 PDT
https://bugs.webkit.org/show_bug.cgi?id=40338 For the display:none case.
Comment 13 Ojan Vafai 2010-06-08 16:02:23 PDT
visibility:hidden case is bug 40342. This bug can focus on the crash.
Comment 14 Tony Chang 2010-06-08 16:35:17 PDT
Sounds good to me. That sounds like we want the second patch, right?
Comment 15 Ojan Vafai 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?
Comment 16 Tony Chang 2010-06-09 00:10:14 PDT
Created attachment 58216 [details]
Patch
Comment 17 Tony Chang 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.
Comment 18 Ojan Vafai 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.
Comment 19 Tony Chang 2010-06-09 18:36:00 PDT
Created attachment 58320 [details]
Patch
Comment 20 Tony Chang 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.
Comment 21 Ojan Vafai 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.
Comment 22 Tony Chang 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.
Comment 23 Tony Chang 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.
Comment 24 Tony Chang 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.
Comment 25 Ojan Vafai 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.
Comment 26 Ojan Vafai 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?
Comment 27 Darin Adler 2010-07-01 12:10:51 PDT
I find these reitveld-created bugs.webkit.org comments difficult to read.
Comment 28 Ojan Vafai 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.
Comment 29 Tony Chang 2010-07-07 09:44:47 PDT
Created attachment 60745 [details]
Patch
Comment 30 WebKit Commit Bot 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>
Comment 31 WebKit Commit Bot 2010-07-08 21:35:41 PDT
All reviewed patches have been landed.  Closing bug.
Comment 32 Csaba Osztrogonác 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.
Comment 33 Tony Chang 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.