Summary: | [chromium] crash in Position::getInlineBoxAndOffset | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tony Chang <tony> | ||||||||||
Component: | WebCore Misc. | Assignee: | Tony Chang <tony> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, eric, mitz, ojan, rniwa | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Attachments: |
|
Description
Tony Chang
2010-07-13 14:41:09 PDT
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. |