Bug 141108 – Stop dispatching events with SVGElementInstance objects as their targets

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Darin Adler]

Stop dispatching events to with SVGElementInstance objects as their targets

[Darin Adler]

Created attachment 245767 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

Strange, use-events-crash did not fail locally on my machine.

[Darin Adler]

Created attachment 245793 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

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.

[Darin Adler]

Committed r179467: <http://trac.webkit.org/changeset/179467>

[Alexey Proskuryakov]

svg/custom/use-events-crash.svg now times out on all WebKit2 bots, as previously suggested by EWS.

Darin, are you available to investigate?

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

That didn't help. Will roll out.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 141144

[Darin Adler]

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.

[Darin Adler]

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.

[Alexey Proskuryakov]

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.

[Darin Adler]

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.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

This test now displays a popup menu on screen, and waits for user input in a nested run loop!

[Darin Adler]

(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?

[Darin Adler]

Why doesn’t this happen when I run the test on my machine? I‘ve done that many times!

[Alexey Proskuryakov]

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.

[Darin Adler]

(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.

[Darin Adler]

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.

[Darin Adler]

OK, that was the wrong thing to try. Now trying:

run-webkit-tests -2 svg/custom/use-events-crash.svg

[Darin Adler]

Committed r179785: <http://trac.webkit.org/changeset/179785>

[Alexey Proskuryakov]

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.

[Darin Adler]

(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.

[Darin Adler]

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.

[Darin Adler]

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.

[Darin Adler]

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.

[Alexey Proskuryakov]

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.

[zalan]

http://trac.webkit.org/changeset/179790 removed svg/custom/use-events-crash.svg

[Darin Adler]

(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.