Bug 151101 – UI-side scripts in WebKitTestRunner should wait until event handling completes before finishing

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Wenson Hsieh]

WebKitTestRunner may still crash in the scenario where a marker event is dequeued and handled after uiScriptComplete has been called. While this scenario should never happen when running iOS interaction tests serially on a single simulator, this seems to happen occasionally when tests are run in parallel. We can fix this by making UI script execution smart enough to defer script completion until all events (currently tap, double tap and hardware keyboard) have been handled.

[Wenson Hsieh]

<rdar://problem/23428601>

[Wenson Hsieh]

Created attachment 265200 [details]
Patch

[Tim Horton]

Comment on attachment 265200 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=265200&action=review

> Tools/WebKitTestRunner/UIScriptContext/Bindings/UIScriptController.idl:53
> -    void uiScriptComplete(DOMString result);
> +    void uiScriptCompleteAfterHandlingEvents(DOMString result);

This probably could have been an implementation detail of uiScriptComplete instead of requiring a rename. We don't list all the other things that happen between calling this and actually being /done/.

> Tools/WebKitTestRunner/UIScriptContext/UIScriptContext.h:53
>      unsigned prepareForAsyncTask(JSValueRef taskCallback);
> +    unsigned prepareForAsyncTaskBlockingScriptCompletion(JSValueRef taskCallback);

Do we ever want async tasks *not* to block uiScriptComplete? Is it ever bad if they do? It would be nice to only have one that always does the right thing, to make it harder to write a broken test (and not change the name like above).

[Build Bot]

Comment on attachment 265200 [details]
Patch

Attachment 265200 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/411197

New failing tests:
fast/harness/concurrent-ui-side-scripts.html
fast/harness/ui-side-scripts.html

[Build Bot]

Created attachment 265208 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Wenson Hsieh]

Created attachment 265272 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 265272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=265272&action=review

> Tools/WebKitTestRunner/UIScriptContext/UIScriptContext.h:49
> +const unsigned minimumPersistentCallbackID = 1000;
> +typedef enum  {
> +    CallbackTypeNonPersistent,
> +    CallbackTypeWillBeginZooming = minimumPersistentCallbackID,
> +    CallbackTypeDidEndZooming = minimumPersistentCallbackID + 1,
> +    CallbackTypeDidShowKeyboard = minimumPersistentCallbackID + 2,
> +    CallbackTypeDidHideKeyboard = minimumPersistentCallbackID + 3
> +} CallbackType;

I think it's better to reserve the low range for persistent callbacks; that way, the upper bound for other callbacks is much higher.

[Wenson Hsieh]

Comment on attachment 265272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=265272&action=review

>> Tools/WebKitTestRunner/UIScriptContext/UIScriptContext.h:49
>> +} CallbackType;
> 
> I think it's better to reserve the low range for persistent callbacks; that way, the upper bound for other callbacks is much higher.

That makes sense -- changed to use minimumNonPersistentCallbackID.

[Wenson Hsieh]

Created attachment 265286 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 265286 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=265286&action=review

> Tools/WebKitTestRunner/UIScriptContext/UIScriptContext.cpp:158
> +    m_uiScriptResultsPendingCompletion.add(m_currentScriptCallbackID, result ? JSStringRetain(result) : result);

: nullptr I think.

> Tools/WebKitTestRunner/UIScriptContext/UIScriptContext.cpp:188
> +    for (WTF::KeyValuePair<unsigned, Task> entry : m_callbacks) {

auto

> Tools/WebKitTestRunner/UIScriptContext/UIScriptContext.h:42
> +const unsigned minimumNonPersistentCallbackID = 1000;

How about firstNonPersistentCallbackID and a blank line after this.

[Wenson Hsieh]

Committed r192314: <http://trac.webkit.org/changeset/192314>