Bug 164762 - UIScriptController: script with no async tasks fails if an earlier script registered a callback
Summary: UIScriptController: script with no async tasks fails if an earlier script reg...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on:
Blocks: 164766
  Show dependency treegraph
 
Reported: 2016-11-14 20:16 PST by Simon Fraser (smfr)
Modified: 2016-11-16 15:05 PST (History)
4 users (show)

See Also:


Attachments
Patch (5.70 KB, patch)
2016-11-15 16:10 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (29.39 KB, patch)
2016-11-16 12:29 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.uiScriptComplete();
                };
                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))
         unregisterCallback(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]
Patch
Comment 3 Simon Fraser (smfr) 2016-11-15 16:25:24 PST
https://trac.webkit.org/changeset/208770
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]
Patch
Comment 8 WebKit Commit Bot 2016-11-16 15:05:06 PST
Comment on attachment 294961 [details]
Patch

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.