| Summary: | REGRESSION (r167652): Broke fast/regions/cssom/region-range-for-box-crash.html in debug mode | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Manuel Rego Casasnovas <rego> | ||||||||||||
| Component: | CSS | Assignee: | Manuel Rego Casasnovas <rego> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | abucur, buildbot, commit-queue, enrica, esprehn+autocc, glenn, kondapallykalyan, mihnea, rniwa | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Manuel Rego Casasnovas
2014-04-22 04:55:51 PDT
After debugging this for a while the problem is in RenderView::splitSelectionBetweenSubtrees() when we try to create the |intialRange|:
RefPtr<Range> initialRange = Range::create(document(), start->node(), startPos, end->node(), endPos);
Basically, Range::create(), calls Range::setEnd() which ends up calling Range::checkNodeWOffset() which returns an error because of the element has not children:
case Node::ELEMENT_NODE:
case Node::ENTITY_REFERENCE_NODE:
case Node::XPATH_NAMESPACE_NODE: {
if (!offset)
return 0;
Node* childBefore = n->childNode(offset - 1);
if (!childBefore)
ec = INDEX_SIZE_ERR;
return childBefore;
}
This is because of in FrameSelection we have:
view->setSelection(startRenderer, startPos.deprecatedEditingOffset(), endRenderer, endPos.deprecatedEditingOffset());
And Position::deprecatedEditingOffset() is calling lastOffsetForEditing() which is returning 1 for the <input> as explained in the following comment:
// This method is used to create positions in the DOM. It returns the maximum valid offset
// in a node. It returns 1 for some elements even though they do not have children, which
// creates technically invalid DOM Positions. Be sure to call parentAnchoredEquivalent
// on a Position before using it to create a DOM Range, or an exception will be thrown.
int lastOffsetForEditing(const Node* node)
As we're only interested in the nodes for the |initialRange| we can avoid the positions and the issue seems to be fixed.
Created attachment 229888 [details]
Patch
Comment on attachment 229888 [details]
Patch
r=me
Comment on attachment 229888 [details] Patch Clearing flags on attachment: 229888 Committed r167675: <http://trac.webkit.org/changeset/167675> All reviewed patches have been landed. Closing bug. Reopen bug, as the fix was not right. It was just hiding the issue due to the wrong !renderer->canBeSelectionLeaf() a few lines below, which has been fixed in bug #132493. We need to find a proper solution for this issue. The problem here is that we're not able to create valid DOM ranges from the positions we receive in RenderView::setSelection(). As explained in comment #2, this is because of we're getting 1 as |endPos| for elements that don't have children. We have to be able to create a DOM range from the positions we receive in RenderView::setSelection() from FrameSelection::updateAppearance(). Created attachment 230982 [details]
Patch
This could be a possible solution but it's introducing regressions in other tests in debug mode. Maybe we should try to create a valid Position just before creating the DOM range, but I'm still not sure if that would fix or not the problem.
Comment on attachment 230982 [details] Patch Attachment 230982 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5698576785080320 New failing tests: svg/text/non-bmp-positioning-lists.svg fast/repaint/selection-gap-transformed-fixed-child.html editing/selection/contains-node-crash.html editing/execCommand/format-block-at-root.html fast/repaint/selection-gap-flipped-absolute-child.html fast/repaint/selection-gap-absolute-child.html fast/repaint/selection-gap-transformed-absolute-child.html Created attachment 230987 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 230982 [details] Patch Attachment 230982 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5400720803102720 New failing tests: svg/text/non-bmp-positioning-lists.svg fast/repaint/selection-gap-transformed-fixed-child.html editing/selection/contains-node-crash.html editing/execCommand/format-block-at-root.html fast/repaint/selection-gap-flipped-absolute-child.html fast/repaint/selection-gap-flipped-fixed-child.html fast/repaint/selection-gap-fixed-child.html fast/repaint/selection-gap-absolute-child.html fast/repaint/selection-gap-transformed-absolute-child.html Created attachment 230988 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Just as a wild idea, we could probably try to avoid create DOM ranges in RenderView. Instead of some kind of structure with the data we need for the selection:
{
RenderObject* start;
int startPos;
RenderObject* end;
int endPost;
}
This would probably fix the kind of problems we're having here.
Comment on attachment 230982 [details] Patch Attachment 230982 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6208198277070848 New failing tests: fast/repaint/selection-gap-transformed-fixed-child.html editing/selection/contains-node-crash.html editing/execCommand/format-block-at-root.html fast/repaint/selection-gap-flipped-absolute-child.html fast/repaint/selection-gap-flipped-fixed-child.html fast/repaint/selection-gap-fixed-child.html fast/repaint/selection-gap-absolute-child.html fast/repaint/selection-gap-transformed-absolute-child.html Created attachment 230995 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.5
This has been fixed in r169105 (bug #132720). The test still crashes, filed bug 133124. |