Bug 96830 – Revert r127457 and following fixes due to several hit-testing regressions

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Julien Chaffraix]

After experiencing several serious regressions on Chromium [1], [2] (and likely on other platforms [3]) and talking with Antonio, it appears that the approach in bug 95204 is not adequate.

This change will revert r127457 and the follow-up fixes.

[1] https://bugs.webkit.org/show_bug.cgi?id=96593
[2] http://code.google.com/p/chromium/issues/detail?id=148670 and http://code.google.com/p/chromium/issues/detail?id=149696 (probably related)
[3] https://bugs.webkit.org/show_bug.cgi?id=96593#c14

[Julien Chaffraix]

Created attachment 164236 [details]
Proposed revert.

[Julien Chaffraix]

Committed r128677: <http://trac.webkit.org/changeset/128677>

[Allan Sandfeld Jensen]

Technically there are also two lines of code the patch moving Document::updateActiveHoverState that should be reverted. It is the two lines that find the inner node of the current document. They don't do anything wrong, but are unnecessary now that innerNode is always in the current document again.

[Allan Sandfeld Jensen]

Created attachment 164355 [details]
Follow-up Patch

Revert two additional lines of code. Keeping these would have been confusing, and they would have been essentially dead.

[WebKit Review Bot]

Comment on attachment 164236 [details]
Proposed revert.

Rejecting attachment 164236 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
frame.html,
which does not exist!  Applying it anyway.
patching file LayoutTests/touchadjustment/resources/inner-navigation-frame.html
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/touchadjustment/resources/inner-navigation-frame.html.rej
patching file ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Antonio Go..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/13868381

[Allan Sandfeld Jensen]

Comment on attachment 164236 [details]
Proposed revert.

clearing flags for landed patch

[WebKit Review Bot]

Comment on attachment 164355 [details]
Follow-up Patch

Clearing flags on attachment: 164355

Committed r128759: <http://trac.webkit.org/changeset/128759>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Comment on attachment 164355 [details]
Follow-up Patch

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

> Source/WebCore/dom/Document.cpp:6157
> +    ASSERT(innerNodeInDocument->document() == this);

We are hitting this assertion in several tests now (at least on EFL port) :
http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug/r128762%20(5986)/results.html

[Chris Dumez]

(In reply to comment #9)
> (From update of attachment 164355 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164355&action=review
> 
> > Source/WebCore/dom/Document.cpp:6157
> > +    ASSERT(innerNodeInDocument->document() == this);
> 
> We are hitting this assertion in several tests now (at least on EFL port) :
> http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug/r128762%20(5986)/results.html

It seems to hit the assertion on Mac as well:
http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20WK1%20%28Tests%29/builds/924

Exited early after 20 crashes.

[WebKit Review Bot]

Re-opened since this is blocked by 96929

[Allan Sandfeld Jensen]

Comment on attachment 164355 [details]
Follow-up Patch

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

>>> Source/WebCore/dom/Document.cpp:6157
>>> +    ASSERT(innerNodeInDocument->document() == this);
>> 
>> We are hitting this assertion in several tests now (at least on EFL port) :
>> http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug/r128762%20(5986)/results.html
> 
> It seems to hit the assertion on Mac as well:
> http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20WK1%20%28Tests%29/builds/924
> 
> Exited early after 20 crashes.

The assertion forgets to check for innerNode being null. ASSERT(!innerNodeInDocument || innerNodeInDocument->document() == this);

[Allan Sandfeld Jensen]

Last patch landed as http://trac.webkit.org/changeset/128869