Bug 164762

Summary: UIScriptController: script with no async tasks fails if an earlier script registered a callback
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: Tools / TestsAssignee: Simon Fraser (smfr) <simon.fraser>
Severity: Normal CC: commit-queue, lforschler, ryanhaddad, simon.fraser
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 164766    
Description Flags
Patch none

Description Simon Fraser (smfr) 2016-11-14 20:16:46 PST
If one UI script registers a callback:

        function getScrollDownUIScript(scrollX, scrollY)
            return `(function() {
                uiController.didEndScrollingCallback = function() {
                uiController.scrollToOffset(${scrollX}, ${scrollY}, function() {});

then a later one just wants to return data:

        function getResultsUIScript()
            return `(function() {
                var results = {
                    zoomScale : uiController.zoomScale,
                    visibleRect : uiController.contentVisibleRect,
                    scrollingTree : uiController.scrollingTreeAsText

                return JSON.stringify(results);

that later one never returns because UIScriptContext::runUIScript() thinks there's an outstanding task.
Comment 1 Simon Fraser (smfr) 2016-11-14 21:15:21 PST
Partial fix:

diff --git a/Tools/TestRunnerShared/UIScriptContext/UIScriptContext.cpp b/Tools/TestRunnerShared/UIScriptContext/UIScriptContext.cpp
index e2e7b0d1375c4def7e0592faf02c4d65fd9ed887..bb5c9e8becaea698f785a3e5bde20842e6549af7 100644
--- a/Tools/TestRunnerShared/UIScriptContext/UIScriptContext.cpp
+++ b/Tools/TestRunnerShared/UIScriptContext/UIScriptContext.cpp
@@ -117,6 +117,9 @@ unsigned UIScriptContext::registerCallback(JSValueRef taskCallback, CallbackType
     if (m_callbacks.contains(type))
+    if (JSValueIsUndefined(m_context.get(), taskCallback))
+        return 0;
     return prepareForAsyncTask(taskCallback, type);
Comment 2 Simon Fraser (smfr) 2016-11-15 16:10:40 PST
Created attachment 294893 [details]
Comment 3 Simon Fraser (smfr) 2016-11-15 16:25:24 PST
Comment 4 Ryan Haddad 2016-11-16 10:00:10 PST
Reverted r208770 for reason:

The test added with this change is timing out on mac-wk1.

Committed r208795: <http://trac.webkit.org/changeset/208795>
Comment 5 Ryan Haddad 2016-11-16 10:03:28 PST
(In reply to comment #4)
> Reverted r208770 for reason:
> The test added with this change is timing out on mac-wk1.
> Committed r208795: <http://trac.webkit.org/changeset/208795>

The test added with this change was timing out on mac-wk1 and causing false positives for other patches. It also looks like the original change was landed before EWS had a chance to complete testing in the first place.
Comment 6 Simon Fraser (smfr) 2016-11-16 10:22:23 PST
The test should be skipped on mac WK1. I'll fix and re-land.
Comment 7 Simon Fraser (smfr) 2016-11-16 12:29:23 PST
Created attachment 294961 [details]
Comment 8 WebKit Commit Bot 2016-11-16 15:05:06 PST
Comment on attachment 294961 [details]

Clearing flags on attachment: 294961

Committed r208816: <http://trac.webkit.org/changeset/208816>
Comment 9 WebKit Commit Bot 2016-11-16 15:05:10 PST
All reviewed patches have been landed.  Closing bug.