Bug 189684 - Clear persistent storage between tests for resourceLoadStatistics
Summary: Clear persistent storage between tests for resourceLoadStatistics
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Woodrow Wang
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-09-17 17:06 PDT by Woodrow Wang
Modified: 2018-10-01 12:26 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.73 KB, patch)
2018-09-17 17:20 PDT, Woodrow Wang
no flags Details | Formatted Diff | Diff
Patch (4.77 KB, patch)
2018-09-17 18:18 PDT, Woodrow Wang
cdumez: review+
cdumez: commit-queue-
Details | Formatted Diff | Diff
Patch (4.89 KB, patch)
2018-09-18 10:30 PDT, Woodrow Wang
no flags Details | Formatted Diff | Diff
Patch (5.06 KB, patch)
2018-09-18 14:34 PDT, Woodrow Wang
no flags Details | Formatted Diff | Diff
Patch (5.41 KB, patch)
2018-09-18 17:39 PDT, Woodrow Wang
cdumez: commit-queue-
Details | Formatted Diff | Diff
Patch (5.10 KB, patch)
2018-09-19 10:18 PDT, Woodrow Wang
no flags Details | Formatted Diff | Diff
Patch (5.24 KB, patch)
2018-09-19 13:27 PDT, Woodrow Wang
no flags Details | Formatted Diff | Diff
Patch (5.26 KB, patch)
2018-09-19 15:08 PDT, Woodrow Wang
no flags Details | Formatted Diff | Diff
Patch (8.52 KB, patch)
2018-09-20 16:43 PDT, Woodrow Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Woodrow Wang 2018-09-17 17:06:54 PDT
Currently, in WKWebsiteDataStoreStatisticsResetToConsistentState, only the resource load statistics' maps in the UI process and web content processes are cleared, but the persistent storage is not. This patch deletes the plist on disk between tests when resetting to a consistent state.
Comment 1 Radar WebKit Bug Importer 2018-09-17 17:07:38 PDT
<rdar://problem/44539993>
Comment 2 Woodrow Wang 2018-09-17 17:08:37 PDT
This is a speculative fix for the flakiness seen here: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2FwebAPIStatistics%2Fnavigator-functions-accessed-data-collection.html%20http%2Ftests%2FwebAPIStatistics%2Fcanvas-read-and-write-data-collection.html%20http%2Ftests%2FwebAPIStatistics%2Ffont-load-data-collection.html%20http%2Ftests%2FwebAPIStatistics%2Fscreen-functions-accessed-data-collection.html 

Some of the data from previous tests (canvas data collection) leaks into subsequent tests (font loading data collection). This patch deletes the plist between tests, hopefully to rule out that as a source of flakiness.
Comment 3 Radar WebKit Bug Importer 2018-09-17 17:08:56 PDT
<rdar://problem/44540099>
Comment 4 Woodrow Wang 2018-09-17 17:20:32 PDT
Created attachment 349977 [details]
Patch
Comment 5 John Wilander 2018-09-17 17:26:55 PDT
Comment on attachment 349977 [details]
Patch

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

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:778
> +            m_memoryStore->clear([callbackAggregator = callbackAggregator.copyRef()] { });

You need to clear the memory store before you grandfather, otherwise you'll clear the grandfathered entries.

Further, you should grandfather in the completion handler of m_memoryStore->clear() to not run the risk of a race. This will remove the need for the callback aggregator.
Comment 6 Woodrow Wang 2018-09-17 17:40:13 PDT
(In reply to John Wilander from comment #5)
> Comment on attachment 349977 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349977&action=review
> 
> > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:778
> > +            m_memoryStore->clear([callbackAggregator = callbackAggregator.copyRef()] { });
> 
> You need to clear the memory store before you grandfather, otherwise you'll
> clear the grandfathered entries.
I'll fix that. 
> 
> Further, you should grandfather in the completion handler of
> m_memoryStore->clear() to not run the risk of a race. This will remove the
> need for the callback aggregator.
Great point, I'll update this shortly!
Comment 7 Woodrow Wang 2018-09-17 18:18:00 PDT
Created attachment 349986 [details]
Patch
Comment 8 Chris Dumez 2018-09-18 08:54:29 PDT
Comment on attachment 349986 [details]
Patch

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

r=me with fixes.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:775
> +            m_memoryStore->clear([this, shouldGrandfather, callCompletionHandlerOnMainThread = WTFMove(callCompletionHandlerOnMainThread)] () mutable {

this, protectedThis = makeRef(*this)

I think you need to protect this here since there is nothing that guarantees that |this| stays alive AFAICT.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:777
> +                    m_memoryStore->grandfatherExistingWebsiteData(WTFMove(callCompletionHandlerOnMainThread));

You likely need to null-check m_memoryStore here. It would have been cleared in between.
Comment 9 John Wilander 2018-09-18 10:00:45 PDT
(In reply to Chris Dumez from comment #8)
> Comment on attachment 349986 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349986&action=review
> 
> r=me with fixes.
> 
> > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:775
> > +            m_memoryStore->clear([this, shouldGrandfather, callCompletionHandlerOnMainThread = WTFMove(callCompletionHandlerOnMainThread)] () mutable {
> 
> this, protectedThis = makeRef(*this)
> 
> I think you need to protect this here since there is nothing that guarantees
> that |this| stays alive AFAICT.
> 
> > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:777
> > +                    m_memoryStore->grandfatherExistingWebsiteData(WTFMove(callCompletionHandlerOnMainThread));
> 
> You likely need to null-check m_memoryStore here. It would have been cleared
> in between.

Could you also ASSERT m_memoryStore, please? I'd like to know if we're failing to grandfather website data.
Comment 10 Woodrow Wang 2018-09-18 10:07:56 PDT
(In reply to Chris Dumez from comment #8)
> Comment on attachment 349986 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349986&action=review
> 
> r=me with fixes.
> 
> > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:775
> > +            m_memoryStore->clear([this, shouldGrandfather, callCompletionHandlerOnMainThread = WTFMove(callCompletionHandlerOnMainThread)] () mutable {
> 
> this, protectedThis = makeRef(*this)
> 
> I think you need to protect this here since there is nothing that guarantees
> that |this| stays alive AFAICT.
I'll add this. 
> 
> > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:777
> > +                    m_memoryStore->grandfatherExistingWebsiteData(WTFMove(callCompletionHandlerOnMainThread));
> 
> You likely need to null-check m_memoryStore here. It would have been cleared
> in between.
Good call, I'll also ASSERT m_memoryStore so we will know if grandfathering has happened as expected.
Comment 11 Woodrow Wang 2018-09-18 10:30:07 PDT
Created attachment 350032 [details]
Patch
Comment 12 Chris Dumez 2018-09-18 13:13:17 PDT
Comment on attachment 350032 [details]
Patch

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

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:777
> +                    ASSERT(m_memoryStore);

I do not believe this assertion is correct. There is no such guarantee. If applicationWillTerminate() gets called, we'll destroy m_memoryStore.
Comment 13 John Wilander 2018-09-18 13:17:30 PDT
(In reply to Chris Dumez from comment #12)
> Comment on attachment 350032 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350032&action=review
> 
> > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:777
> > +                    ASSERT(m_memoryStore);
> 
> I do not believe this assertion is correct. There is no such guarantee. If
> applicationWillTerminate() gets called, we'll destroy m_memoryStore.

Are you saying we'll hit this assertion in the TestRunner on debug bots? How would you rather check that we don't have regular cases where the memory store is null when we're supposed to grandfather?
Comment 14 Chris Dumez 2018-09-18 13:22:42 PDT
(In reply to John Wilander from comment #13)
> (In reply to Chris Dumez from comment #12)
> > Comment on attachment 350032 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=350032&action=review
> > 
> > > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:777
> > > +                    ASSERT(m_memoryStore);
> > 
> > I do not believe this assertion is correct. There is no such guarantee. If
> > applicationWillTerminate() gets called, we'll destroy m_memoryStore.
> 
> Are you saying we'll hit this assertion in the TestRunner on debug bots? How
> would you rather check that we don't have regular cases where the memory
> store is null when we're supposed to grandfather?

Probably not on the bots but people will be able to hit it on their device is some code calls scheduleClearInMemoryAndPersistent(ShouldGrandfather::Yes) and then exit the application quickly.
Comment 15 John Wilander 2018-09-18 13:24:35 PDT
(In reply to Chris Dumez from comment #14)
> (In reply to John Wilander from comment #13)
> > (In reply to Chris Dumez from comment #12)
> > > Comment on attachment 350032 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=350032&action=review
> > > 
> > > > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:777
> > > > +                    ASSERT(m_memoryStore);
> > > 
> > > I do not believe this assertion is correct. There is no such guarantee. If
> > > applicationWillTerminate() gets called, we'll destroy m_memoryStore.
> > 
> > Are you saying we'll hit this assertion in the TestRunner on debug bots? How
> > would you rather check that we don't have regular cases where the memory
> > store is null when we're supposed to grandfather?
> 
> Probably not on the bots but people will be able to hit it on their device
> is some code calls
> scheduleClearInMemoryAndPersistent(ShouldGrandfather::Yes) and then exit the
> application quickly.

Are people using debug builds that way? I've never come across that we take application quit into consideration when we add a correctness debug ASSERT.
Comment 16 Chris Dumez 2018-09-18 13:25:19 PDT
Comment on attachment 350032 [details]
Patch

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

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:-793
> -        if (shouldGrandfather == ShouldGrandfather::Yes && m_memoryStore)

Also note that the previous code was doing a null check as expected.

>>>> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:777
>>>> +                    ASSERT(m_memoryStore);
>>> 
>>> I do not believe this assertion is correct. There is no such guarantee. If applicationWillTerminate() gets called, we'll destroy m_memoryStore.
>> 
>> Are you saying we'll hit this assertion in the TestRunner on debug bots? How would you rather check that we don't have regular cases where the memory store is null when we're supposed to grandfather?
> 
> Probably not on the bots but people will be able to hit it on their device is some code calls scheduleClearInMemoryAndPersistent(ShouldGrandfather::Yes) and then exit the application quickly.

Also note that this assertion would anyway not cover the case where m_memoryStore is null before we call clear...

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:781
> +                    callCompletionHandlerOnMainThread();

i.e. this code path.
Comment 17 Chris Dumez 2018-09-18 13:30:05 PDT
(In reply to John Wilander from comment #15)
> (In reply to Chris Dumez from comment #14)
> > (In reply to John Wilander from comment #13)
> > > (In reply to Chris Dumez from comment #12)
> > > > Comment on attachment 350032 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=350032&action=review
> > > > 
> > > > > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:777
> > > > > +                    ASSERT(m_memoryStore);
> > > > 
> > > > I do not believe this assertion is correct. There is no such guarantee. If
> > > > applicationWillTerminate() gets called, we'll destroy m_memoryStore.
> > > 
> > > Are you saying we'll hit this assertion in the TestRunner on debug bots? How
> > > would you rather check that we don't have regular cases where the memory
> > > store is null when we're supposed to grandfather?
> > 
> > Probably not on the bots but people will be able to hit it on their device
> > is some code calls
> > scheduleClearInMemoryAndPersistent(ShouldGrandfather::Yes) and then exit the
> > application quickly.
> 
> Are people using debug builds that way? I've never come across that we take
> application quit into consideration when we add a correctness debug ASSERT.

We have to worry about application termination here because:
void WebResourceLoadStatisticsStore::applicationWillTerminate()
{
    flushAndDestroyPersistentStore();
}

We do a best effort at flushing things to disk before exiting the app, and then we destroy the objects.

It is not OK to add an assertion that can hit in valid use cases, even if those cases are rare.
Comment 18 Woodrow Wang 2018-09-18 14:34:53 PDT
Created attachment 350059 [details]
Patch
Comment 19 Chris Dumez 2018-09-18 14:38:21 PDT
Comment on attachment 350059 [details]
Patch

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

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:780
> +                        RELEASE_LOG(ResourceLoadStatistics, "WebResourceLoadStatisticsStore::scheduleClearInMemoryAndPersistent m_memoryStore is null when trying to grandfather data.");

Why log here but not..

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:786
>              callCompletionHandlerOnMainThread();

.. here?
Comment 20 Woodrow Wang 2018-09-18 16:19:16 PDT
(In reply to Chris Dumez from comment #19)
> Comment on attachment 350059 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350059&action=review
> 
> > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:780
> > +                        RELEASE_LOG(ResourceLoadStatistics, "WebResourceLoadStatisticsStore::scheduleClearInMemoryAndPersistent m_memoryStore is null when trying to grandfather data.");
> 
> Why log here but not..
> 
> > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:786
> >              callCompletionHandlerOnMainThread();
> 
> .. here?

To my understanding, the existing log is useful as we expect when grandfathering is requested that the memory store is non-null. The latter site is where we have no memory store to clear at all in the function, which seems like possible behavior. If this is unexpected, what should the log statement be?
Comment 21 Chris Dumez 2018-09-18 16:33:19 PDT
Comment on attachment 350059 [details]
Patch

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

>>> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:780
>>> +                        RELEASE_LOG(ResourceLoadStatistics, "WebResourceLoadStatisticsStore::scheduleClearInMemoryAndPersistent m_memoryStore is null when trying to grandfather data.");
>> 
>> Why log here but not..
> 
> To my understanding, the existing log is useful as we expect when grandfathering is requested that the memory store is non-null. The latter site is where we have no memory store to clear at all in the function, which seems like possible behavior. If this is unexpected, what should the log statement be?

You could do the exact same login gin the else case below if (shouldGrandfather == ShouldGrandfather::Yes). There is no difference really. It is just a matter a timing: was the memory store destroyed before or after the call to clear() ?
Comment 22 Woodrow Wang 2018-09-18 17:13:01 PDT
(In reply to Chris Dumez from comment #21)
> Comment on attachment 350059 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350059&action=review
> 
> >>> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:780
> >>> +                        RELEASE_LOG(ResourceLoadStatistics, "WebResourceLoadStatisticsStore::scheduleClearInMemoryAndPersistent m_memoryStore is null when trying to grandfather data.");
> >> 
> >> Why log here but not..
> > 
> > To my understanding, the existing log is useful as we expect when grandfathering is requested that the memory store is non-null. The latter site is where we have no memory store to clear at all in the function, which seems like possible behavior. If this is unexpected, what should the log statement be?
> 
> You could do the exact same login gin the else case below if
> (shouldGrandfather == ShouldGrandfather::Yes). There is no difference
> really. It is just a matter a timing: was the memory store destroyed before
> or after the call to clear() ?
I see, I can add the extra logging statement with slightly different wording to differentiate the two cases.
Comment 23 Woodrow Wang 2018-09-18 17:39:13 PDT
Created attachment 350082 [details]
Patch
Comment 24 Chris Dumez 2018-09-19 09:10:00 PDT
Comment on attachment 350082 [details]
Patch

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

Please see if you can refactor this code a bit so that it is not as big and not as error prone. You may also consider using CompletionHandlerCallingScope to make sure the completion handler is always called.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:780
> +                        RELEASE_LOG(ResourceLoadStatistics, "WebResourceLoadStatisticsStore::scheduleClearInMemoryAndPersistent After being cleared, m_memoryStore is null when trying to grandfather data.");

The completion is not called in this case, which is wrong.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:785
> +        } else

The else case is multi-line so you need curly brackets.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:788
> +            else

This is wrong, you now fail to call the completion handler in the if case. This else should be dropped so that callCompletionHandlerOnMainThread() is always called.
Comment 25 Woodrow Wang 2018-09-19 10:14:51 PDT
(In reply to Chris Dumez from comment #24)
> Comment on attachment 350082 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350082&action=review
> 
> Please see if you can refactor this code a bit so that it is not as big and
> not as error prone. You may also consider using
> CompletionHandlerCallingScope to make sure the completion handler is always
> called.
I'll refactor the code to use CompletionHandlerCallingScope to ensure the completion handler is always called. 
> 
> > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:780
> > +                        RELEASE_LOG(ResourceLoadStatistics, "WebResourceLoadStatisticsStore::scheduleClearInMemoryAndPersistent After being cleared, m_memoryStore is null when trying to grandfather data.");
> 
> The completion is not called in this case, which is wrong.
> 
> > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:785
> > +        } else
> 
> The else case is multi-line so you need curly brackets.
> 
> > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:788
> > +            else
> 
> This is wrong, you now fail to call the completion handler in the if case.
> This else should be dropped so that callCompletionHandlerOnMainThread() is
> always called.
Comment 26 Woodrow Wang 2018-09-19 10:18:22 PDT
Created attachment 350130 [details]
Patch
Comment 27 Chris Dumez 2018-09-19 12:58:39 PDT
Comment on attachment 350130 [details]
Patch

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

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:769
>          CompletionHandler<void()> callCompletionHandlerOnMainThread = [completionHandler = WTFMove(completionHandler)]() mutable {

We do not need this local variable anymore, we could just inline the lambda when constructing the CompletionHandlerCallingScope. LGTM otherwise.
Comment 28 Woodrow Wang 2018-09-19 13:27:42 PDT
Created attachment 350147 [details]
Patch
Comment 29 Chris Dumez 2018-09-19 13:41:33 PDT
Comment on attachment 350147 [details]
Patch

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

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:772
> +        CompletionHandlerCallingScope completionHandlerCaller(WTFMove(callCompletionHandlerOnMainThread));

You still did not inline the lambda in here.
Comment 30 Woodrow Wang 2018-09-19 14:26:14 PDT
(In reply to Chris Dumez from comment #29)
> Comment on attachment 350147 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350147&action=review
> 
> > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:772
> > +        CompletionHandlerCallingScope completionHandlerCaller(WTFMove(callCompletionHandlerOnMainThread));
> 
> You still did not inline the lambda in here.
Oops, I'll do so right now.
Comment 31 Woodrow Wang 2018-09-19 15:08:17 PDT
Created attachment 350151 [details]
Patch
Comment 32 Chris Dumez 2018-09-19 15:11:31 PDT
Comment on attachment 350151 [details]
Patch

LGTM
Comment 33 WebKit Commit Bot 2018-09-19 15:30:09 PDT
Comment on attachment 350151 [details]
Patch

Rejecting attachment 350151 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 350151, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebKit/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/9274508
Comment 34 WebKit Commit Bot 2018-09-19 15:57:30 PDT
Comment on attachment 350151 [details]
Patch

Clearing flags on attachment: 350151

Committed r236229: <https://trac.webkit.org/changeset/236229>
Comment 35 Dawei Fenton (:realdawei) 2018-09-19 17:04:57 PDT
(In reply to WebKit Commit Bot from comment #34)
> Comment on attachment 350151 [details]
> Patch
> 
> Clearing flags on attachment: 350151
> 
> Committed r236229: <https://trac.webkit.org/changeset/236229>

Seeing API Timeouts after on Mac WK1 after this revision

sample output:
https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK1%20(Tests)/builds/8069/steps/run-api-tests/logs/stdio
Test suite failed

Timeout

    TestWebKitAPI.ResourceLoadStatistics.GrandfatherCallback
Comment 36 Dawei Fenton (:realdawei) 2018-09-19 17:08:27 PDT
(In reply to Dawei Fenton (:realdawei) from comment #35)
> (In reply to WebKit Commit Bot from comment #34)
> > Comment on attachment 350151 [details]
> > Patch
> > 
> > Clearing flags on attachment: 350151
> > 
> > Committed r236229: <https://trac.webkit.org/changeset/236229>
> 
> Seeing API Timeouts on Mac WK1 after this revision
> 
> sample output:
> https://build.webkit.org/builders/
> Apple%20High%20Sierra%20Release%20WK1%20(Tests)/builds/8069/steps/run-api-
> tests/logs/stdio
> Test suite failed
> 
> Timeout
> 
>     TestWebKitAPI.ResourceLoadStatistics.GrandfatherCallback


Also timing out on iOS
https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Release%20WK2%20(Tests)/builds/7505/steps/run-api-tests/logs/stdio
Comment 37 Dawei Fenton (:realdawei) 2018-09-19 17:20:14 PDT
Reverted r236229 for reason:

caused API timouts on mac and ios

Committed r236233: <https://trac.webkit.org/changeset/236233>
Comment 38 Woodrow Wang 2018-09-20 16:37:55 PDT
(In reply to Dawei Fenton (:realdawei) from comment #37)
> Reverted r236229 for reason:
> 
> caused API timouts on mac and ios
> 
> Committed r236233: <https://trac.webkit.org/changeset/236233>

Submitting another patch with a fix for the timeout on the ResourceLoadStatistics::GrandfatherCallback test.
Comment 39 Woodrow Wang 2018-09-20 16:43:29 PDT
Created attachment 350279 [details]
Patch
Comment 40 Chris Dumez 2018-09-21 08:50:58 PDT
Comment on attachment 350279 [details]
Patch

r=me
Comment 41 WebKit Commit Bot 2018-09-21 09:17:26 PDT
Comment on attachment 350279 [details]
Patch

Clearing flags on attachment: 350279

Committed r236320: <https://trac.webkit.org/changeset/236320>
Comment 42 John Wilander 2018-10-01 12:26:48 PDT
Moving to resolved based on the landed patch.