Bug 45814 – fast/dom/DeviceOrientation/add-listener-from-callback flaky

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Jonathan Dixon]

This test is sometimes failing with text difference:

2010-09-15 01:44:47,251 executive.py:301  DEBUG "/usr/bin/wdiff --start-delete=##WDIFF_DEL## --end-delete=##WDIFF_END## --start-insert=##WDIFF_ADD## --end-insert=##WDIFF_END## /b/slave/webkit-rel-linux/build/src/webkit/Release/../../../layout-test-results/fast/dom/DeviceOrientation/add-listener-from-callback-expected.txt /b/slave/webkit-rel-linux/build/src/webkit/Release/../../../layout-test-results/fast/dom/DeviceOrientation/add-listener-from-callback-actual.txt" took 0.01s
2010-09-15 01:44:47,252 dump_render_tree_thread.py:408  DEBUG Thread-2 fast/dom/DeviceOrientation/add-listener-from-callback.html failed:
  Text diff mismatch
2010-09-15 01:44:47,258 printing.py:535  INFO   fast/dom/DeviceOrientation/add-listener-from-callback.html -> unexpected text diff mismatch

(http://buildbot.jail.google.com/buildbot/chromium/builders/Webkit%20Linux/builds/29744/steps/webkit_tests/logs/stdio)

[Hans Wennborg]

Created attachment 67677 [details]
Patch

[Steve Block]

Comment on attachment 67677 [details]
Patch

> +        There was a race in the add-listener-from-callback test: it used to do
> +        setMockOrientation() and then window.addEventListner(), but since the
> +        event caused by setMockOrientation() is fired asynchronously, it would
> +        sometimes be fired after the new event listener was added, causing the
> +        test to receive one more event than it expected.
You're right that the test is flaky, but I think this reasoning is wrong. Both the calls to setMockDeviceOrientation() and addEventListener() will trigger events to fire asynchronously. The order is deterministic based on the the order in which the functions are called.

The problem is that when we call addEventListener(), an event is sent to all registered event listeners on that page (whether this is the correct behaviour is questionable, but should be addressed elsewhere I think). So in the existing test, both the calls to setMockDeviceOrientation() and addEventListener() will trigger events to fire asynchronously on both listeners (both will be registered by the time the event fires), giving a total of five events. The race condition is whether the test finishes after the third event, before the fourth event fires.

>      if (++firstListenerEvents == 1)
>          setMockOrientation(11.1, 22.2, 33.3);
I think I was wrong to update the mock orientation - it's extra complication and not the point of this test.

> -    else if (firstListenerEvents > 2)
> +    else if (firstListenerEvents == 2)
> +        window.addEventListener('deviceorientation', secondListener);
I think the best fix is to just add the second listener from the first listener, and check that a total of three events fire. I already have a patch and will upload it.

[Steve Block]

Created attachment 67679 [details]
Patch

[Hans Wennborg]

(In reply to comment #3)
> Created an attachment (id=67679) [details]
> Patch

Ah, this is much simpler. I'd r+ if I could :)

[Jeremy Orlow]

Comment on attachment 67679 [details]
Patch

r=me

[Steve Block]

Committed r67557: <http://trac.webkit.org/changeset/67557>