Bug 42202 – [chromium] crash in Position::getInlineBoxAndOffset

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Tony Chang]

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=
    0x7fffeabeeb40)
    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.

[Tony Chang]

Created attachment 61556 [details]
Patch

[Tony Chang]

Created attachment 61558 [details]
Patch

[Tony Chang]

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.

[Eric Seidel (no email)]

Attachment 61558 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3492289

[Tony Chang]

Created attachment 61565 [details]
Patch

[Tony Chang]

Adding some more people who might be able to review or comment on this patch.

[Ryosuke Niwa]

Looks like it's the right fix.

[Ojan Vafai]

Comment on attachment 61565 [details]
Patch

> -    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.

[Tony Chang]

Created attachment 62012 [details]
Patch

[Tony Chang]

Comment on attachment 62012 [details]
Patch

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).

[WebKit Commit Bot]

Comment on attachment 62012 [details]
Patch

Clearing flags on attachment: 62012

Committed r63731: <http://trac.webkit.org/changeset/63731>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.