compareDocumentPosition() should report PRECEEDING or FOLLOWING information even if nodes are disco…

compareDocumentPosition() should report PRECEEDING or FOLLOWING information even if nodes are disconnected

As per the latest specification, compareDocumentPosition() should report PRECEEDING or FOLLOWING
information even if nodes are disconnected:
- http://dom.spec.whatwg.org/#dom-node-comparedocumentposition

This behavior is consistent with both IE10 and Firefox 22.

Blink was not reporting PRECEEDING or FOLLOWING in all cases. This patches makes Blink behave
as expected.

BUG=263340
R=arv@chromium.org, haraken@chromium.org, tkent@chromium.org

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

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

[arv (Not doing code reviews), 2013-07-23 17:10:45]

LGTM

[haraken, 2013-07-24 00:51:35]

LGTM.

[tkent, 2013-07-24 00:53:43]

The behavior change sounds reasonable.  But the test
dom/xhtml/level3/core/nodecomparedocumentposition38.xhtml  is failing.

[do-not-use, 2013-07-24 06:57:54]

On 2013/07/24 00:53:43, tkent wrote:
> The behavior change sounds reasonable.  But the test
> dom/xhtml/level3/core/nodecomparedocumentposition38.xhtml  is failing.

The test was already failing (the nodes are detected as disconnected although
they are not). Since the nodes are detected as disconnected, the output has
changed (still failing but with a different value). I'm simply rebaselining the
test for now as this is a pre-existing bug (either in Blink or in the test).

I'll investigate the cause for that failure separately.

[tkent, 2013-07-24 06:59:13]

ok, lgtm

[commit-bot: I haz the power, 2013-07-24 07:09:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/20042003/10001

[commit-bot: I haz the power, 2013-07-24 10:14:14]

Retried try job too often on win_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[commit-bot: I haz the power, 2013-07-24 11:50:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/20042003/10001

[commit-bot: I haz the power, 2013-07-24 13:36:00]

Retried try job too often on win_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[commit-bot: I haz the power, 2013-07-24 14:08:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/20042003/10001

[do-not-use, 2013-07-24 15:17:02]

Committed patchset #3 manually as r154835 (presubmit successful).

[adamk, 2013-07-24 22:22:14]

This is causing flakiness on various platforms due to pointer comparisons, see
the flakiness dashboard:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=dom...

https://codereview.chromium.org/20042003/diff/10001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/20042003/diff/10001/Source/core/dom/Node.cpp#...
Source/core/dom/Node.cpp:1708: unsigned short direction = (this > otherNode) ?
DOCUMENT_POSITION_PRECEDING : DOCUMENT_POSITION_FOLLOWING;
These pointer comparisons look totally bogus to me, and are causing different
results on different platforms. Given that I can't tell what they were trying to
do, I'm going to roll out this patch.

[do-not-use, 2013-07-25 06:38:32]

Reopening as it was rolled out. I'll investigate the flakiness.

[do-not-use, 2013-07-25 06:46:46]

Ok, found the reason. It affects the following test (that I rebased)
LayoutTests/dom/xhtml/level3/core/nodecomparedocumentposition38.xhtml

The elements are detected as disconnected so the result is:
DOCUMENT_POSITION_DISCONNECTED | DOCUMENT_POSITION_IMPLEMENTATION _SPECIFIC |
direction

The direction can change between test runs so the value can change and the test
has 50% changes failing. Other tests are properly written so that they accept
both values for direction. However, this particular w3c test is not. The reason
for that is that the test is failing (the elements should not be detected as
disconnected).

I propose to add this test to TestExpectations since it is failing and we have a
bug in Blink (unrelated to my change).

[tkent, 2013-07-25 06:52:04]

Is it ok to have pointer comparisons which adamk pointed out?

[do-not-use, 2013-07-25 07:03:56]

On 2013/07/25 06:52:04, tkent wrote:
> Is it ok to have pointer comparisons which adamk pointed out?

Yes, this is done on purpose and advised by the specification:
"""
* If other and reference 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, together and terminate these steps.

Note: Whether to return DOCUMENT_POSITION_PRECEDING or
DOCUMENT_POSITION_FOLLOWING is typically implemented via pointer comparison.
"""

Note that I filed a bug to explain why the test is failing:
http://code.google.com/p/chromium/issues/detail?id=264138

[tkent, 2013-07-25 07:09:24]

Wow, it's scary specification!

[do-not-use, 2013-07-25 07:12:37]

On 2013/07/25 07:09:24, tkent wrote:
> Wow, it's scary specification!

Note that I looked at the mozilla implementation and it is done in a very
similar way. As I said, the result is as expected and specified. The only issue
is that this test is failing and that the expected result contains a value that
can change between test runs.

I do believe the best way to handle this is to add the test to TestExpectations
and update its -expected.txt to contains the real expected result in case of
success.

[do-not-use, 2013-07-25 07:19:31]

On 2013/07/25 07:12:37, Christophe Dumez wrote:
> On 2013/07/25 07:09:24, tkent wrote:
> > Wow, it's scary specification!
> 
> Note that I looked at the mozilla implementation and it is done in a very
> similar way. As I said, the result is as expected and specified. The only
issue
> is that this test is failing and that the expected result contains a value
that
> can change between test runs.
> 
> I do believe the best way to handle this is to add the test to
TestExpectations
> and update its -expected.txt to contains the real expected result in case of
> success.

I updated the patch set accordingly, please take another look.

[tkent, 2013-07-25 07:42:28]

lgtm. please go ahead.

[commit-bot: I haz the power, 2013-07-25 07:43:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/20042003/43001

[commit-bot: I haz the power, 2013-07-25 08:57:16]

Change committed as 154908

[arv (Not doing code reviews), 2013-07-25 18:59:28]

Wow, that pointer comparison is actually crazy. Can we file a bug to
the relevant spec?

On Thu, Jul 25, 2013 at 1:57 AM,  <commit-bot@chromium.org> wrote:
> Change committed as 154908
>
> https://chromiumcodereview.appspot.com/20042003/



-- 
erik

[adamk, 2013-07-25 19:05:04]

The pointer-comparison recommendation in the spec is non-normative; the
spec only requires that the comparison be stable (which this pointer
comparison wouldn't be if we moved to a managed heap with a compacting
garbage collector).


On Thu, Jul 25, 2013 at 11:59 AM, Erik Arvidsson <arv@chromium.org> wrote:

> Wow, that pointer comparison is actually crazy. Can we file a bug to
> the relevant spec?
>
> On Thu, Jul 25, 2013 at 1:57 AM,  <commit-bot@chromium.org> wrote:
> > Change committed as 154908
> >
> > https://chromiumcodereview.appspot.com/20042003/
>
>
>
> --
> erik
>

[erikcorry, 2013-08-27 11:14:48]

FWIW Oilpan doesn't move objects, and it probably never will.

On the other hand it's a good security policy not to reveal the addresses
of objects, and this gives the attacker a hint so it feels unhealthy.


On Thu, Jul 25, 2013 at 9:05 PM, Adam Klein <adamk@chromium.org> wrote:

> The pointer-comparison recommendation in the spec is non-normative; the
> spec only requires that the comparison be stable (which this pointer
> comparison wouldn't be if we moved to a managed heap with a compacting
> garbage collector).
>
>
> On Thu, Jul 25, 2013 at 11:59 AM, Erik Arvidsson <arv@chromium.org> wrote:
>
>> Wow, that pointer comparison is actually crazy. Can we file a bug to
>> the relevant spec?
>>
>> On Thu, Jul 25, 2013 at 1:57 AM,  <commit-bot@chromium.org> wrote:
>> > Change committed as 154908
>> >
>> > https://chromiumcodereview.appspot.com/20042003/
>>
>>
>>
>> --
>> erik
>>
>
>


-- 
Erik Corry
Google Denmark ApS
Sankt Petri Passage 5, 2. sal
1165 København K - Denmark - CVR nr. 28 86 69 84

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.