Bug 189213 – Add infrastructure to dump resource load statistics

• Component: New Bugs
• Status: RESOLVED FIXED

[Woodrow Wang]

Added dumping function for testing resource load statistics

[Woodrow Wang]

Created attachment 348679 [details]
Patch

[Woodrow Wang]

Created attachment 348683 [details]
Patch

[Woodrow Wang]

Created attachment 348690 [details]
Patch

[Daniel Bates]

Comment on attachment 348690 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Added dumping function for testing resource load statistics

Maybe a better title would be: Add infrastructure to dump resource load statistics

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:155
> +    store->dumpResourceLoadStatistics([context, callback] (String dumpResourceLoadStatistics) {

This is ref'ing the passed string and this function does not need to take ownership of the string. I suggest the callback take a "const String&" as the parameter type. Can we come up with a better name for the parameter? Maybe resourceLoadStatistics?

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:156
> +        callback(WKStringCreateWithUTF8CString(dumpResourceLoadStatistics.utf8().data()), context);

Is there a better way to convert from WTF::String to WKStringRef. I am surprised that this problem has not already been solved. Does it make sense to to use toCopiedAPI()?

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:678
> +    

Please remove the leading whitespace from this line.

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:679
> +    StringBuilder dumpResourceLoadStatisticsMapString;

Can we come up with a better name for this variable? Maybe result?

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:500
> +    

Please remove the leading whitespace from this line.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:502
> +        String dumpResourceLoadStatistics = m_memoryStore ? m_memoryStore->dumpResourceLoadStatistics() : emptyString();

Can we come up with a better name for name of the local variable? Maybe result? or resourceLoadStatistics?

> Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:271
> +    void setDumpResourceLoadStatistics(boolean shouldDump);

I suggest that we take a similar approach as the other dumping function is this file and call this function dumpResourceLoadStatistics() and it should not take any arguments. We should also group this declaration with the declarations of the other dumping functions at the top of this file.

> Tools/WebKitTestRunner/TestController.cpp:2808
> +    WKStringRef dump;

Can we come up with a more descriptive name for this?

[Woodrow Wang]

(In reply to Daniel Bates from comment #4)
> Comment on attachment 348690 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=348690&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Added dumping function for testing resource load statistics
> 
> Maybe a better title would be: Add infrastructure to dump resource load
> statistics

Agreed, changing accordingly. 
> 
> > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:155
> > +    store->dumpResourceLoadStatistics([context, callback] (String dumpResourceLoadStatistics) {
> 
> This is ref'ing the passed string and this function does not need to take
> ownership of the string. I suggest the callback take a "const String&" as
> the parameter type. Can we come up with a better name for the parameter?
> Maybe resourceLoadStatistics?

Fixed.
> 
> > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:156
> > +        callback(WKStringCreateWithUTF8CString(dumpResourceLoadStatistics.utf8().data()), context);
> 
> Is there a better way to convert from WTF::String to WKStringRef. I am
> surprised that this problem has not already been solved. Does it make sense
> to to use toCopiedAPI()?

Yes, toCopiedAPI() is a utility function we can use. 
> 
> > Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:678
> > +    
> 
> Please remove the leading whitespace from this line.

Done. 
> 
> > Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:679
> > +    StringBuilder dumpResourceLoadStatisticsMapString;
> 
> Can we come up with a better name for this variable? Maybe result?

Changed. 
> 
> > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:500
> > +    
> 
> Please remove the leading whitespace from this line.

Done. 
> 
> > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:502
> > +        String dumpResourceLoadStatistics = m_memoryStore ? m_memoryStore->dumpResourceLoadStatistics() : emptyString();
> 
> Can we come up with a better name for name of the local variable? Maybe
> result? or resourceLoadStatistics?

Changed to resourceLoadStatistics. 
> 
> > Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:271
> > +    void setDumpResourceLoadStatistics(boolean shouldDump);
> 
> I suggest that we take a similar approach as the other dumping function is
> this file and call this function dumpResourceLoadStatistics() and it should
> not take any arguments. We should also group this declaration with the
> declarations of the other dumping functions at the top of this file.
> 
Done. 
> > Tools/WebKitTestRunner/TestController.cpp:2808
> > +    WKStringRef dump;
> 
> Can we come up with a more descriptive name for this?

Changed to resourceLoadStatisticsRepresentation.

[Woodrow Wang]

Created attachment 348859 [details]
Patch

[Chris Dumez]

Comment on attachment 348859 [details]
Patch

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

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:151
> +        callback(WKStringCreateWithUTF8CString(""), context);

It seems odd to me that we're transferring ownership of the WKString to the callback here. I think we usually do not transfer ownership of such string to callback functions.

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:156
> +        callback(WebKit::toCopiedAPI(resourceLoadStatistics), context);

Ditto. I would have used:
toAPI(resourceLoadStatistics.impl())

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:497
> +void WebResourceLoadStatisticsStore::dumpResourceLoadStatistics(CompletionHandler<void(String)>&& completionHandler)

Why String and not const String& ?

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:503
> +        postTaskReply([result, completionHandler = WTFMove(completionHandler)] () mutable {

you are failing to isolate copy result before passing it to the other thread here. You want "result = result.isolatedCopy()".

> Tools/WebKitTestRunner/TestInvocation.cpp:1363
> +            m_savedResourceLoadStatistics = toWTFString(TestController::singleton().dumpResourceLoadStatistics());

It looks to me that the value returned by dumpResourceLoadStatistics() was leaked here.

[Daniel Bates]

Comment on attachment 348859 [details]
Patch

r- per comment #7.

[Woodrow Wang]

(In reply to Chris Dumez from comment #7)
> Comment on attachment 348859 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=348859&action=review

Thanks for the tips/help!
> 
> > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:151
> > +        callback(WKStringCreateWithUTF8CString(""), context);
> 
> It seems odd to me that we're transferring ownership of the WKString to the
> callback here. I think we usually do not transfer ownership of such string
> to callback functions.
> 
> > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:156
> > +        callback(WebKit::toCopiedAPI(resourceLoadStatistics), context);
> 
> Ditto. I would have used:
> toAPI(resourceLoadStatistics.impl())

Fixed to use toAPI instead of toCopiedAPI
> 
> > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:497
> > +void WebResourceLoadStatisticsStore::dumpResourceLoadStatistics(CompletionHandler<void(String)>&& completionHandler)
> 
> Why String and not const String& ?

Changed to use const String&
> 
> > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:503
> > +        postTaskReply([result, completionHandler = WTFMove(completionHandler)] () mutable {
> 
> you are failing to isolate copy result before passing it to the other thread
> here. You want "result = result.isolatedCopy()".

Thanks for pointing this out!
> 
> > Tools/WebKitTestRunner/TestInvocation.cpp:1363
> > +            m_savedResourceLoadStatistics = toWTFString(TestController::singleton().dumpResourceLoadStatistics());
> 
> It looks to me that the value returned by dumpResourceLoadStatistics() was
> leaked here.

[Woodrow Wang]

Created attachment 348864 [details]
Patch

[Woodrow Wang]

Created attachment 348871 [details]
Patch

[Chris Dumez]

Comment on attachment 348864 [details]
Patch

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

> Tools/WebKitTestRunner/TestController.cpp:2808
> +    WKStringRef resourceLoadStatisticsRepresentation;

This should probably be a WKRetainPtr<WKStringRef> so that your string stays alive.

> Tools/WebKitTestRunner/TestController.cpp:2879
> +WKStringRef TestController::dumpResourceLoadStatistics()

Maybe this could this return a String instead of a WKStringRef ?

> Tools/WebKitTestRunner/TestController.cpp:2885
> +    return context.resourceLoadStatisticsRepresentation;

return context.resourceLoadStatisticsRepresentation.get();

if you make the change I suggest above.

> Tools/WebKitTestRunner/TestInvocation.cpp:1363
> +            m_savedResourceLoadStatistics = toWTFString(TestController::singleton().dumpResourceLoadStatistics());

Is this really the right place to dump the statistics? Seems unrelated to resetting state?

[Woodrow Wang]

(In reply to Chris Dumez from comment #12)
> Comment on attachment 348864 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=348864&action=review
> 
> > Tools/WebKitTestRunner/TestController.cpp:2808
> > +    WKStringRef resourceLoadStatisticsRepresentation;
> 
> This should probably be a WKRetainPtr<WKStringRef> so that your string stays
> alive.

Fixed!
> 
> > Tools/WebKitTestRunner/TestController.cpp:2879
> > +WKStringRef TestController::dumpResourceLoadStatistics()
> 
> Maybe this could this return a String instead of a WKStringRef ?

Agreed that it seems cleaner this way. Modified here and call sites accordingly. 
> 
> > Tools/WebKitTestRunner/TestController.cpp:2885
> > +    return context.resourceLoadStatisticsRepresentation;
> 
> return context.resourceLoadStatisticsRepresentation.get();
> 
> if you make the change I suggest above.
> 
> > Tools/WebKitTestRunner/TestInvocation.cpp:1363
> > +            m_savedResourceLoadStatistics = toWTFString(TestController::singleton().dumpResourceLoadStatistics());
> 
> Is this really the right place to dump the statistics? Seems unrelated to
> resetting state?

The design of this dumping function only allows and intends for the resource load statistics map to be dumped once at the conclusion of a test. At that point in the test, however, if the test calls resetStatisticsToConsistentState, the resource load statistics map will be cleared, and so the dumped string will be empty. Thus, right now we can simply record the state of the resource load statistics map at the time of resetting the statistics and store it in the instance variable m_savedResourceLoadStatistics.

[Woodrow Wang]

(In reply to Chris Dumez from comment #12)
> Comment on attachment 348864 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=348864&action=review
> 
> > Tools/WebKitTestRunner/TestController.cpp:2808
> > +    WKStringRef resourceLoadStatisticsRepresentation;
> 
> This should probably be a WKRetainPtr<WKStringRef> so that your string stays
> alive.
> 
> > Tools/WebKitTestRunner/TestController.cpp:2879
> > +WKStringRef TestController::dumpResourceLoadStatistics()
> 
> Maybe this could this return a String instead of a WKStringRef ?

Is there a reason we should return a String over a WKStringRef? It seems like in the existing code that the precedence is to use WKStringRefs in the TestController.
> 
> > Tools/WebKitTestRunner/TestController.cpp:2885
> > +    return context.resourceLoadStatisticsRepresentation;
> 
> return context.resourceLoadStatisticsRepresentation.get();
> 
> if you make the change I suggest above.
> 
> > Tools/WebKitTestRunner/TestInvocation.cpp:1363
> > +            m_savedResourceLoadStatistics = toWTFString(TestController::singleton().dumpResourceLoadStatistics());
> 
> Is this really the right place to dump the statistics? Seems unrelated to
> resetting state?

[Woodrow Wang]

Created attachment 348877 [details]
Patch

[Chris Dumez]

(In reply to Woodrow Wang from comment #14)
> (In reply to Chris Dumez from comment #12)
> > Comment on attachment 348864 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=348864&action=review
> > 
> > > Tools/WebKitTestRunner/TestController.cpp:2808
> > > +    WKStringRef resourceLoadStatisticsRepresentation;
> > 
> > This should probably be a WKRetainPtr<WKStringRef> so that your string stays
> > alive.
> > 
> > > Tools/WebKitTestRunner/TestController.cpp:2879
> > > +WKStringRef TestController::dumpResourceLoadStatistics()
> > 
> > Maybe this could this return a String instead of a WKStringRef ?
> 
> Is there a reason we should return a String over a WKStringRef? It seems
> like in the existing code that the precedence is to use WKStringRefs in the
> TestController.

I do not feel strongly but it seems your call site seems to want a String anyway. Also a String is much nicer to deal with than a WKStringRef because you do not have to worry about lifetime.

> > 
> > > Tools/WebKitTestRunner/TestController.cpp:2885
> > > +    return context.resourceLoadStatisticsRepresentation;
> > 
> > return context.resourceLoadStatisticsRepresentation.get();
> > 
> > if you make the change I suggest above.
> > 
> > > Tools/WebKitTestRunner/TestInvocation.cpp:1363
> > > +            m_savedResourceLoadStatistics = toWTFString(TestController::singleton().dumpResourceLoadStatistics());
> > 
> > Is this really the right place to dump the statistics? Seems unrelated to
> > resetting state?

[EWS Watchlist]

Comment on attachment 348877 [details]
Patch

Attachment 348877 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9096872

New failing tests:
fast/workers/worker-exception-during-navigation.html

[EWS Watchlist]

Created attachment 348895 [details]
Archive of layout-test-results from ews203 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews203  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Chris Dumez]

Comment on attachment 348877 [details]
Patch

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

LGTM with a couple of comments.

> Tools/WebKitTestRunner/TestController.cpp:2879
> +WKStringRef TestController::dumpResourceLoadStatistics()

nit: I think it would look nicer and be less error-prone in terms of lifetime management if this returned a String. Both your call sites need a String anyway.

> Tools/WebKitTestRunner/TestInvocation.cpp:254
> +        m_textOutput.append(m_savedResourceLoadStatistics.isEmpty() ? toWTFString(TestController::singleton().dumpResourceLoadStatistics()) : m_savedResourceLoadStatistics);

AFAICT, the empty string is a valid value that can be returned by dumpResourceLoadStatistics(). Therefore, I think the check for the ternary should be m_savedResourceLoadStatistics.isNull().

> Tools/WebKitTestRunner/TestInvocation.cpp:1361
>      if (WKStringIsEqualToUTF8CString(messageName, "StatisticsResetToConsistentState")) {

FYI, longer term, I think we should probably drop this TestRunner method. We should not rely on the test to go back to a consistent state, TestController should reset to a consistent state between tests by itself.

[Woodrow Wang]

(In reply to Chris Dumez from comment #19)
> Comment on attachment 348877 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=348877&action=review
> 
> LGTM with a couple of comments.
> 
> > Tools/WebKitTestRunner/TestController.cpp:2879
> > +WKStringRef TestController::dumpResourceLoadStatistics()
> 
> nit: I think it would look nicer and be less error-prone in terms of
> lifetime management if this returned a String. Both your call sites need a
> String anyway.

Thanks for the help with avoiding memory leaks with the WKStringRef!
> 
> > Tools/WebKitTestRunner/TestInvocation.cpp:254
> > +        m_textOutput.append(m_savedResourceLoadStatistics.isEmpty() ? toWTFString(TestController::singleton().dumpResourceLoadStatistics()) : m_savedResourceLoadStatistics);
> 
> AFAICT, the empty string is a valid value that can be returned by
> dumpResourceLoadStatistics(). Therefore, I think the check for the ternary
> should be m_savedResourceLoadStatistics.isNull().

This makes sense, so I'll change it. 
> 
> > Tools/WebKitTestRunner/TestInvocation.cpp:1361
> >      if (WKStringIsEqualToUTF8CString(messageName, "StatisticsResetToConsistentState")) {
> 
> FYI, longer term, I think we should probably drop this TestRunner method. We
> should not rely on the test to go back to a consistent state, TestController
> should reset to a consistent state between tests by itself.

Agreed, this call to dumpResourceLoadStatistics() is replicated in the dumpResults() function, as the latter call site is much more logical. As an artifact of the resource load statistics' state being cleared by this function, we have this slightly more complex logic to save that state for dumping at the end of a test. Longer term, we can let the TestController handle the resetting of state by itself, making this code slightly simpler.

[Woodrow Wang]

Created attachment 348930 [details]
Patch

[Chris Dumez]

Comment on attachment 348930 [details]
Patch

LGTM

[Chris Dumez]

(In reply to Chris Dumez from comment #22)
> Comment on attachment 348930 [details]
> Patch
> 
> LGTM

r=me

[EWS Watchlist]

Comment on attachment 348930 [details]
Patch

Attachment 348930 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9103565

New failing tests:
accessibility/smart-invert-reference.html

[EWS Watchlist]

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

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

[Woodrow Wang]

Created attachment 348949 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 348949 [details]
Patch

Clearing flags on attachment: 348949

Committed r235690: <https://trac.webkit.org/changeset/235690>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44154196>