Bug 141108 - Stop dispatching events with SVGElementInstance objects as their targets
Summary: Stop dispatching events with SVGElementInstance objects as their targets
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on: 141144
Blocks: 140602
  Show dependency treegraph
 
Reported: 2015-01-30 19:44 PST by Darin Adler
Modified: 2015-02-07 17:46 PST (History)
14 users (show)

See Also:


Attachments
Patch (291.36 KB, patch)
2015-01-30 19:49 PST, Darin Adler
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (688.79 KB, application/zip)
2015-01-30 20:16 PST, Build Bot
no flags Details
Patch (291.45 KB, patch)
2015-01-31 12:37 PST, Darin Adler
andersca: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-mavericks-wk2 (691.72 KB, application/zip)
2015-01-31 13:06 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2015-01-30 19:44:10 PST
Stop dispatching events to with SVGElementInstance objects as their targets
Comment 1 Darin Adler 2015-01-30 19:49:07 PST
Created attachment 245767 [details]
Patch
Comment 2 Build Bot 2015-01-30 20:16:00 PST
Comment on attachment 245767 [details]
Patch

Attachment 245767 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5344422917046272

New failing tests:
svg/custom/use-events-crash.svg
Comment 3 Build Bot 2015-01-30 20:16:05 PST
Created attachment 245768 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 4 Darin Adler 2015-01-30 22:30:47 PST
Strange, use-events-crash did not fail locally on my machine.
Comment 5 Darin Adler 2015-01-31 12:37:12 PST
Created attachment 245793 [details]
Patch
Comment 6 Build Bot 2015-01-31 13:06:17 PST
Comment on attachment 245793 [details]
Patch

Attachment 245793 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5983997234511872

New failing tests:
svg/custom/use-events-crash.svg
Comment 7 Build Bot 2015-01-31 13:06:22 PST
Created attachment 245795 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 8 Darin Adler 2015-02-01 07:14:23 PST
These test failures are timeouts. I am seeing some intermittent timeouts on SVG tests. One time it was svg/custom/composited-svg-with-opacity.html instead of this one. I don’t see any clue about what is causing the timeout in either case. This is just a simple webpage with a script on it. The whole point of the test is to check for a crash.
Comment 9 Darin Adler 2015-02-01 07:22:53 PST
Committed r179467: <http://trac.webkit.org/changeset/179467>
Comment 10 Alexey Proskuryakov 2015-02-01 10:58:50 PST
svg/custom/use-events-crash.svg now times out on all WebKit2 bots, as previously suggested by EWS.

Darin, are you available to investigate?
Comment 11 Alexey Proskuryakov 2015-02-01 14:02:49 PST
This test runs right after the new test added in this patch, svg/custom/use-event-retargeting.html. Perhaps the new test somehow breaks subsequent ones? That would still indicate a bug in DOM code, because WebKitTestRunner verifies that it can successfully load a blank page between tests.

I'm going to try skipping the new test too see if that helps.
Comment 12 Alexey Proskuryakov 2015-02-01 15:38:21 PST
That didn't help. Will roll out.
Comment 13 WebKit Commit Bot 2015-02-01 15:42:10 PST
Re-opened since this is blocked by bug 141144
Comment 14 Darin Adler 2015-02-02 08:18:06 PST
I have no leads about why this test is timing out on bots but not my local machine. The patch should have no effect on this test; it only changes one function and its changes only affect which object <use> element events go to. And the only point of the test is to see if an assertion is hit.

I think this patch and the entire "remove SVGElementInstance" project will now be delayed indefinitely until someone can figure out what's going on.
Comment 15 Darin Adler 2015-02-02 08:59:44 PST
My current guess is that the timing out of the test was caused by the change in the exact number and order of other tests in svg/custom. Maybe the removal of some tests that is part of this patch.
Comment 16 Alexey Proskuryakov 2015-02-02 17:26:02 PST
I tried to collect more info from the bots, but couldn't find anything else.

This unusual in that both EWS and regression testing bots were affected - these have a different number of cores, and thus rarely hit the same test dependency issues. Yet, this happened every time on all WebKit2 testers.
Comment 17 Darin Adler 2015-02-03 09:17:53 PST
I think the next step is to separate out only the layout test changes, presumably with some expected failures, and try to land those without the code change.
Comment 18 Alexey Proskuryakov 2015-02-03 11:43:16 PST
I updated to 179467, and could reproduce the timeout locally with "run-webkit-tests -2 svg/custom".

Furthermore, this test times out when run individually, so it's not something that earlier tests affect. I should have guessed, because EWS detected the failure, and EWS glosses over failures that don't reproduce on retry.
Comment 19 Alexey Proskuryakov 2015-02-03 11:45:28 PST
This test now displays a popup menu on screen, and waits for user input in a nested run loop!
Comment 20 Darin Adler 2015-02-04 08:56:37 PST
(In reply to comment #19)
> This test now displays a popup menu on screen, and waits for user input in a
> nested run loop!

Wow! That explains everything. The test was relying on right click not doing anything. I wonder what I should do with this test. Probably just delete it?
Comment 21 Darin Adler 2015-02-04 08:59:44 PST
Why doesn’t this happen when I run the test on my machine? I‘ve done that many times!
Comment 22 Alexey Proskuryakov 2015-02-04 09:54:40 PST
What was the command you used to run the test?

Since this only happens with WebKit2 and not WebKit1, I suspect that it's a deficiency in WebKitTestRunner that it doesn't block context menus. But I don't see such code anywhere in DumpRenderTree either. So, I'm puzzled too.
Comment 23 Darin Adler 2015-02-05 11:29:29 PST
(In reply to comment #22)
> Since this only happens with WebKit2 and not WebKit1

Aha! I did not notice that detail. I was definitely running with legacy WebKit, not modern.
Comment 24 Darin Adler 2015-02-07 14:20:16 PST
This is driving me crazy. When I run this:

run-webkit-tests -2 svg/custom/use-event-retargeting.html

I do not get a timeout. When I run this:

run-webkit-tests -2 svg/custom

I get lots of timeouts, not just on one test.
Comment 25 Darin Adler 2015-02-07 14:23:46 PST
OK, that was the wrong thing to try. Now trying:

run-webkit-tests -2 svg/custom/use-events-crash.svg
Comment 26 Darin Adler 2015-02-07 15:17:32 PST
Committed r179785: <http://trac.webkit.org/changeset/179785>
Comment 27 Alexey Proskuryakov 2015-02-07 16:47:30 PST
Unfortunately, svg/custom/use-events-crash.svg times out on WebKit2 bots again.

I don't understand how this change could fix that - I don't think that we have any mechanism for dispatching an event into a nested run loop, other than for drag and drop.
Comment 28 Darin Adler 2015-02-07 17:15:53 PST
(In reply to comment #27)
> Unfortunately, svg/custom/use-events-crash.svg times out on WebKit2 bots
> again.
> 
> I don't understand how this change could fix that - I don't think that we
> have any mechanism for dispatching an event into a nested run loop, other
> than for drag and drop.

Yes, we have been discussing this on IRC for the last hour or so.

The change does fix the timeout. Locally the timeout was 100% reproducible and after the change the test passed with no timeout. I do think we have a mechanism for dispatching the event.

The bots say the test is failing now, *not* timing out, as far as I can tell. But the test results are not uploading so I can’t see why the expected result isn’t matching.

I suggest we delete the test, but I would love to talk this over with you on IRC.
Comment 29 Darin Adler 2015-02-07 17:20:16 PST
Zalan says I am wrong and it actually is timing out.

This test has no value. It checks for a bug in code I have deleted. Lets just delete it.
Comment 30 Darin Adler 2015-02-07 17:22:55 PST
I definitely would not have re-landed the patch if I hadn’t:

1) reproduced the timeout locally

2) made a change that given how we split things between UI and web process seemed like it could fix the timeout

3) seen the test pass locally after the change

But somehow it seems that I still got something wrong. I have no interest, though, in doing more work to keep the test around. Zalan is going to help me out by deleting the test.
Comment 31 Darin Adler 2015-02-07 17:35:50 PST
Darins-Home-iMac:OpenSource darin$ run-webkit-tests -2 svg/custom/use-events-crash.svg

Using port 'mac-yosemite-wk2'
Test configuration: <yosemite, x86_64, debug>
Placing test results in /Users/darin/Build/Debug/layout-test-results
Baseline search path: mac-wk2 -> wk2 -> mac -> generic
Using Debug build
Pixel tests disabled
Regular timeout: 30000, slow test timeout: 150000
Command line: /Users/darin/Build/Debug/WebKitTestRunner -

Found 1 test; running 1, skipping 0.
Running 1 WebKitTestRunner.     

The test ran as expected.
Comment 32 Alexey Proskuryakov 2015-02-07 17:36:27 PST
Sorry, I can't get on IRC soon.

Yes, it was already quite clear that this change fixed the test for you locally. It certainly seems right to delete a test that has no value.

I think that the most interesting parts here are 

1. why the test didn't fail in WebKit1, and 
2. given that it almost worked for you with the fix, maybe we can make it possible to test context menus.

Currently, context menu tests are just skipped on WebKit2, together with a lot of other eventSender tests. That's a lot of missing test coverage.
Comment 33 zalan 2015-02-07 17:42:17 PST
http://trac.webkit.org/changeset/179790 removed svg/custom/use-events-crash.svg
Comment 34 Darin Adler 2015-02-07 17:46:54 PST
(In reply to comment #32)
> Currently, context menu tests are just skipped on WebKit2, together with a
> lot of other eventSender tests. That's a lot of missing test coverage.

I see. Maybe I could look into this once I have finished the SVGElementInstance task.