Bug 96830

Summary: Revert r127457 and following fixes due to several hit-testing regressions
Product: WebKit Reporter: Julien Chaffraix <jchaffraix>
Component: UI EventsAssignee: Allan Sandfeld Jensen <allan.jensen>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, ap, cdumez, eric, jamesr, jberlin, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 96929    
Bug Blocks: 95204    
Attachments:
Description Flags
Proposed revert.
none
Follow-up Patch none

Julien Chaffraix
Reported 2012-09-14 15:19:31 PDT
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
Attachments
Proposed revert. (39.93 KB, patch)
2012-09-14 15:26 PDT, Julien Chaffraix 		no flags
DetailsFormatted DiffDiff
Follow-up Patch (1.35 KB, patch)
2012-09-17 01:34 PDT, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Julien Chaffraix
Comment 1 2012-09-14 15:26:08 PDT
Created attachment 164236 [details] Proposed revert.
Julien Chaffraix
Comment 2 2012-09-14 18:24:17 PDT
Committed r128677: <http://trac.webkit.org/changeset/128677>
Allan Sandfeld Jensen
Comment 3 2012-09-16 07:35:32 PDT
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
Comment 4 2012-09-17 01:34:16 PDT
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 5 2012-09-17 01:36:44 PDT
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 6 2012-09-17 02:02:33 PDT
Comment on attachment 164236 [details] Proposed revert. clearing flags for landed patch
WebKit Review Bot
Comment 7 2012-09-17 08:04:20 PDT
Comment on attachment 164355 [details] Follow-up Patch Clearing flags on attachment: 164355 Committed r128759: <http://trac.webkit.org/changeset/128759>
WebKit Review Bot
Comment 8 2012-09-17 08:04:25 PDT
All reviewed patches have been landed. Closing bug.
Chris Dumez
Comment 9 2012-09-17 09:02:46 PDT
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
Comment 10 2012-09-17 09:17:17 PDT
(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
Comment 11 2012-09-17 09:21:20 PDT
Re-opened since this is blocked by 96929
Allan Sandfeld Jensen
Comment 12 2012-09-17 11:54:24 PDT
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
Comment 13 2012-09-18 03:30:24 PDT
Last patch landed as http://trac.webkit.org/changeset/128869
Note You need to log in before you can comment on or make changes to this bug.