Summary: | Crash when navigating with arrow key into empty anchor block with padding | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Jalkut <jalkut> | ||||||||||||
Component: | HTML Editing | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED WORKSFORME | ||||||||||||||
Severity: | Major | CC: | enrica, eric, leviw, rniwa, webkit-bug-importer, webkit.review.bot, xji | ||||||||||||
Priority: | P1 | Keywords: | InRadar | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Mac (Intel) | ||||||||||||||
OS: | OS X 10.7 | ||||||||||||||
Attachments: |
|
Description
Daniel Jalkut
2011-12-14 21:46:52 PST
The crash seems to stem from the assumption that a given InlineBox will have non-NULL leaf children. InlineBox* RootInlineBox::closestLeafChildForLogicalLeftPosition(int leftPosition, bool onlyEditableLeaves) { InlineBox* firstLeaf = firstLeafChild(); InlineBox* lastLeaf = lastLeafChild(); if (firstLeaf == lastLeaf && (!onlyEditableLeaves || isEditableLeaf(firstLeaf))) return firstLeaf; Currently the behavior when this method is reached for a box with no children, is to crash hard later in the method, trying to dereference firstLeaf. The implicit contract for closestLeafChildForLogicalLeftPosition seems to be that it will always return a non-NULL result (its callers blindly dereference the result). So what is the appropriate return value when a RootInlineBox with no children is asked for the closestLeafChildForLogicalLeftPosition? Is it just the box itself? What if "onlyEditableLeaves" is true but the box itself is not editable? I think in deciding how to address this bug, it should be determined whether navigating into this empty anchor block should or shouldn't succeed. I think ideally it would move the insertion position to the point in the block where a character would exist if it were part of the anchor innerHTML. Currently if you position the cursor to the LEFT of the problematic block, and attempt to right-arrow into the block, it also fails, but doesn't crash. In this scenario, modifyMovingRight rejects the block as a navigable target, and returns the current position. This behavior would be an acceptable compromise for the attempt to move up or down into the block as well (selectNextLine and selectPreviousLine). Created attachment 120807 [details]
Fix and manual test to prevent crashing when navigating into an empty anchor
I decided I could offer a patch that at least alleviates the crashing nature of the bug. With the attached patch, the closestLeafChild... methods in RootInlineBox are allowed to return 0, and the callers (only two I could find) are now expected to handle this situation gracefully.
I feel that ideally the behavior when navigating into this empty anchor would be to place the cursor where typing would change the innerHTML of the anchor from void to something. But I don't feel qualified to develop a patch achieving this yet.
I hope you will consider the patch as-is since it will at least change the behavior from one where WebKit crashes every time, to one where the behavior is merely a little frustrating (the cursor navigates as far in the adjacent box as possible, without entering the empty anchor).
Comment on attachment 120807 [details]
Fix and manual test to prevent crashing when navigating into an empty anchor
Oops - my patch doesn't contain the manual tests I added. Will fix and reattach.
Attachment 120807 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1
Source/WebCore/editing/visible_units.cpp:612: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
Source/WebCore/editing/visible_units.cpp:718: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
Total errors found: 2 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 120809 [details]
Patch take two
I added the ManualTests to the patch and also fixed some style issues.
Attachment 120809 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'ManualTests/crash-on-arrow-i..." exit_code: 1
Source/WebCore/editing/visible_units.cpp:619: An else should appear on the same line as the preceding } [whitespace/newline] [4]
Source/WebCore/editing/visible_units.cpp:612: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
Source/WebCore/editing/visible_units.cpp:725: An else should appear on the same line as the preceding } [whitespace/newline] [4]
Source/WebCore/editing/visible_units.cpp:718: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
Total errors found: 4 in 5 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 120809 [details]
Patch take two
Ugh! Not running check-webkit-style myself diligently enough, obviously. Sorry about this.
Created attachment 120810 [details]
Patch take three
To further comply with style guidelines I just removed the else statements that set root = 0. The variable is not referenced again in the function but it was my instinct to defensively nil it.
Comment on attachment 120810 [details] Patch take three View in context: https://bugs.webkit.org/attachment.cgi?id=120810&action=review > ManualTests/crash-on-arrow-into-empty-anchor.html:9 > +<li>Press the up or down arrow key to attempt to enter the red rectangle.</li> You should be able to automate this using getSelection().modify. r- for this. > Source/WebCore/ChangeLog:8 > + Return 0 from closestLeafChildForLogicalLeftPosition instead of crashing when a non-leaf box with no children is being asked for its leaf children. Adjust logic for callers in previousLinePosition and nextLinePosition to detect 0 response and treat the box as non-navigable. This line is way too long. Please wrap line as needed. See other entries for example. > Source/WebCore/editing/visible_units.cpp:716 > + InlineBox* leafChild = root->closestLeafChildForPoint(pointInLine, isEditablePosition(p)); > + if (leafChild) { It's odd that we can get null here. What is box in line 702 then? Is it a root inline box? We should probably check that root box as at least line leaf in line 700 immediately after pos.getInlineBoxAndOffset(DOWNSTREAM, box, ignoredCaretOffset); and fall back to return VisiblePosition(pos, DOWNSTREAM); because that's the code path we normally use for an empty block. Thanks, I am naive about the layout tests and assumed you couldn't test crashes. I've developed a test case that uses the technique of updating the body innerHTML to a "not crashed" content. (In reply to comment #11) > > Source/WebCore/editing/visible_units.cpp:716 > > + InlineBox* leafChild = root->closestLeafChildForPoint(pointInLine, isEditablePosition(p)); > > + if (leafChild) { > > It's odd that we can get null here. What is box in line 702 then? Is it a root inline box? We should probably check that root box as at least line leaf in line 700 immediately after pos.getInlineBoxAndOffset(DOWNSTREAM, box, ignoredCaretOffset); and fall back to return VisiblePosition(pos, DOWNSTREAM); because that's the code path we normally use for an empty block. In the crashing scenario, line 702 is not reached. The root is established at line 677: if (box) { root = box->root()->nextRootBox(); // We want to skip zero height boxes. // This could happen in case it is a TrailingFloatsRootInlineBox. if (!root || !root->logicalHeight()) root = 0; } box is an InlineBox and root is established as a RootInlineBox (gdb) p box $1 = ('WebCore::InlineBox' *) 0x106d84118 (gdb) p box->root() $2 = (const 'WebCore::RootInlineBox' *) 0x106dcfef8 (gdb) p box->root()->nextRootBox() $3 = ('WebCore::RootInlineBox' *) 0x106de5648 It is this RootInlineBox that has one child, but no "leaf" children. (gdb) p root->m_firstChild $3 = ('WebCore::InlineBox' *) 0x1082d42b8 (gdb) p root->m_firstChild->isLeaf() $4 = false (gdb) p root->m_lastChild $5 = ('WebCore::InlineBox' *) 0x1082d42b8 I'm in over my head here with the box stuff, so I'm not sure how much more I can do on my own. But if you have a better idea for how to protect against this I'm happy to execute it in the patch and test it. Since the code at line 700 is never reached in this case, I'm guessing you think we should be doing some test around line 676 to see if the first getInlineBoxAndOffset() box returned is suitable for further examination? Created attachment 120815 [details]
Patch take four
I modified the patch to include an automated layout test. Let me know if you think we should be pursuing another means of safeguarding against the crashing behavior.
Created attachment 120817 [details]
Patch take five: amend ChangeLog to reference the automated layout test
(In reply to comment #12) > (gdb) p box > $1 = ('WebCore::InlineBox' *) 0x106d84118 > (gdb) p box->root() > $2 = (const 'WebCore::RootInlineBox' *) 0x106dcfef8 > (gdb) p box->root()->nextRootBox() > $3 = ('WebCore::RootInlineBox' *) 0x106de5648 > > It is this RootInlineBox that has one child, but no "leaf" children. How can it have a child and not a leaf? Can you call box->showLineTreeForThis() and print out the line tree ? (In reply to comment #15) > How can it have a child and not a leaf? Can you call box->showLineTreeForThis() and print out the line tree ? It seems that the RootInlineBox being targeted (the one that contains this pesky empty anchor node) contains only an "InlineFlowBox" which seems to have its "isLeaf()" hardcoded to false. I tried changing the implementation of isLeaf to return true if it has no children, but that seemed to cause logic problems elsewhere. (gdb) call (void) box->showLineTreeForThis() RenderBlock 0x10919b2d8 P 0x10d954c30 RootInlineBox 0x10911d638 RenderBlock 0x10919b2d8 InlineTextBox 0x1091c4ed8 RenderText 0x109181a88 (0,74) "Click to place the editing cursor anywhere on this line ... then click the" RootInlineBox 0x1091c45d8 RenderBlock 0x10919b2d8 * InlineTextBox 0x1091d7998 RenderText 0x109181a88 (75,86) "down arrow." InlineTextBox 0x10dc04398 RenderBR 0x1091a92f8 (0,1) "\n" RootInlineBox 0x10911a9e8 RenderBlock 0x10919b2d8 InlineFlowBox 0x1091de488 RenderInline 0x1091849a8 (gdb) p root $7 = ('WebCore::RootInlineBox' *) 0x10911a9e8 (gdb) p root->isLeaf() $8 = false (gdb) p root->m_firstChild->isLeaf() $9 = false (In reply to comment #16) > (In reply to comment #15) > > How can it have a child and not a leaf? Can you call box->showLineTreeForThis() and print out the line tree ? > > It seems that the RootInlineBox being targeted (the one that contains this pesky empty anchor node) contains only an "InlineFlowBox" which seems to have its "isLeaf()" hardcoded to false. I tried changing the implementation of isLeaf to return true if it has no children, but that seemed to cause logic problems elsewhere. Okay. Thanks for the clarification. I think we need to deal it around line 677 and add a similar bail out as line 706: return VisiblePosition(pos, DOWNSTREAM);. Also, I'd like to see a test case where we have some contents after the anchor. I bet your current patch won't work as expected in such case. (In reply to comment #17) > Okay. Thanks for the clarification. I think we need to deal it around line 677 and add a similar bail out as line 706: return VisiblePosition(pos, DOWNSTREAM);. > > Also, I'd like to see a test case where we have some contents after the anchor. I bet your current patch won't work as expected in such case. Can you give me some advice for how I could test the box and its children for this situation? Should it literally look for a "no leaf children" situation or is there a higher-level test of the found root that makes more sense? I'm still very shaky on the box classes and only know a little from poking around at this bug. I will amend the test case to have content on the bottom, too. I actually tested this manually to make sure that up-arrowing (which also crashed) is fixed as well. In the scenario where there is content on the other side of the anchor (another line), my patch does still prevent the crash, and the selection ends up at the end of the anchor line, where further arrowing will continue moving the cursor to the good line of content that follows. Sample source for this which I will be incorporating into the test case: <div contentEditable="true"> Click to place the editing cursor anywhere on this line ... then click the down arrow.<br /> <a style="background-color:red; padding-left:200px;"></a><br /> Or click here, then click the up arrow. </div> This crash no longer reproduces for me. I also am not able to reproduce the crash using Safari Beta Preview or WebKit nightly. Thanks! |