Bug 25571

Summary: DOM range methods setStartBefore/setEndAfter fails on detached elements
Product: WebKit Reporter: Moxiecode Systems <spam>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Major CC: annevk, ap, b.rout, buildbot, commit-queue, esprehn+autocc, kangil.han, nathaneggen, rniwa
Priority: P4 Keywords: WebExposed
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 6627    
Attachments:
Description Flags
Testcase displaying the bug
none
Patch
none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
none
Patch
none
Patch
none
Patch
none
Patch darin: review-

Description Moxiecode Systems 2009-05-05 10:49:08 PDT
It's currently impossible to use the setStartBefore/setEndAfter on nodes detached form the document. This works fine on Gecko and it also works fine when you use the setStart/setEnd methods.

TinyMCE uses ranges on a detached element in it's serialization logic and it's currently failing on WebKit only.
Comment 1 Moxiecode Systems 2009-05-05 10:49:29 PDT
Created attachment 30025 [details]
Testcase displaying the bug
Comment 2 Bhanu 2014-01-17 05:14:29 PST
I am looking into this issue. I could see it, getting reproduced in version 162197 (local webkit repository version). I will be submitting some patch soon.
Comment 3 Bhanu 2014-01-25 02:25:54 PST
Created attachment 222202 [details]
Patch
Comment 4 Bhanu 2014-01-25 02:50:33 PST
The reason behind the range methods setStartBefore, setStartAfter, setEndBefore, setEndAfter failing on detached node is, the root container of the node is not an Attribute, Document, or DocumentFragment node.

I am not sure why this restriction is imposed on these methods while the same restriction is not applicable to setStart and setEnd methods.

I would like someone to review on the above range methods' specification before reviewing my patch, since my patch is violating the requirement specified in the documentation. Does the specification require a change here?
Comment 5 Build Bot 2014-01-25 03:42:42 PST
Comment on attachment 222202 [details]
Patch

Attachment 222202 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4756350466785280

New failing tests:
fast/dom/Range/range-exceptions.html
Comment 6 Build Bot 2014-01-25 03:42:44 PST
Created attachment 222205 [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
Comment 7 Build Bot 2014-01-25 05:25:02 PST
Comment on attachment 222202 [details]
Patch

Attachment 222202 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4934948091854848

New failing tests:
fast/dom/Range/range-exceptions.html
Comment 8 Build Bot 2014-01-25 05:25:04 PST
Created attachment 222207 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 9 Build Bot 2014-01-26 00:17:26 PST
Comment on attachment 222202 [details]
Patch

Attachment 222202 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5813448017444864

New failing tests:
fast/dom/Range/range-exceptions.html
Comment 10 Build Bot 2014-01-26 00:17:29 PST
Created attachment 222264 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 11 Bhanu 2014-01-27 01:35:06 PST
Created attachment 222309 [details]
Patch
Comment 12 Ryosuke Niwa 2014-01-27 15:59:48 PST
Comment on attachment 222309 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Range methods setStartBefore, setStartAfter, setEndBefore, setEndAfter throwing InvalidNodeTypeError exception
> +        since on detached node, root container of the node is not an Attr, Document, or DocumentFragment node.

Why are we making this behavioral change?  Is there any specification that mandates this behavior?
If so, please refer to the section of the specification in a specific revision (so that we'll have a reference even if the latest spec has changed).
r- due to the lack of justification.

> Source/WebCore/dom/Range.cpp:1214
> +        case Node::ELEMENT_NODE:

Please update or remove the comment at the beginning of this function.

> LayoutTests/fast/dom/Range/range-detachedNode.html:6
> +<head>
> +<title>Test for Range methods setStartBefore, setStartAfter, setEndBefore, setEndAfter on detached node</title>
> +<script src="../../../resources/js-test-pre.js"></script>
> +</head>

We don't need title or head. Just include js-test-pre.js in the body.

> LayoutTests/fast/dom/Range/range-detachedNode.html:15
> +var elm = document.getElementById('element');
> +function test() {
> +    // clone node, detaches the DOM
> +    elm = elm.cloneNode(true);

Please don't use abbreviations like elem. Spell out element.

> LayoutTests/fast/dom/Range/range-detachedNode.html:20
> +    shouldNotThrow('range.setStartBefore(elm.firstChild)');
> +
> +    shouldThrow('range.setStartBefore(elm)', '"Error: NotFoundError: DOM Exception 8"');
> +

We should also check that range.setStartBefore is setting the startContainer and startOffset correctly.
r- due to the inadequate testing.
Comment 13 Bhanu 2014-01-28 04:40:57 PST
Hi Ryosuke,

According to current recommendation it doesn't seem to be bug. But, if we look at other browsers like Mozilla it works.

current recommendation URL :
http://www.w3.org/TR/DOM-Level-2-Traversal-Range/ranges.html

But, I noticed there are some changes being made in W3C DOM4 on the range methods. 

http://www.w3.org/TR/dom/#concept-range-bp-set

But, its a working draft. Can it be considered as complete to proceed in the direction. If so, then I can follow the draft and create another patch with all your review comments addressed. Please suggest.

Thanks,
Bhanu
Comment 14 Ryosuke Niwa 2014-01-28 19:40:27 PST
We should use DOM4 instead of DOM3 for the reference, and you're right in that consistency with other browsers is usually more important in that the specification unless other browser vendors have already agreed to modify their engines' behaviors to match the current specification.
Comment 15 Bhanu 2014-02-03 01:10:51 PST
Created attachment 222969 [details]
Patch
Comment 16 Bhanu 2014-02-03 01:13:38 PST
The above patch fixes the range methods on detached node. I kept all the constraints as per DOM2 spec with additional null parent check as mentioned in the spec http://dom.spec.whatwg.org/ . The actual fix is made by allowing element node to be a root container though its not specified anywhere in the specs.

After looking at chromium code, I couldn't conclude whether to completely stick by DOM4 spec or both DOM2 as well as DOM4. So, I made the fix in similar fashion as chromium.

Chromium patch reference :
https://codereview.chromium.org/23404003

Chromium code reference :
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/dom/Range.cpp&q=range.cpp&sq=package:chromium
Comment 17 Bhanu 2014-02-09 22:34:39 PST
Any comments?
Comment 18 Ryosuke Niwa 2014-02-13 11:43:26 PST
Comment on attachment 222969 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        this requirement. Chromium code has the same change to fix the issue.

Please refer to the blink change & the original author since you're presumably making this change after having looked at their code change.

> Source/WebCore/ChangeLog:11
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the long description but after the bug url.

> Source/WebCore/dom/Range.cpp:1189
> -    // INVALID_NODE_TYPE_ERR: Raised if the root container of refNode is not an
> -    // Attr, Document, DocumentFragment or ShadowRoot node, or part of a SVG shadow DOM tree,
> +    // INVALID_NODE_TYPE_ERR: Raised if parent of refNode is null or the root container of refNode 
> +    // is not an Attr, Document, DocumentFragment or ShadowRoot node, or part of a SVG shadow DOM tree, Element node

I'd prefer deleting this comment altogether instead of updating it.
Comment 19 Ryosuke Niwa 2014-02-13 11:44:46 PST
The change log should also have a reference to a specific revision of the relevant specification where this behavior is defined.  It's important that we refer to a specific revision so that we know which behavior was implemented even if the specification changes in the future.
Comment 20 Bhanu 2014-02-14 06:02:34 PST
Created attachment 224209 [details]
Patch
Comment 21 Bhanu 2014-02-14 06:04:52 PST
After discussing with author of the new spec, I followed only http://dom.spec.whatwg.org/ which resolves the issue. There are methods which still go by the old spec http://www.w3.org/TR/DOM-Level-2-Traversal-Range/ranges.html. I think those methods should be re-structured based on the new spec separately.
Comment 22 Ryosuke Niwa 2014-02-14 12:00:16 PST
Comment on attachment 224209 [details]
Patch

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

r- because this patch removes some important node type checks and introduces bugs that the DOM4 spec has.

> Source/WebCore/ChangeLog:13
> +        DOM2 spec : http://www.w3.org/TR/DOM-Level-2-Traversal-Range/ranges.html
> +        latest spec : http://dom.spec.whatwg.org/

Please refer to a specific revision (git commit, publishing date, etc...) of the specification.
Otherwise the specification will change in the future and we'll have no idea which specification
this change set was referring to.

> Source/WebCore/dom/Range.cpp:1213
>      setStart(refNode->parentNode(), refNode->nodeIndex() + 1, ec);

setStart must throw InvalidNodeTypeError when refNode is doc type but it doesn't.
So this effectively breaks setStartAfter in that regard as well.

> Source/WebCore/dom/Range.cpp:-1361
> -    if (&ownerDocument() != &refNode->document())
> -        setDocument(refNode->document());

The fact we don't check this condition in the spec is a bug.
I've filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=24668 for setStart/setEnd
but we should probably file one for this one as well.
Comment 23 Bhanu 2014-02-17 04:02:34 PST
As per my earlier discussion with Anne, Attr, Entity and Notation nodes no longer exist. In addition to it, if a refNode is of document or ducument fragment type, then the parent will be null which suffices the need.

> r- because this patch removes some important node type checks and introduces bugs that the DOM4 spec has.
> 
> > Source/WebCore/ChangeLog:13
> > +        DOM2 spec : http://www.w3.org/TR/DOM-Level-2-Traversal-Range/ranges.html
> > +        latest spec : http://dom.spec.whatwg.org/

I am referring to the latest one. git commit id (33a11bb68544e2ae23d326448a438e7ac46bf640) as there is no change in the methods I am concerned, from the previous methods.
 
> Please refer to a specific revision (git commit, publishing date, etc...) of the specification.
> Otherwise the specification will change in the future and we'll have no idea which specification
> this change set was referring to.

Actually, there are few public methods invoked by the methods where this issue lies. But, I didn't touch any of these methods even though the new spec has some changes to these methods. I think it should be tracked separately as an activity to sync with new spec. Let me know your suggestion.

> > Source/WebCore/dom/Range.cpp:1213
> >      setStart(refNode->parentNode(), refNode->nodeIndex() + 1, ec);
> 
> setStart must throw InvalidNodeTypeError when refNode is doc type but it doesn't.
> So this effectively breaks setStartAfter in that regard as well.


The ownerdocument check is present in setStart as well as setEnd even though spec doesn't have it. Do we really require this check to be duplicated again in the methods invoking setStart/setEnd?
 
> > Source/WebCore/dom/Range.cpp:-1361
> > -    if (&ownerDocument() != &refNode->document())
> > -        setDocument(refNode->document());
> 
> The fact we don't check this condition in the spec is a bug.
> I've filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=24668 for setStart/setEnd
> but we should probably file one for this one as well.
Comment 24 Ryosuke Niwa 2014-02-17 21:11:28 PST
(In reply to comment #23)
> As per my earlier discussion with Anne, Attr, Entity and Notation nodes no longer exist. In addition to it, if a refNode is of document or ducument fragment type, then the parent will be null which suffices the need.
> 
> > r- because this patch removes some important node type checks and introduces bugs that the DOM4 spec has.
> > 
> > > Source/WebCore/ChangeLog:13
> > > +        DOM2 spec : http://www.w3.org/TR/DOM-Level-2-Traversal-Range/ranges.html
> > > +        latest spec : http://dom.spec.whatwg.org/
> 
> I am referring to the latest one. git commit id (33a11bb68544e2ae23d326448a438e7ac46bf640) as there is no change in the methods I am concerned, from the previous methods.

We should mention that in the change log as my point was that the spec could change in the future, not that it may have already changed.

> > Please refer to a specific revision (git commit, publishing date, etc...) of the specification.
> > Otherwise the specification will change in the future and we'll have no idea which specification
> > this change set was referring to.
> 
> Actually, there are few public methods invoked by the methods where this issue lies. But, I didn't touch any of these methods even though the new spec has some changes to these methods. I think it should be tracked separately as an activity to sync with new spec. Let me know your suggestion.

That doesn't matter. This change affects those publicly exposed API's behavior.

> > > Source/WebCore/dom/Range.cpp:1213
> > >      setStart(refNode->parentNode(), refNode->nodeIndex() + 1, ec);
> > 
> > setStart must throw InvalidNodeTypeError when refNode is doc type but it doesn't.
> > So this effectively breaks setStartAfter in that regard as well.
> 
> 
> The ownerdocument check is present in setStart as well as setEnd even though spec doesn't have it. Do we really require this check to be duplicated again in the methods invoking setStart/setEnd?

Ah I see. We should probably mention that in the change log as a per-function inline comment.

See http://www.webkit.org/coding/contributing.html#changelogs for an example of a well written change log.
Comment 25 Bhanu 2014-02-18 05:24:57 PST
After cross-verifying, I found this has been already handled in checkNodeWOffset() which is invoked from setStart/setEnd.

> > Source/WebCore/dom/Range.cpp:1213
> >      setStart(refNode->parentNode(), refNode->nodeIndex() + 1, ec);
> 
> setStart must throw InvalidNodeTypeError when refNode is doc type but it doesn't.
> So this effectively breaks setStartAfter in that regard as well.

I will have a detailed changelog with the spec revision I am following.
.
> We should mention that in the change log as my point was that the spec could change in the future, not that it may have already changed.
 
> Ah I see. We should probably mention that in the change log as a per-function inline comment.
> 
> See http://www.webkit.org/coding/contributing.html#changelogs for an example of a well written change log.

Let me know if I need to take care of anything in particular to this issue and the new spec.
Comment 26 Bhanu 2014-02-25 06:07:45 PST
Created attachment 225142 [details]
Patch
Comment 27 Bhanu 2014-03-04 05:58:38 PST
I have detailed out the change log, kindly let me know comments if any.
Comment 28 Ryosuke Niwa 2014-03-05 00:54:55 PST
Comment on attachment 225142 [details]
Patch

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

> Source/WebCore/dom/Range.cpp:1211
> -    ec = 0;
> -    checkNodeBA(refNode, ec);
> -    if (ec)
> +    if (!refNode->parentNode()) {
> +        ec = RangeException::INVALID_NODE_TYPE_ERR;
>          return;
> +    }

The spec says "set the start or end" should throw InvalidNodeTypeError if refNode->parentNode() is a doctype.
Where are we checking that now that the call to checkNodeBA has been removed?
Comment 29 Bhanu 2014-03-05 02:23:35 PST
doctype check for the node resides in Range::checkNodeWOffset() which is invoked inside setStart/setEnd.

> The spec says "set the start or end" should throw InvalidNodeTypeError if refNode->parentNode() is a doctype.
> Where are we checking that now that the call to checkNodeBA has been removed?
Comment 30 Bhanu 2014-03-12 06:54:21 PDT
Hi Ryosuke,

Let me know you have any more comments on the patch. 

Just kept my last reply in this comment in case you have missed to read, to your question on "InvalidNodetypeError to be thrown while setting start and end if the refNode is a doctype".

> doctype check for the node resides in Range::checkNodeWOffset() which is invoked inside setStart/setEnd.
> 
> > The spec says "set the start or end" should throw InvalidNodeTypeError if refNode->parentNode() is a doctype.
> > Where are we checking that now that the call to checkNodeBA has been removed?

Thanks,
Bhanu
Comment 31 Darin Adler 2014-07-12 16:53:13 PDT
Comment on attachment 225142 [details]
Patch

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

review- because of Ryosuke’s comment

> Source/WebCore/ChangeLog:27
> +        * dom/Range.h: added default value ASSERT_NO_EXCEPTION to exception code in setStartBefore() method 
> +        to avoid unwanted behavior due to uninitialized value.

I don’t understand what the uninitialized value issue is. Which call site has this unwanted behavior due to an uninitialized value?
Comment 32 Darin Adler 2014-07-12 16:57:41 PDT
Comment on attachment 225142 [details]
Patch

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

Hmm, wait it looks like there was a reply to Ryosuke’s comment, just not in the right place for him to see it.

The real reason for review- is a different mistake. See below:

> Source/WebCore/dom/Range.cpp:1273
> -    ec = 0;
> -    setStartBefore(refNode, ec);
> +    setStart(refNode->parentNode(), refNode->nodeIndex(), ec);

This change is incorrect. We need to set ec to zero before calling setStart just as the old code set ec to zero before setStartBefore; the caller is not obliged to set ec to anything before calling us. Otherwise we can’t safely check the value of ec after calling setStart.

> Source/WebCore/dom/Range.h:113
> +    void setStartBefore(Node*, ExceptionCode& = ASSERT_NO_EXCEPTION);

This change is incorrect. I don’t understand why we think it’s needed.