Changeset 228983 in webkit


Ignore:
Timestamp:
Feb 25, 2018 6:18:24 PM (6 years ago)
Author:
ap@apple.com
Message:

Various crashes in WebKitTestRunner, especially when system is under heavy load
​https://bugs.webkit.org/show_bug.cgi?id=183109

Reviewed by Tim Horton.

WebKitTestRunner had many places where it sent messages to WebContent with a timeout,
but it didn't handle the timeout when it did occur. Nearly all of those would result
in logic errors and failing tests, and most would even result in stack corruption,
as the response handler modified local variables.

There is only one timeout scenario that we actually mean to handle in WKTR. That's
when a test freezes after it is done (e.g. an infinite loop in beforeunload) - we don't
want to blame the next test for freezing, so we silently relaunch WebContent.
Everything else is cargo cult code that never worked.

This patch addresses the crashes, and actually makes tests pass a lot more on an
overloaded system.

  • WebKitTestRunner/TestController.cpp:

(WTR::TestController::resetStateToConsistentValues): Moved m_doneResetting assignment
to where it's actually needed, for clarity.
(WTR::TestController::reattachPageToWebProcess): This function used to always hit
and ignore message timeout, as m_doneResetting is only updated by navigation callback
when the state is Resetting. This change makes it faster.
(WTR::TestController::platformResetStateToConsistentValues): Style fix.
(WTR::TestController::clearServiceWorkerRegistrations): Timing out here wasn't
handled in a meaningful manner, and would even corrupt the stack.
(WTR::TestController::clearDOMCache): Ditto.
(WTR::TestController::clearDOMCaches): Ditto.
(WTR::TestController::hasDOMCache): Ditto.
(WTR::TestController::domCacheSize): Ditto.
(WTR::TestController::isStatisticsPrevalentResource): Ditto.
(WTR::TestController::isStatisticsRegisteredAsSubFrameUnder): Ditto.
(WTR::TestController::isStatisticsRegisteredAsRedirectingTo): Ditto.
(WTR::TestController::isStatisticsHasHadUserInteraction): Ditto.
(WTR::TestController::isStatisticsGrandfathered): Ditto.
(WTR::TestController::statisticsUpdateCookiePartitioning): Ditto.
(WTR::TestController::statisticsSetShouldPartitionCookiesForHost): Ditto.
(WTR::TestController::statisticsClearInMemoryAndPersistentStore): Ditto.
(WTR::TestController::statisticsClearInMemoryAndPersistentStoreModifiedSinceHours): Ditto.
(WTR::TestController::statisticsClearThroughWebsiteDataRemoval): Ditto.

  • WebKitTestRunner/TestInvocation.cpp:

(WTR::TestInvocation::shortTimeout const): Made shortTimeout shorter (on a hunch).
(WTR::TestInvocation::invoke): Removed a timeout waiting for initial response. There
is never a logical reason for such a timeout, as we always have a new or responsive
WebContent process here.
(WTR::TestInvocation::dumpResults): Removed another timeout that we don't know how to
properly handle.
(WTR::TestInvocation::didReceiveMessageFromInjectedBundle): Removed assignment to
m_errorMessage, which had no effect in this context.

  • WebKitTestRunner/TestInvocation.h: Removed no longer used code.
  • WebKitTestRunner/cocoa/TestControllerCocoa.mm:

(WTR::TestController::cocoaResetStateToConsistentValues): Use a named constant for
no timeout.

  • WebKitTestRunner/ios/TestControllerIOS.mm:

(WTR::TestController::platformConfigureViewForTest): Removed a useless timeout.
Not sure if timing out here would corrupt the stack or not, but there is no reason
to impose arbitrary limits on individual steps of a test.

  • WebKitTestRunner/mac/TestControllerMac.mm:

(WTR::TestController::platformConfigureViewForTest): Use a named constant for
no timeout.

Location:
trunk/Tools
Files:
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/Tools/ChangeLog

    r228980 r228983  
     12018-02-25  Alexey Proskuryakov  <ap@apple.com>
     2
     3        Various crashes in WebKitTestRunner, especially when system is under heavy load
     4        https://bugs.webkit.org/show_bug.cgi?id=183109
     5
     6        Reviewed by Tim Horton.
     7
     8        WebKitTestRunner had many places where it sent messages to WebContent with a timeout,
     9        but it didn't handle the timeout when it did occur. Nearly all of those would result
     10        in logic errors and failing tests, and most would even result in stack corruption,
     11        as the response handler modified local variables.
     12
     13        There is only one timeout scenario that we actually mean to handle in WKTR. That's
     14        when a test freezes after it is done (e.g. an infinite loop in beforeunload) - we don't
     15        want to blame the next test for freezing, so we silently relaunch WebContent.
     16        Everything else is cargo cult code that never worked.
     17
     18        This patch addresses the crashes, and actually makes tests pass a lot more on an
     19        overloaded system.
     20
     21        * WebKitTestRunner/TestController.cpp:
     22        (WTR::TestController::resetStateToConsistentValues): Moved m_doneResetting assignment
     23        to where it's actually needed, for clarity.
     24        (WTR::TestController::reattachPageToWebProcess): This function used to always hit
     25        and ignore message timeout, as m_doneResetting is only updated by navigation callback
     26        when the state is Resetting. This change makes it faster.
     27        (WTR::TestController::platformResetStateToConsistentValues): Style fix.
     28        (WTR::TestController::clearServiceWorkerRegistrations): Timing out here wasn't
     29        handled in a meaningful manner, and would even corrupt the stack.
     30        (WTR::TestController::clearDOMCache): Ditto.
     31        (WTR::TestController::clearDOMCaches): Ditto.
     32        (WTR::TestController::hasDOMCache): Ditto.
     33        (WTR::TestController::domCacheSize): Ditto.
     34        (WTR::TestController::isStatisticsPrevalentResource): Ditto.
     35        (WTR::TestController::isStatisticsRegisteredAsSubFrameUnder): Ditto.
     36        (WTR::TestController::isStatisticsRegisteredAsRedirectingTo): Ditto.
     37        (WTR::TestController::isStatisticsHasHadUserInteraction): Ditto.
     38        (WTR::TestController::isStatisticsGrandfathered): Ditto.
     39        (WTR::TestController::statisticsUpdateCookiePartitioning): Ditto.
     40        (WTR::TestController::statisticsSetShouldPartitionCookiesForHost): Ditto.
     41        (WTR::TestController::statisticsClearInMemoryAndPersistentStore): Ditto.
     42        (WTR::TestController::statisticsClearInMemoryAndPersistentStoreModifiedSinceHours): Ditto.
     43        (WTR::TestController::statisticsClearThroughWebsiteDataRemoval): Ditto.
     44
     45        * WebKitTestRunner/TestInvocation.cpp:
     46        (WTR::TestInvocation::shortTimeout const): Made shortTimeout shorter (on a hunch).
     47        (WTR::TestInvocation::invoke): Removed a timeout waiting for initial response. There
     48        is never a logical reason for such a timeout, as we always have a new or responsive
     49        WebContent process here.
     50        (WTR::TestInvocation::dumpResults): Removed another timeout that we don't know how to
     51        properly handle.
     52        (WTR::TestInvocation::didReceiveMessageFromInjectedBundle): Removed assignment to
     53        m_errorMessage, which had no effect in this context.
     54
     55        * WebKitTestRunner/TestInvocation.h: Removed no longer used code.
     56
     57        * WebKitTestRunner/cocoa/TestControllerCocoa.mm:
     58        (WTR::TestController::cocoaResetStateToConsistentValues): Use a named constant for
     59        no timeout.
     60       
     61        * WebKitTestRunner/ios/TestControllerIOS.mm:
     62        (WTR::TestController::platformConfigureViewForTest): Removed a useless timeout.
     63        Not sure if timing out here would corrupt the stack or not, but there is no reason
     64        to impose arbitrary limits on individual steps of a test.
     65
     66        * WebKitTestRunner/mac/TestControllerMac.mm:
     67        (WTR::TestController::platformConfigureViewForTest): Use a named constant for
     68        no timeout.
     69
    1702018-02-25  Ali Juma  <ajuma@chromium.org>
    271

  • trunk/Tools/WebKitTestRunner/TestController.cpp

    r228881 r228983  
    854854    platformResetStateToConsistentValues();
    855855
    856     // Reset main page back to about:blank
    857     m_doneResetting = false;
    858 
    859856    m_shouldDecideNavigationPolicyAfterDelay = false;
    860857
    … …  
    867864    statisticsResetToConsistentState();
    868865
     866    // Reset main page back to about:blank
     867    m_doneResetting = false;
    869868    WKPageLoadURL(m_mainWebView->page(), blankURL());
    870869    runUntil(m_doneResetting, m_currentInvocation->shortTimeout());
    … …  
    880879{
    881880    // Loading a web page is the only way to reattach an existing page to a process.
     881    SetForScope<State> changeState(m_state, Resetting);
    882882    m_doneResetting = false;
    883883    WKPageLoadURL(m_mainWebView->page(), blankURL());
    884     runUntil(m_doneResetting, m_currentInvocation->shortTimeout());
     884    runUntil(m_doneResetting, noTimeout);
    885885}
    886886
    … …  
    23682368void TestController::platformResetStateToConsistentValues()
    23692369{
    2370 
    23712370}
    23722371
    … …  
    24082407
    24092408    WKWebsiteDataStoreRemoveAllServiceWorkerRegistrations(websiteDataStore, &context, clearServiceWorkerRegistrationsCallback);
    2410 
    2411     if (!context.done)
    2412         runUntil(context.done, m_currentInvocation->shortTimeout());
     2409    runUntil(context.done, noTimeout);
    24132410#endif
    24142411}
    … …  
    24412438    auto cacheOrigin = adoptWK(WKSecurityOriginCreateFromString(origin));
    24422439    WKWebsiteDataStoreRemoveFetchCacheForOrigin(websiteDataStore, cacheOrigin.get(), &context, clearDOMCacheCallback);
    2443 
    2444     if (!context.done)
    2445         runUntil(context.done, m_currentInvocation->shortTimeout());
     2440    runUntil(context.done, noTimeout);
    24462441#endif
    24472442}
    … …  
    24542449
    24552450    WKWebsiteDataStoreRemoveAllFetchCaches(websiteDataStore, &context, clearDOMCacheCallback);
    2456     if (!context.done)
    2457         runUntil(context.done, m_currentInvocation->shortTimeout());
     2451    runUntil(context.done, noTimeout);
    24582452#endif
    24592453}
    … …  
    24922486    FetchCacheOriginsCallbackContext context(*this, origin);
    24932487    WKWebsiteDataStoreGetFetchCacheOrigins(dataStore, &context, fetchCacheOriginsCallback);
    2494     if (!context.done)
    2495         runUntil(context.done, m_currentInvocation->shortTimeout());
     2488    runUntil(context.done, noTimeout);
    24962489    return context.result;
    24972490}
    … …  
    25222515    FetchCacheSizeForOriginCallbackContext context(*this);
    25232516    WKWebsiteDataStoreGetFetchCacheSizeForOrigin(dataStore, origin, &context, fetchCacheSizeForOriginCallback);
    2524     if (!context.done)
    2525         runUntil(context.done, m_currentInvocation->shortTimeout());
     2517    runUntil(context.done, noTimeout);
    25262518    return context.result;
    25272519}
    … …  
    25702562    ResourceStatisticsCallbackContext context(*this);
    25712563    WKWebsiteDataStoreIsStatisticsPrevalentResource(dataStore, host, &context, resourceStatisticsBooleanResultCallback);
    2572     if (!context.done)
    2573         runUntil(context.done, m_currentInvocation->shortTimeout());
     2564    runUntil(context.done, noTimeout);
    25742565    return context.result;
    25752566}
    … …  
    25802571    ResourceStatisticsCallbackContext context(*this);
    25812572    WKWebsiteDataStoreIsStatisticsRegisteredAsSubFrameUnder(dataStore, subFrameHost, topFrameHost, &context, resourceStatisticsBooleanResultCallback);
    2582     if (!context.done)
    2583         runUntil(context.done, m_currentInvocation->shortTimeout());
     2573    runUntil(context.done, noTimeout);
    25842574    return context.result;
    25852575}
    … …  
    25902580    ResourceStatisticsCallbackContext context(*this);
    25912581    WKWebsiteDataStoreIsStatisticsRegisteredAsRedirectingTo(dataStore, hostRedirectedFrom, hostRedirectedTo, &context, resourceStatisticsBooleanResultCallback);
    2592     if (!context.done)
    2593         runUntil(context.done, m_currentInvocation->shortTimeout());
     2582    runUntil(context.done, noTimeout);
    25942583    return context.result;
    25952584}
    … …  
    26122601    ResourceStatisticsCallbackContext context(*this);
    26132602    WKWebsiteDataStoreIsStatisticsHasHadUserInteraction(dataStore, host, &context, resourceStatisticsBooleanResultCallback);
    2614     if (!context.done)
    2615         runUntil(context.done, m_currentInvocation->shortTimeout());
     2603    runUntil(context.done, noTimeout);
    26162604    return context.result;
    26172605}
    … …  
    26282616    ResourceStatisticsCallbackContext context(*this);
    26292617    WKWebsiteDataStoreIsStatisticsGrandfathered(dataStore, host, &context, resourceStatisticsBooleanResultCallback);
    2630     if (!context.done)
    2631         runUntil(context.done, m_currentInvocation->shortTimeout());
     2618    runUntil(context.done, noTimeout);
    26322619    return context.result;
    26332620}
    … …  
    26922679    ResourceStatisticsCallbackContext context(*this);
    26932680    WKWebsiteDataStoreStatisticsUpdateCookiePartitioning(dataStore, &context, resourceStatisticsVoidResultCallback);
    2694     if (!context.done)
    2695         runUntil(context.done, m_currentInvocation->shortTimeout());
     2681    runUntil(context.done, noTimeout);
    26962682    m_currentInvocation->didSetPartitionOrBlockCookiesForHost();
    26972683}
    … …  
    27022688    ResourceStatisticsCallbackContext context(*this);
    27032689    WKWebsiteDataStoreSetStatisticsShouldPartitionCookiesForHost(dataStore, host, value, &context, resourceStatisticsVoidResultCallback);
    2704     if (!context.done)
    2705         runUntil(context.done, m_currentInvocation->shortTimeout());
     2690    runUntil(context.done, noTimeout);
    27062691    m_currentInvocation->didSetPartitionOrBlockCookiesForHost();
    27072692}
    … …  
    27602745    ResourceStatisticsCallbackContext context(*this);
    27612746    WKWebsiteDataStoreStatisticsClearInMemoryAndPersistentStore(dataStore, &context, resourceStatisticsVoidResultCallback);
    2762     if (!context.done)
    2763         runUntil(context.done, m_currentInvocation->shortTimeout());
     2747    runUntil(context.done, noTimeout);
    27642748    m_currentInvocation->didClearStatisticsThroughWebsiteDataRemoval();
    27652749}
    … …  
    27702754    ResourceStatisticsCallbackContext context(*this);
    27712755    WKWebsiteDataStoreStatisticsClearInMemoryAndPersistentStoreModifiedSinceHours(dataStore, hours, &context, resourceStatisticsVoidResultCallback);
    2772     if (!context.done)
    2773         runUntil(context.done, m_currentInvocation->shortTimeout());
     2756    runUntil(context.done, noTimeout);
    27742757    m_currentInvocation->didClearStatisticsThroughWebsiteDataRemoval();
    27752758}
    … …  
    27802763    ResourceStatisticsCallbackContext context(*this);
    27812764    WKWebsiteDataStoreStatisticsClearThroughWebsiteDataRemoval(dataStore, &context, resourceStatisticsVoidResultCallback);
    2782     if (!context.done)
    2783         runUntil(context.done, m_currentInvocation->shortTimeout());
     2765    runUntil(context.done, noTimeout);
    27842766    m_currentInvocation->didClearStatisticsThroughWebsiteDataRemoval();
    27852767}

  • trunk/Tools/WebKitTestRunner/TestInvocation.cpp

    r228587 r228983  
    110110    // for each test individually.
    111111    // But there shouldn't be any observable negative consequences from this.
    112     return m_timeout / 1000. / 2;
     112    return m_timeout / 1000. / 4;
    113113}
    114114
    … …  
    168168    bool shouldOpenExternalURLs = false;
    169169
    170     TestController::singleton().runUntil(m_gotInitialResponse, shortTimeout());
    171     if (!m_gotInitialResponse) {
    172         m_errorMessage = "Timed out waiting for initial response from web process\n";
    173         m_webProcessIsUnresponsive = true;
    174         goto end;
    175     }
     170    TestController::singleton().runUntil(m_gotInitialResponse, TestController::noTimeout);
    176171    if (m_error)
    177172        goto end;
    … …  
    191186#endif // !PLATFORM(IOS)
    192187
    193     if (m_webProcessIsUnresponsive)
    194         dumpWebProcessUnresponsiveness();
    195     else if (TestController::singleton().resetStateToConsistentValues(m_options))
     188    if (TestController::singleton().resetStateToConsistentValues(m_options))
    196189        return;
    197190
    … …  
    200193    // Make sure that we have a process, as invoke() will need one to send bundle messages for the next test.
    201194    TestController::singleton().reattachPageToWebProcess();
    202 }
    203 
    204 void TestInvocation::dumpWebProcessUnresponsiveness()
    205 {
    206     dumpWebProcessUnresponsiveness(m_errorMessage.c_str());
    207195}
    208196
    … …  
    272260            m_gotRepaint = false;
    273261            WKPageForceRepaint(TestController::singleton().mainWebView()->page(), this, TestInvocation::forceRepaintDoneCallback);
    274             TestController::singleton().runUntil(m_gotRepaint, shortTimeout());
    275             if (!m_gotRepaint) {
    276                 m_errorMessage = "Timed out waiting for pre-pixel dump repaint\n";
    277                 m_webProcessIsUnresponsive = true;
    278                 return;
    279             }
    280 
     262            TestController::singleton().runUntil(m_gotRepaint, TestController::noTimeout);
    281263            dumpPixelsAndCompareWithExpected(SnapshotResultType::WebView, m_repaintRects.get());
    282264        }
    … …  
    326308        m_gotFinalMessage = true;
    327309        m_error = true;
    328         m_errorMessage = "FAIL\n";
    329310        TestController::singleton().notifyDone();
    330311        return;

  • trunk/Tools/WebKitTestRunner/TestInvocation.h

    r228109 r228983  
    6161    WKRetainPtr<WKTypeRef> didReceiveSynchronousMessageFromInjectedBundle(WKStringRef messageName, WKTypeRef messageBody);
    6262
    63     void dumpWebProcessUnresponsiveness();
    6463    static void dumpWebProcessUnresponsiveness(const char* errorMessage);
    6564    void outputText(const WTF::String&);
    … …  
    120119    bool m_dumpPixels { false };
    121120    bool m_pixelResultIsPending { false };
    122     bool m_webProcessIsUnresponsive { false };
    123121
    124122    StringBuilder m_textOutput;
    … …  
    126124    WKRetainPtr<WKImageRef> m_pixelResult;
    127125    WKRetainPtr<WKArrayRef> m_repaintRects;
    128     std::string m_errorMessage;
    129126   
    130127    std::unique_ptr<UIScriptContext> m_UIScriptContext;

  • trunk/Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm

    r228304 r228983  
    196196        doneRemoving = true;
    197197    }];
    198     platformRunUntil(doneRemoving, 0);
     198    platformRunUntil(doneRemoving, noTimeout);
    199199    [[_WKUserContentExtensionStore defaultStore] _removeAllContentExtensions];
    200200

  • trunk/Tools/WebKitTestRunner/ios/TestControllerIOS.mm

    r222890 r228983  
    120120        }];
    121121
    122         platformRunUntil(doneResizing, 10);
    123         if (!doneResizing)
    124             WTFLogAlways("Timed out waiting for view resize to complete in platformConfigureViewForTest()");
     122        platformRunUntil(doneResizing, noTimeout);
    125123    }
    126124   

  • trunk/Tools/WebKitTestRunner/mac/TestControllerMac.mm

    r226751 r228983  
    150150        doneCompiling = true;
    151151    }];
    152     platformRunUntil(doneCompiling, 0);
     152    platformRunUntil(doneCompiling, noTimeout);
    153153#endif
    154154}
Note: See TracChangeset for help on using the changeset viewer.