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: | P4 | Keywords: | WebExposed | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||
Bug Blocks: | 6627 | ||||||||||||||||||||||
Attachments: |
|
Description
Moxiecode Systems
2009-05-05 10:49:08 PDT
Created attachment 30025 [details]
Testcase displaying the bug
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. Created attachment 222202 [details]
Patch
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 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 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 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 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 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 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
Created attachment 222309 [details]
Patch
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. 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 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. Created attachment 222969 [details]
Patch
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 Any comments? 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. 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. Created attachment 224209 [details]
Patch
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 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. 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. (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. 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. Created attachment 225142 [details]
Patch
I have detailed out the change log, kindly let me know comments if any. 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? 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?
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 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 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. |