Bug 75851 - NULL ptr in WebCore::RenderSVGInlineText::localCaretRect
Summary: NULL ptr in WebCore::RenderSVGInlineText::localCaretRect
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows Vista
: P1 Normal
Assignee: Stephen Chenney
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-09 06:26 PST by Berend-Jan Wever
Modified: 2012-01-17 18:03 PST (History)
3 users (show)

See Also:


Attachments
Repro (350 bytes, image/svg+xml)
2012-01-09 06:26 PST, Berend-Jan Wever
no flags Details
Patch (1.58 KB, patch)
2012-01-13 13:56 PST, Stephen Chenney
no flags Details | Formatted Diff | Diff
Patch (1.58 KB, patch)
2012-01-13 16:11 PST, Stephen Chenney
no flags Details | Formatted Diff | Diff
Patch (2.95 KB, patch)
2012-01-17 13:42 PST, Stephen Chenney
no flags Details | Formatted Diff | Diff
Patch (2.93 KB, patch)
2012-01-17 13:59 PST, Stephen Chenney
no flags Details | Formatted Diff | Diff
Patch (2.90 KB, patch)
2012-01-17 15:28 PST, Stephen Chenney
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 2012-01-09 06:26:22 PST
Created attachment 121659 [details]
Repro

Chromium: http://code.google.com/p/chromium/issues/detail?id=109607

Repro:
<svg xmlns="http://www.w3.org/2000/svg"><style>
  * {
    border-top-style: inset
  }
</style><script>
  window.onload=function() {
    _Selection_0=getSelection();
    _Selection_0.setBaseAndExtent(document,5,document,5);
    _Selection_0.deleteFromDocument();
    _Selection_0.modify('extend','backward','line');
  }
</script>
<text></text><text> 1
Comment 1 Stephen Chenney 2012-01-13 13:56:34 PST
Created attachment 122495 [details]
Patch

Every other implementation of localCaretRect(...) checks for a null box. This makes the SVG inline text do the same, which fixes the reported chromium crash. I was unable to create a LayoutTest that crashed, so there is none added.
Comment 2 Ryosuke Niwa 2012-01-13 14:22:04 PST
Comment on attachment 122495 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122495&action=review

> Source/WebCore/ChangeLog:10
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the description.
Comment 3 Ryosuke Niwa 2012-01-13 15:47:35 PST
Kelly, please see my review comment. "Reviewed by" line in the change log is inserted at the wrong location and that's why I didn't cq+ it.
Comment 4 Stephen Chenney 2012-01-13 16:11:40 PST
Created attachment 122517 [details]
Patch

Fixed review line location. Apologies for not fixing pre-commit.
Comment 5 Ryosuke Niwa 2012-01-13 17:34:36 PST
Wait a minute, you didn't a test. Please add a new layout test.
Comment 6 Stephen Chenney 2012-01-17 08:52:52 PST
I could not figure out how to make LayoutTestController generate this stack trace, which is the crash stack trace. I'll try some more, but any help would be appreciated.


#0  0x00007ffff396646c in WebCore::RenderSVGInlineText::localCaretRect (this=0x7fffe2494658, box=0x0, caretOffset=0)
    at third_party/WebKit/Source/WebCore/rendering/svg/RenderSVGInlineText.cpp:141
#1  0x00007ffff314d9d3 in WebCore::RenderedPosition::absoluteRect (this=0x7fffffff9b90, extraWidthToEndOfLine=
    0x7fffffff9cec) at third_party/WebKit/Source/WebCore/editing/RenderedPosition.cpp:232
#2  0x00007ffff3130cfb in WebCore::RenderedPosition::absoluteRect (this=0x7fffffff9b90, extraWidthToEndOfLine=
    @0x7fffffff9cec) at third_party/WebKit/Source/WebCore/editing/RenderedPosition.h:71
#3  0x00007ffff312d097 in WebCore::Editor::firstRectForRange (this=0x7fffe23feef8, range=0x7fffe14b4600)
    at third_party/WebKit/Source/WebCore/editing/Editor.cpp:2640
#4  0x00007ffff294cd02 in WebKit::WebViewImpl::selectionBounds (this=0x7fffefe22600, start=..., end=...)
    at third_party/WebKit/Source/WebKit/chromium/src/WebViewImpl.cpp:1674
#5  0x00007ffff43f4823 in RenderWidget::GetSelectionBounds (this=0x7fffefe53000, start=0x7fffffff9fa0, end=
    0x7fffffff9f90) at content/renderer/render_widget.cc:1454
#6  0x00007ffff43dcfcd in RenderViewImpl::GetSelectionBounds (this=0x7fffefe53000, start=0x7fffffff9fa0, end=
    0x7fffffff9f90) at content/renderer/render_view_impl.cc:4551
#7  0x00007ffff43f48ea in RenderWidget::UpdateSelectionBounds (this=0x7fffefe53000)
    at content/renderer/render_widget.cc:1465
#8  0x00007ffff43f204e in RenderWidget::DoDeferredUpdate (this=0x7fffefe53000) at content/renderer/render_widget.cc:908
#9  0x00007ffff43f0b43 in RenderWidget::DoDeferredUpdateAndSendInputAck (this=0x7fffefe53000)
    at content/renderer/render_widget.cc:730
#10 0x00007ffff43eee2c in RenderWidget::OnUpdateRectAck (this=0x7fffefe53000) at content/renderer/render_widget.cc:383
Comment 7 Stephen Chenney 2012-01-17 13:42:23 PST
Created attachment 122807 [details]
Patch

Adding a manual test case, as the issue was not reproducable with DRT.
Comment 8 Stephen Chenney 2012-01-17 13:50:43 PST
Damn. Crash isn't happening with this test - swear it was.
Comment 9 Stephen Chenney 2012-01-17 13:59:20 PST
Created attachment 122810 [details]
Patch

This test file crashes.
Comment 10 Ryosuke Niwa 2012-01-17 15:03:58 PST
Comment on attachment 122810 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122810&action=review

It's still sad that we can't add an automated test.

> ManualTests/svg-modify-deleted-selection.svg:12
> +    _Selection_0=getSelection();
> +    _Selection_0.setBaseAndExtent(document,5,document,5);
> +    _Selection_0.deleteFromDocument();
> +    _Selection_0.modify('extend','backward','line');

Why do we need _Selection_0? You can just call getSelection() on every line.
Comment 11 Stephen Chenney 2012-01-17 15:28:10 PST
Created attachment 122824 [details]
Patch

Got rid of the _Selection_0 variable.
Comment 12 WebKit Review Bot 2012-01-17 18:03:43 PST
Comment on attachment 122824 [details]
Patch

Clearing flags on attachment: 122824

Committed r105224: <http://trac.webkit.org/changeset/105224>
Comment 13 WebKit Review Bot 2012-01-17 18:03:48 PST
All reviewed patches have been landed.  Closing bug.