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.
Created attachment 61556 [details] Patch
Created attachment 61558 [details] Patch
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.
Attachment 61558 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/3492289
Created attachment 61565 [details] Patch
Adding some more people who might be able to review or comment on this patch.
Looks like it's the right fix.
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.
Created attachment 62012 [details] Patch
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).
Comment on attachment 62012 [details] Patch Clearing flags on attachment: 62012 Committed r63731: <http://trac.webkit.org/changeset/63731>
All reviewed patches have been landed. Closing bug.