Update owner document of Range objects if start/end boundary points in different document

Update owner document of Range objects if start/end boundary points in different document

This patch updates owner document of Range objects when orphan Node object moved
to another document in new member function |Range::updateOwnerDocumentIfNeeded|.

Note: when we move non-null parent Node object to another document, it is done
by removeChild and appendChild and Range objects to reset to start of document.

The root cause of issue 350362 is boundary points of Range objects isn't
adjusted when owner document of Range and boundary points are different.

Because |Range::nodeChildrenChanged|, which adjusts boundary points for
|Node.appendChild|, is called for Range objects in another document which
isn't owner of Range.

This patch also updates "move-detached-child-in-range.html" to have right
value.

BUG=350362
TEST=LayoutTests/fast/dom/Range/range-extract-contents-after-move-to-another-document-crash.html
TEST=LayoutTests/fast/dom/move-detached-child-in-range.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176410

[yosin_UTC9, 2014-06-18 01:23:17]

Could you review this patch?
Thanks in advance.

Note: "linux_blink_dbg" bot failure doesn't relate to this aptch.

[Yuta Kitamura, 2014-06-18 03:22:37]

https://codereview.chromium.org/332173003/diff/80001/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/332173003/diff/80001/Source/core/dom/Range.cp...
Source/core/dom/Range.cpp:1557: m_ownerDocument->attachRange(this);
I'm not sure migrating to another document is an expected
behavior. My reading of DOM spec tells the range should
stick to the same document.

How do other browsers behave?

[yosin_UTC9, 2014-06-18 05:50:42]

On 2014/06/18 03:22:37, Yuta Kitamura wrote:
> https://codereview.chromium.org/332173003/diff/80001/Source/core/dom/Range.cpp
> File Source/core/dom/Range.cpp (right):
> 
>
https://codereview.chromium.org/332173003/diff/80001/Source/core/dom/Range.cp...
> Source/core/dom/Range.cpp:1557: m_ownerDocument->attachRange(this);
> I'm not sure migrating to another document is an expected
> behavior. My reading of DOM spec tells the range should
> stick to the same document.
> 
> How do other browsers behave?

FireFox and IE yield same result with this patch. I checked with this
http://jsfiddle.net/HYYZ7/1/

- FireFox, IE and Chrome with this patch yield: start=SPAN offset=0 end=SPAN
offset=1
- Chrome w/o this patch: start=SPAN offset=2, end=SPAN offset=1

This is wrong.

If we don't move to Range object to actual owner document, boundary points of
Range object isn't updated.

Regarding section 9.3 DOM Ranges in http://dom.spec.whatwg.org/, it says:
"Range objects can now be moved between documents and used on nodes that are not
in a document."
and in DOM4 Range.detach() is no-op.


I read this as Range can be associated to another document which is different
from original document which Range object was associated.

Also, |Range::m_ownerDocument| is implementation detail. It isn't exposed as
API. Web developer's don't care where Range object belongs to.

I think we can get rid of |m_ownerDocument| from |Range| object, because we can
get owner document from boundary points, can't be null.

[Yuta Kitamura, 2014-06-18 06:09:48]

On 2014/06/18 05:50:42, yosi wrote:
> Regarding section 9.3 DOM Ranges in http://dom.spec.whatwg.org/, it says:
> "Range objects can now be moved between documents and used on nodes that are
not
> in a document."
> and in DOM4 Range.detach() is no-op.

I couldn't find the rationale of this statement. Please note that
this statement is in a section explaining historical changes,
which do not define the actual algorithm at all.

My reading of section 5.2.1 tells that a Range should stay
within its initial document, if I understand correctly.

Also, what will happen if only a part of a Range moves to
a different document? For example, consider you have a Range
spanning [S, E] in the following HTML:

   <div>aaaaSaa<span>bbbbEbb</span></div>

If I move the span element to another document, what will
happen?

[Yuta Kitamura, 2014-06-18 07:48:27]

I started to think this change is okay.

Please make your change description clear that this patch
tries to fix the issue that happens when a node without
a parent moves to another document.

https://codereview.chromium.org/332173003/diff/80001/LayoutTests/fast/dom/Ran...
File
LayoutTests/fast/dom/Range/range-extract-contents-after-move-to-another-document-crash.html
(right):

https://codereview.chromium.org/332173003/diff/80001/LayoutTests/fast/dom/Ran...
LayoutTests/fast/dom/Range/range-extract-contents-after-move-to-another-document-crash.html:16:
target.removeChild(child3);
Is this line necessary?

https://codereview.chromium.org/332173003/diff/80001/LayoutTests/fast/dom/Ran...
LayoutTests/fast/dom/Range/range-extract-contents-after-move-to-another-document-crash.html:19:
target.appendChild(child1);
Is this line necessary?

https://codereview.chromium.org/332173003/diff/80001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/332173003/diff/80001/Source/core/dom/Document...
Source/core/dom/Document.h:691: void didMoveNodeToNewDocument(const Node&);
1. There should be some comment saying that:
- This function is called for the document which the node
  previously belonged to.
- This function is called after the node is moved to a new
  document.

2. This function should not be declared at this line;
   functions at lines 688-690 and 692 are for mutations of
   Text nodes and not for node tree mutations. You probably
   should move this function to somewhere near lines 681-685
   and also should follow the naming convention of the
   functions there.

3. "New" -> "Another", maybe? The target document may not
   be necessarily new.

[yosin_UTC9, 2014-06-18 08:24:24]

On 2014/06/18 06:09:48, Yuta Kitamura wrote:
> On 2014/06/18 05:50:42, yosi wrote:
> > Regarding section 9.3 DOM Ranges in http://dom.spec.whatwg.org/, it says:
> > "Range objects can now be moved between documents and used on nodes that are
> not
> > in a document."
> > and in DOM4 Range.detach() is no-op.
> 
> I couldn't find the rationale of this statement. Please note that
> this statement is in a section explaining historical changes,
> which do not define the actual algorithm at all.
> 
> My reading of section 5.2.1 tells that a Range should stay
> within its initial document, if I understand correctly.
> 
> Also, what will happen if only a part of a Range moves to
> a different document? For example, consider you have a Range
> spanning [S, E] in the following HTML:
> 
>    <div>aaaaSaa<span>bbbbEbb</span></div>
> 
> If I move the span element to another document, what will
> happen?

These pages do this:
- http://jsfiddle.net/eX5Qm/ boundary points has non-null parent node
- http://jsfiddle.net/nT2pt/ make boundary points has null parent after creating
Range
- http://jsfiddle.net/pT6Z2/ make boundary points has null parent before
creating Range (the case for this patch)
 start =  "aaaSaa" offset=4, end = container="DIV" offset=1

[yosin_UTC9, 2014-06-18 08:55:41]

PTAL

https://codereview.chromium.org/332173003/diff/80001/LayoutTests/fast/dom/Ran...
File
LayoutTests/fast/dom/Range/range-extract-contents-after-move-to-another-document-crash.html
(right):

https://codereview.chromium.org/332173003/diff/80001/LayoutTests/fast/dom/Ran...
LayoutTests/fast/dom/Range/range-extract-contents-after-move-to-another-document-crash.html:16:
target.removeChild(child3);
On 2014/06/18 07:48:27, Yuta Kitamura wrote:
> Is this line necessary?

Yes, this |target.removeChild| invalidates offset cache in boundary points. This
cache is fulfilled at |range.extractContents|. When we remove this statement,
offset cache set by |target.selectNode| is used at |ragne.extractContents|.

https://codereview.chromium.org/332173003/diff/80001/LayoutTests/fast/dom/Ran...
LayoutTests/fast/dom/Range/range-extract-contents-after-move-to-another-document-crash.html:19:
target.appendChild(child1);
On 2014/06/18 07:48:27, Yuta Kitamura wrote:
> Is this line necessary?

Yes, this |target.appendChild()| invokes Range boundary points update for Range
objects in owner document of |target|, which is |anotherDocument|. So, this
statement assumes that owner document of Range and owner document of boundary
points is same.

https://codereview.chromium.org/332173003/diff/80001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/332173003/diff/80001/Source/core/dom/Document...
Source/core/dom/Document.h:691: void didMoveNodeToNewDocument(const Node&);
On 2014/06/18 07:48:27, Yuta Kitamura wrote:
> 1. There should be some comment saying that:
> - This function is called for the document which the node
>   previously belonged to.
> - This function is called after the node is moved to a new
>   document.
> 
> 2. This function should not be declared at this line;
>    functions at lines 688-690 and 692 are for mutations of
>    Text nodes and not for node tree mutations. You probably
>    should move this function to somewhere near lines 681-685
>    and also should follow the naming convention of the
>    functions there.
> 
> 3. "New" -> "Another", maybe? The target document may not
>    be necessarily new.

I renamed the function to |updateRangesAfterNodeMovedToAnotherDocument|.

https://codereview.chromium.org/332173003/diff/80001/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/332173003/diff/80001/Source/core/dom/Range.cp...
Source/core/dom/Range.cpp:1557: m_ownerDocument->attachRange(this);
On 2014/06/18 03:22:37, Yuta Kitamura wrote:
> I'm not sure migrating to another document is an expected
> behavior. My reading of DOM spec tells the range should
> stick to the same document.
> 
> How do other browsers behave?

I checked FireFox and IE. Behavior is as same as Chrome with this patch with
http://jsfiddle.net/HYYZ7/1/

[Yuta Kitamura, 2014-06-18 09:30:29]

LGTM with nits. There is one accidental edit of comments
in the new patch set.

https://codereview.chromium.org/332173003/diff/120001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/332173003/diff/120001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:3776: if (!m_ranges.isEmpty()) {
nit: Early exit is better here.

https://codereview.chromium.org/332173003/diff/120001/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/332173003/diff/120001/Source/core/dom/Range.c...
Source/core/dom/Range.cpp:90: // Simply setting the containers and offsets
directly would not do any of the king
"king"?

[yosin_UTC9, 2014-06-18 09:52:33]

Thanks for reviewing with deep thinking.
Committing...

https://codereview.chromium.org/332173003/diff/120001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/332173003/diff/120001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:3776: if (!m_ranges.isEmpty()) {
On 2014/06/18 09:30:29, Yuta Kitamura wrote:
> nit: Early exit is better here.

Done.

https://codereview.chromium.org/332173003/diff/120001/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/332173003/diff/120001/Source/core/dom/Range.c...
Source/core/dom/Range.cpp:90: // Simply setting the containers and offsets
directly would not do any of the king
On 2014/06/18 09:30:29, Yuta Kitamura wrote:
> "king"?

Done.

[yosin_UTC9, 2014-06-18 09:52:38]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2014-06-18 09:53:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yosin@chromium.org/332173003/130009

[commit-bot: I haz the power, 2014-06-18 12:20:31]

Change committed as 176410