Bug 25571

Summary:

DOM range methods setStartBefore/setEndAfter fails on detached elements

Product:

WebKit

Reporter:

Moxiecode Systems <spam>

Component:

DOM

Assignee:

Nobody <webkit-unassigned>

Status:

NEW

Severity:

Major

CC:

annevk, ap, b.rout, buildbot, commit-queue, esprehn+autocc, kangil.han, nathaneggen, rniwa

Priority:

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-

Moxiecode Systems

Reported 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.

Attachments
Testcase displaying the bug (626 bytes, text/html) 2009-05-05 10:49 PDT, Moxiecode Systems	no flags	Details
Patch (4.77 KB, patch) 2014-01-25 02:25 PST, Bhanu	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (485.23 KB, application/zip) 2014-01-25 03:42 PST, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (453.50 KB, application/zip) 2014-01-25 05:25 PST, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (493.95 KB, application/zip) 2014-01-26 00:17 PST, Build Bot	no flags	Details
Patch (6.71 KB, patch) 2014-01-27 01:35 PST, Bhanu	no flags	Details Formatted Diff Diff
Patch (10.17 KB, patch) 2014-02-03 01:10 PST, Bhanu	no flags	Details Formatted Diff Diff
Patch (15.58 KB, patch) 2014-02-14 06:02 PST, Bhanu	no flags	Details Formatted Diff Diff
Patch (16.13 KB, patch) 2014-02-25 06:07 PST, Bhanu	darin: review-	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Moxiecode Systems

Comment 1 2009-05-05 10:49:29 PDT

Created attachment 30025 [details] Testcase displaying the bug

Bhanu

Comment 2 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.

Bhanu

Comment 3 2014-01-25 02:25:54 PST

Created attachment 222202 [details] Patch

Bhanu

Comment 4 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?

Build Bot

Comment 5 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

Build Bot

Comment 6 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

Build Bot

Comment 7 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

Build Bot

Comment 8 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

Build Bot

Comment 9 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

Build Bot

Comment 10 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

Bhanu

Comment 11 2014-01-27 01:35:06 PST

Created attachment 222309 [details] Patch

Ryosuke Niwa

Comment 12 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.

Bhanu

Comment 13 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

Ryosuke Niwa

Comment 14 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.

Bhanu

Comment 15 2014-02-03 01:10:51 PST

Created attachment 222969 [details] Patch

Bhanu

Comment 16 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

Bhanu

Comment 17 2014-02-09 22:34:39 PST

Any comments?

Ryosuke Niwa

Comment 18 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.

Ryosuke Niwa

Comment 19 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.

Bhanu

Comment 20 2014-02-14 06:02:34 PST

Created attachment 224209 [details] Patch

Bhanu

Comment 21 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.

Ryosuke Niwa

Comment 22 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.

Bhanu

Comment 23 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.

Ryosuke Niwa

Comment 24 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.

Bhanu

Comment 25 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.

Bhanu

Comment 26 2014-02-25 06:07:45 PST

Created attachment 225142 [details] Patch

Bhanu

Comment 27 2014-03-04 05:58:38 PST

I have detailed out the change log, kindly let me know comments if any.

Ryosuke Niwa

Comment 28 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?

Bhanu

Comment 29 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?

Bhanu

Comment 30 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

Darin Adler

Comment 31 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?

Darin Adler

Comment 32 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.

Note You need to log in before you can comment on or make changes to this bug.