ASSERTION FAILURE: useDownstream ? (result > vp) : (result < vp) in WebCore::nextSentenceBoundaryInDirection()

• Component: HTML Editing
• Status: RESOLVED FIXED

[Daniel Bates]

The ASSERT(useDownstream ? (result > vp) : (result < vp)) in WebCore::nextSentenceBoundaryInDirection() is not correct because it does not account for detached positions: <https://trac.webkit.org/browser/trunk/Source/WebCore/dom/Node.cpp?rev=252554#L1667>.

[Radar WebKit Bug Importer]

<rdar://problem/57332559>

[Daniel Bates]

<https://trac.webkit.org/browser/trunk/Source/WebCore/dom/Node.cpp?rev=252554#L1674>

[Daniel Bates]

You can only compare positions whose anchors are connected (in a document) and are in the same tree scope

[Daniel Bates]

(In reply to Daniel Bates from comment #3)
> You can only compare positions whose anchors are connected (in a document)
> and are in the same tree scope

Otherwise, the result is non-deterministic by spec!
[[

static inline unsigned short compareDetachedElementsPosition(Node& firstNode, Node& secondNode)
{
    // If the 2 nodes are not in the same tree, return the result of adding DOCUMENT_POSITION_DISCONNECTED,
    // DOCUMENT_POSITION_IMPLEMENTATION_SPECIFIC, and either DOCUMENT_POSITION_PRECEDING or
    // DOCUMENT_POSITION_FOLLOWING, with the constraint that this is to be consistent. Whether to return
    // DOCUMENT_POSITION_PRECEDING or DOCUMENT_POSITION_FOLLOWING is implemented by comparing cryptographic
    // hashes of Node pointers.
    // See step 3 in https://dom.spec.whatwg.org/#dom-node-comparedocumentposition
    SHA1::Digest firstHash = hashPointer(&firstNode);
    SHA1::Digest secondHash = hashPointer(&secondNode);

    unsigned short direction = memcmp(firstHash.data(), secondHash.data(), SHA1::hashSize) > 0 ? Node::DOCUMENT_POSITION_PRECEDING : Node::DOCUMENT_POSITION_FOLLOWING;

    return Node::DOCUMENT_POSITION_DISCONNECTED | Node::DOCUMENT_POSITION_IMPLEMENTATION_SPECIFIC | direction;
}

]]

<https://trac.webkit.org/browser/trunk/Source/WebCore/dom/Node.cpp?rev=252554#L1597>

[Daniel Bates]

I was so tempted to use WebCore::comparePositions() to fix this, but it's implementation does not look the same to the detached element position check in Node::compareDocumentPosition() though I didn't dive deep enough to verify.

[Daniel Bates]

Created attachment 383898 [details]
Patch

[Daniel Bates]

Comment on attachment 383898 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=383898&action=review

> Source/WebCore/dom/Node.cpp:1613
> +bool areNodesConnectedInSameDocument(const Node* a, const Node* b)

This is a misnomer. Not so much same document as same tree scope.

[Daniel Bates]

Comment on attachment 383898 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=383898&action=review

> Source/WebCore/editing/VisibleUnits.cpp:1780
> +    ASSERT(areVisiblePositionsInSameDocument(result, vp) && useDownstream ? (result > vp) : (result < vp));

This is wrong: only want to assert if areVisiblePositionsInSameDocument()

[Daniel Bates]

Created attachment 383899 [details]
Patch

[Daniel Bates]

Thanks for the review!

[Daniel Bates]

Committed r252647: <https://trac.webkit.org/changeset/252647>

[Darin Adler]

Comment on attachment 383899 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=383899&action=review

> Source/WebCore/dom/Node.cpp:1617
> +    // Note that we avoid comparing Attr nodes here, since they return false from isConnected() all the time (which seems like a bug).

I don’t think this comment belongs here. I’d move it back where it came from. The code that "avoids" Attr nodes is not here, it’s in compareDocumentPosition, and the name of the function doesn't hide that it calls isConnected.

[Daniel Bates]

Comment on attachment 383899 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=383899&action=review

>> Source/WebCore/dom/Node.cpp:1617
>> +    // Note that we avoid comparing Attr nodes here, since they return false from isConnected() all the time (which seems like a bug).
> 
> I don’t think this comment belongs here. I’d move it back where it came from. The code that "avoids" Attr nodes is not here, it’s in compareDocumentPosition, and the name of the function doesn't hide that it calls isConnected.

Yep, I made a goof here. I don't get what you're saying about the name of the function. But the name of this function is also bad and should be changed: it checks if the nodes have the same connectedness.