Bug 32519 - mouseEvent fires mutiple times in LayoutTests/svg/custom/use-instanceRoot-as-event-target.xhtml
Summary: mouseEvent fires mutiple times in LayoutTests/svg/custom/use-instanceRoot-as-...
Status: RESOLVED DUPLICATE of bug 33835
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
Depends on:
Blocks: 32437
  Show dependency treegraph
Reported: 2009-12-14 08:12 PST by Robert Hogan
Modified: 2010-01-19 20:03 PST (History)
4 users (show)

See Also:

gzipped backtrace (13.30 KB, application/octet-stream)
2009-12-14 08:14 PST, Robert Hogan
no flags Details
Update Skipped Lists for Qt, Win and GTK (4.13 KB, patch)
2009-12-14 08:50 PST, Robert Hogan
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Updated patch (4.23 KB, patch)
2009-12-14 13:25 PST, Robert Hogan
eric: review-
Details | Formatted Diff | Diff
Corrected patch (4.19 KB, patch)
2009-12-14 13:37 PST, Robert Hogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2009-12-14 08:12:47 PST

is skipped in gtk and win, and if the patch in https://bugs.webkit.org/show_bug.cgi?id=32437 is accepted it will be in Qt too

In Qt the test was passing due to a bug in notifyDone that masked the multiple output of 'test 10' (i.e. the results of test 10 are output three times). I've confirmed that gtk also sees multiple output and if you try the test manually in the launcher you can see it there too. I believe this is the reason the test is skipped in both gtk and win.

In the case of Qt at least, notifyDone in the DRT gets called three times, which seems a bit too coincidental, and I've attached a bt of each call. Given that the test is all about adding/removing event listeners it looks like either the test is not managing the addition/removal correctly, or more likely WebCore is firing the event multiple times for some reason.
Comment 1 Robert Hogan 2009-12-14 08:14:17 PST
Created attachment 44798 [details]
gzipped backtrace
Comment 2 Robert Hogan 2009-12-14 08:50:54 PST
Created attachment 44800 [details]
Update Skipped Lists for Qt, Win and GTK
Comment 3 WebKit Review Bot 2009-12-14 08:51:47 PST
style-queue ran check-webkit-style on attachment 44800 [details] without any errors.
Comment 4 WebKit Commit Bot 2009-12-14 13:13:14 PST
Comment on attachment 44800 [details]
Update Skipped Lists for Qt, Win and GTK

Rejecting patch 44800 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Darin Adler', '--force']" exit_code: 1
Last 500 characters of output:
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/platform/gtk/Skipped
Hunk #2 succeeded at 5730 (offset -3 lines).
patching file LayoutTests/platform/qt/Skipped
Hunk #1 FAILED at 5187.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/qt/Skipped.rej
patching file LayoutTests/platform/win/Skipped
Hunk #2 FAILED at 723.
1 out of 2 hunks FAILED -- saving rejects to file LayoutTests/platform/win/Skipped.rej
Comment 5 Robert Hogan 2009-12-14 13:25:00 PST
Created attachment 44816 [details]
Updated patch
Comment 6 Eric Seidel (no email) 2009-12-14 13:27:08 PST
Comment on attachment 44816 [details]
Updated patch

Those paths look wrong.

They should not include "LayoutTests/" or?
Comment 7 Robert Hogan 2009-12-14 13:37:11 PST
Created attachment 44819 [details]
Corrected patch

They were - sorry about that.
Comment 8 Eric Seidel (no email) 2009-12-14 13:39:08 PST
Comment on attachment 44819 [details]
Corrected patch

Comment 9 WebKit Commit Bot 2009-12-14 13:57:09 PST
Comment on attachment 44819 [details]
Corrected patch

Clearing flags on attachment: 44819

Committed r52117: <http://trac.webkit.org/changeset/52117>
Comment 10 WebKit Commit Bot 2009-12-14 13:57:14 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Robert Hogan 2009-12-15 05:15:22 PST
Needs to stay open to track fix.
Comment 12 Eric Seidel (no email) 2009-12-28 21:37:11 PST
Comment on attachment 44800 [details]
Update Skipped Lists for Qt, Win and GTK

Clearing Darin's r+ on this obsolete patch.
Comment 13 Robert Hogan 2009-12-29 13:05:01 PST
After a bit more digging it appears that you can force this test to pass in Qt/Gtk if you remove the following line:

useElement.instanceRoot.addEventListener("click", eventHandler, false);

It will also pass if you also remove the line above it (leaving both lines commented out):

rectElement.removeEventListener("click", eventHandler, false);

With the unmodified test, printing the timestamp of each event that prints the final test shows that both rectElement and useElement fire an event on fireSimulatedMouseClick(). If you don't add an event listener to useElement it passes. And if you try to remove the rectElement listener it still passes, when it should really timeout. The multiple outputs of test 10 under Qt/GTK are happening because the 'click' event listener associated with rectElement cannot be removed for some reason. (It's defined in the SVG markup rather than added dynamically, which must be the problem). It could be that event listeners 

I suspect this is only passing for other ports due to notifyDone() causing the DRTs to finish early rather than wait for all output.

Unfortunately, gdb segfaults on --svg builds for me at the moment so I can't investigate further.
Comment 14 Eric Seidel (no email) 2009-12-29 13:29:04 PST
notifyDone() does cause the testing to end right then.  If it doesn't for the Qt DRT, then that's a bug.  We might need to better document what notifyDone() does.
Comment 15 Robert Hogan 2009-12-29 16:39:10 PST
(In reply to comment #14)
> notifyDone() does cause the testing to end right then.  If it doesn't for the
> Qt DRT, then that's a bug.  We might need to better document what notifyDone()
> does.

In this case I think that the DRT sticking around for all output reveals that the test is actually failing. The detail for the penultimate test is "Test #9: Verify that the original click event listener got removed and a new one attached to the use root SVGElementInstance". If you remove one of the lines that set up test 9:

rectElement.removeEventListener("click", eventHandler, false);
//useElement.instanceRoot.addEventListener("click", eventHandler, false);

it still passes, when it shouldn't. Because the last few tests depend on adding/removing 'click' events, and removing the click event defined in markup has no effect, the 'click' event ends up firing multiple times for each click - when the test comes to an end, the event handler function in the test is still processing the events and sends them all to the last test in the script:

function eventHandler(evt)
    if (evt.type != nextEventType)
    currentEvent = evt;

Modiying the above as follows also 'fixes' the test by choking off these surplus events that shouldn't be happening.

function eventHandler(evt)
    if (evt.type != nextEventType)
    nextEventType = undefined;
    currentEvent = evt;
Comment 16 Nikolas Zimmermann 2010-01-19 20:03:25 PST
A fix for all current stability problems with <use> tests is waiting for review at bug 33835.

*** This bug has been marked as a duplicate of bug 33835 ***