Bug 42202 - [chromium] crash in Position::getInlineBoxAndOffset
Summary: [chromium] crash in Position::getInlineBoxAndOffset
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Tony Chang
Depends on:
Reported: 2010-07-13 14:41 PDT by Tony Chang
Modified: 2010-07-20 01:17 PDT (History)
5 users (show)

See Also:

Patch (40.46 KB, patch)
2010-07-14 13:22 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (40.32 KB, patch)
2010-07-14 13:26 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (40.57 KB, patch)
2010-07-14 14:23 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (42.16 KB, patch)
2010-07-19 16:51 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 Tony Chang 2010-07-13 14:41:09 PDT
We're hitting a crash in chromium that has the following stack:

#0  0x0000000001a3fa66 in WebCore::RenderObject::isText (this=0x0)
    at third_party/WebKit/WebCore/rendering/RenderObject.h:375
#1  0x0000000001af5927 in WebCore::Position::getInlineBoxAndOffset (this=
    0x450cdcf0, affinity=WebCore::UPSTREAM, primaryDirection=WebCore::LTR, 
    inlineBox=@0x450cdd48, caretOffset=@0x450cdd70)
    at third_party/WebKit/WebCore/dom/Position.cpp:1014
#2  0x0000000001af6359 in WebCore::Position::getInlineBoxAndOffset (this=
    0x450cdcf0, affinity=WebCore::UPSTREAM, inlineBox=@0x450cdd48, 
    caretOffset=@0x450cdd70) at third_party/WebKit/WebCore/dom/Position.cpp:949
#3  0x0000000001cf88bb in WebCore::Frame::firstRectForRange (this=
    0x7fffeb953000, range=0x7fffc3a19940)
    at third_party/WebKit/WebCore/page/Frame.cpp:312
#4  0x00000000019b886f in WebKit::WebViewImpl::caretOrSelectionBounds (this=
    at third_party/WebKit/WebKit/chromium/src/WebViewImpl.cpp:1241
#5  0x0000000000cd0065 in RenderWidget::UpdateInputMethod (this=0x7fffeacacc00)
    at chrome/renderer/render_widget.cc:876

It's crashing, because the node()->renderer() is null when we call Position::getInlineBoxAndOffset.  I'm adding mitz because I'm not sure at what level I'm supposed to be checking for null:
1) in Position::getInlineBoxAndOffset
2) in Frame::firstRectForRange
3) in Chromium's WebViewImpl

It seems like there are a few other places where random range's can get passed to Frame::firstRectForRange, so I think that's the right spot for a check.  For example, scrollDOMRangeToVisible in WebView.mm and EventHandler::sendContextMenuEventForKey use firstRectForRange without doing any checks on the range bounds.
Comment 1 Tony Chang 2010-07-14 13:22:11 PDT
Created attachment 61556 [details]
Comment 2 Tony Chang 2010-07-14 13:26:05 PDT
Created attachment 61558 [details]
Comment 3 Tony Chang 2010-07-14 13:28:57 PDT
I'm not 100% sure about the change in LayoutTests/platform/mac/editing/input/*.  I'm not sure why the cursor position would have moved up, but it might be a misunderstanding of how firstRectForCharactersFrom works.  Maybe these are the positions of white space characters?

mitz, can you review?  Otherwise, maybe eric can review since this is touching Position/VisiblePosition code.
Comment 4 Eric Seidel (no email) 2010-07-14 13:43:25 PDT
Attachment 61558 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3492289
Comment 5 Tony Chang 2010-07-14 14:23:28 PDT
Created attachment 61565 [details]
Comment 6 Tony Chang 2010-07-19 09:38:29 PDT
Adding some more people who might be able to review or comment on this patch.
Comment 7 Ryosuke Niwa 2010-07-19 14:14:34 PDT
Looks like it's the right fix.
Comment 8 Ojan Vafai 2010-07-19 15:55:41 PDT
Comment on attachment 61565 [details]

> -    range->startPosition().getInlineBoxAndOffset(DOWNSTREAM, startInlineBox, startCaretOffset);
> +    Position startPosition = VisiblePosition(range->startPosition()).deepEquivalent();
> +    startPosition.getInlineBoxAndOffset(DOWNSTREAM, startInlineBox, startCaretOffset);

> -    range->endPosition().getInlineBoxAndOffset(UPSTREAM, endInlineBox, endCaretOffset);
> +    Position endPosition = VisiblePosition(range->endPosition()).deepEquivalent();
> +    endPosition.getInlineBoxAndOffset(UPSTREAM, endInlineBox, endCaretOffset);

Said this to Tony in person, but, I wonder if we need to null-check startPosition and endPosition here. The case I'm thinking of is if the node containing the selection is made visibility:hidden. The contextMenu key support would hit this codepath. In either case, I agree this patch is correct. Might just need a null-check.
Comment 9 Tony Chang 2010-07-19 16:51:34 PDT
Created attachment 62012 [details]
Comment 10 Tony Chang 2010-07-19 16:57:34 PDT
Comment on attachment 62012 [details]

Ojan was right, we need to null check the visibleposition or we crash when it is null.  I added a manual test for this case (it uses the contextmenu key on windows).
Comment 11 WebKit Commit Bot 2010-07-20 01:17:46 PDT
Comment on attachment 62012 [details]

Clearing flags on attachment: 62012

Committed r63731: <http://trac.webkit.org/changeset/63731>
Comment 12 WebKit Commit Bot 2010-07-20 01:17:51 PDT
All reviewed patches have been landed.  Closing bug.