Bug 32519 – mouseEvent fires mutiple times in LayoutTests/svg/custom/use-instanceRoot-as-event-target.xhtml

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebCore JavaScript

[Robert Hogan]

LayoutTests/svg/custom/use-instanceRoot-as-event-target.xhtml

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.

[Robert Hogan]

Created attachment 44798 [details]
gzipped backtrace

[Robert Hogan]

Created attachment 44800 [details]
Update Skipped Lists for Qt, Win and GTK

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44800 [details] without any errors.

[WebKit Commit Bot]

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

[Robert Hogan]

Created attachment 44816 [details]
Updated patch

[Eric Seidel (no email)]

Comment on attachment 44816 [details]
Updated patch

Those paths look wrong.

They should not include "LayoutTests/" or?

[Robert Hogan]

Created attachment 44819 [details]
Corrected patch

They were - sorry about that.

[Eric Seidel (no email)]

Comment on attachment 44819 [details]
Corrected patch

OK.

[WebKit Commit Bot]

Comment on attachment 44819 [details]
Corrected patch

Clearing flags on attachment: 44819

Committed r52117: <http://trac.webkit.org/changeset/52117>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Robert Hogan]

Needs to stay open to track fix.

[Eric Seidel (no email)]

Comment on attachment 44800 [details]
Update Skipped Lists for Qt, Win and GTK

Clearing Darin's r+ on this obsolete patch.

[Robert Hogan]

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.

[Eric Seidel (no email)]

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.

[Robert Hogan]

(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)
        return;
    currentEvent = evt;
    eval(eventNotification);
}

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)
        return;
    nextEventType = undefined;
    currentEvent = evt;
    eval(eventNotification);
}

[Nikolas Zimmermann]

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 ***