Bug 175569 – Add the PerformanceServerTiming Interface which makes Server-Timing header timing values available to JavaScript running in the browser.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

Parse Server-Timing response headers and make the metric name(s), value(s), and description(s) available on the `serverTiming` property of `PerformanceNavigationTiming` and `PerformanceResourceTiming` entries on the global performance timeline and to PerformanceObservers. 

Spec found here:
https://w3c.github.io/server-timing/

Landed as "experimental feature" in Chrome:
https://www.chromestatus.com/features/5695708376072192

Created attachment 333605 [details]
Patch

[EWS Watchlist]

Attachment 333605 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 333619 [details]
Patch

Created attachment 333624 [details]
Patch

[EWS Watchlist]

Attachment 333624 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 202 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 333627 [details]
Patch

Created attachment 333657 [details]
Patch

Created attachment 333660 [details]
Patch

Created attachment 333668 [details]
Patch

Created attachment 333686 [details]
Patch

Created attachment 333687 [details]
Patch

Created attachment 333805 [details]
Patch

Created attachment 333817 [details]
Patch

Created attachment 333902 [details]
Patch

Created attachment 333943 [details]
Patch

Created attachment 333945 [details]
Patch

[EWS Watchlist]

Comment on attachment 333945 [details]
Patch

Attachment 333945 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6524626

New failing tests:
http/tests/preload/onerror_event.html

[EWS Watchlist]

Created attachment 333966 [details]
Archive of layout-test-results from ews201 for win-future

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

Created attachment 336586 [details]
Patch

Created attachment 336619 [details]
Patch

Bots are finally green! Can I get a review for this, please? :)

[Alex Christensen]

Comment on attachment 336619 [details]
Patch

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

Cool!

> Source/WebCore/loader/ParsedServerTiming.h:14
> + *     * Neither the name of Google Inc. nor the names of its

I think you have the wrong copyright header.  Google isn't mentioned in the copyright of this file.

> Source/WebCore/loader/ParsedServerTiming.h:39
> +class ParsedServerTiming {

ServerTimingParser?

> Source/WebCore/loader/ParsedServerTiming.h:41
> +    static std::unique_ptr<Vector<std::unique_ptr<ServerTiming>>> parseServerTiming(const String&);

This should be Vector<ServerTiming> or std::optional<Vector<ServerTiming>> if it can fail.

> Source/WebCore/loader/ResourceTiming.cpp:113
> +            serverTiming.append(adoptRef(new PerformanceServerTiming(entry->name(), entry->duration(), entry->description())));

adoptRef(*new ...)
Replace Vector<RefPtr<...>> with Vector<Ref<...>> when possible so we know it can't be null.

> Source/WebCore/loader/ServerTiming.cpp:49
> +std::unique_ptr<ServerTiming> ServerTiming::isolatedCopy() const
> +{
> +    return std::make_unique<ServerTiming>(*this);
> +}

This should call .isolatedCopy on each string.

> Source/WebCore/loader/ServerTiming.h:36
> +    : m_name(name) { }

indent

> Source/WebCore/loader/ServerTiming.h:38
> +    : m_name(other.m_name), m_duration(other.m_duration), m_description(other.m_description) { }

ditto

> Source/WebCore/loader/ServerTiming.h:44
> +    std::unique_ptr<ServerTiming> isolatedCopy() const;

ServerTiming.  unique_ptr unnecessary here.

> Source/WebCore/page/PerformanceResourceTiming.cpp:84
> +    resourceTiming.populateServerTiming(m_serverTiming);

m_serverTiming(resourceTiming.serverTiming());
or something like that.  Return-by-reference is confusing.  It leaves it unclear if m_serverTiming was expected to be empty before or not.

> LayoutTests/imported/w3c/web-platform-tests/server-timing/cross_origin.html:1
> +<!DOCTYPE html>

These tests need a ChangeLog and a -expected.txt file so we can see what passes.

[Alex Christensen]

Comment on attachment 336619 [details]
Patch

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

> Source/WebCore/loader/ParsedServerTiming.cpp:68
> +            entries->append(std::make_unique<ServerTiming>(entry));

This should become entries.append(WTFMove(entry));

[youenn fablet]

Comment on attachment 336619 [details]
Patch

Some additional points.

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

> Source/WebCore/loader/HTTPHeaderField.h:88
> +    static bool isCommentText(UChar);

I would keep the namespace RTFC7230 and define functions we want to expose there.
The other functions would remain declared only in the cpp file.

> Source/WebCore/loader/HeaderFieldTokenizer.cpp:2
> + * Copyright 2017 The Chromium Authors. All rights reserved.

Is this class coming from Chromium?

> Source/WebCore/loader/HeaderFieldTokenizer.h:35
> +class HeaderFieldTokenizer final {

No need for final here.

> Source/WebCore/loader/HeaderFieldTokenizer.h:38
> +    HeaderFieldTokenizer(const String& header_field);

explicit.

> Source/WebCore/loader/HeaderFieldTokenizer.h:45
> +    bool consumeToken(String& output);

Why not returning directly a String or an optional<String> if we need to disambiguate error cases?
I wonder whether it should not be returning something like optional<StringView> to reduce the memory allocation.

> Source/WebCore/loader/ResourceTiming.cpp:109
> +void ResourceTiming::populateServerTiming(Vector<RefPtr<PerformanceServerTiming>>& serverTiming)

Instead of passing an out parameter, can we directly return a Vector<Ref<>>?
For improved efficiency, you could then write it as wtf::map(*m_serverTiming) [] (auto&& item) { return adoptRef(*new ...)l };

If serverTiming is already full of entries, that might be fine. Or we could use the reserveCapacity/uncheckedAppend pattern.

> Source/WebCore/loader/ResourceTiming.cpp:122
> +            serverTiming->append(entry->isolatedCopy());

We probably have a CrossThreadCopier that will do that for you.

> Source/WebCore/loader/ResourceTiming.h:78
> +    std::unique_ptr<Vector<std::unique_ptr<ServerTiming>>> m_serverTiming;

Can we just have a Vector<ServerTiming>?

> Source/WebCore/loader/ServerTiming.cpp:37
> +        }

We could early return here.

> Source/WebCore/loader/ServerTiming.h:32
> +class ServerTiming {

How about making it a struct instead?

> Source/WebCore/page/PerformanceServerTiming.h:36
> +    PerformanceServerTiming(const String& name, double duration, const String& description);

We usually make the constructor private and add a static create method.
We also usually pass String&& as in parameters.

> Source/WebCore/page/PerformanceServerTiming.h:39
> +    String name() const { return m_name; }

can be a const String&, ditto below.

[youenn fablet]

By the way, is there any implication in leaking such header value information to third party origins? Is the plan to rely on Timing-Allow-Origin?

Created attachment 337036 [details]
Patch

I'm not quite ready for another review yet, a few more things to address.

(In reply to youenn fablet from comment #25)
> By the way, is there any implication in leaking such header value
> information to third party origins? Is the plan to rely on
> Timing-Allow-Origin?

Yes, exactly. Parsing server timing headers is gated by `m_allowTimingDetails`

Created attachment 337052 [details]
Patch

[EWS Watchlist]

Comment on attachment 337052 [details]
Patch

Attachment 337052 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7187299

New failing tests:
imported/w3c/web-platform-tests/server-timing/test_server_timing.html
imported/w3c/web-platform-tests/server-timing/cross_origin.html
imported/w3c/web-platform-tests/server-timing/server_timing_header-parsing.html

[EWS Watchlist]

Created attachment 337057 [details]
Archive of layout-test-results from ews114 for mac-sierra

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

Created attachment 337139 [details]
Patch

[youenn fablet]

Comment on attachment 337139 [details]
Patch

Looks almost ready.
Some comments below.

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

> Source/WebCore/loader/HeaderFieldTokenizer.h:46
> +    bool consumeTokenOrQuotedString(String& output);

Since these are public, it would probably be more consistent to have these returning std::optional<String> or maybe just a String and String::isNull would be the error case.

> Source/WebCore/loader/ResourceTiming.cpp:106
> +        m_serverTiming = ServerTimingParser::parseServerTiming(response.httpHeaderField(HTTPHeaderName::ServerTiming));

To be noted that we are currently tightening our security model and reducing exposure of HTTP response headers in WebProcess to the minimum.
When going from NetworkProcess to WebProcess, we will in a near future, filter response headers that are not absolutely needed (see bug 184310).
We will probably need to implement something similar to m_allowTimingDetails in NetworkProcess when sending the resource response.

> Source/WebCore/loader/ResourceTiming.cpp:113
> +        serverTiming.append(PerformanceServerTiming::create(WTFMove(entry.name), entry.duration, WTFMove(entry.description)));

You can probably use WTF::map here so that uncheckedAppend/reserveInitialCapacity are used behind the scenes.

> Source/WebCore/loader/ResourceTiming.cpp:122
> +        serverTiming.append(entry.isolatedCopy());

You can probably make use of CrossThreadCopy instead.

> Source/WebCore/loader/ResourceTiming.h:62
> +    ResourceTiming(const URL& url, const String& initiator, const LoadTiming& loadTiming, const NetworkLoadMetrics& networkLoadMetrics, bool allowTimingDetails, Vector<ServerTiming> serverTiming)

Should be &&

> Source/WebCore/loader/ResourceTiming.h:68
> +        , m_serverTiming(serverTiming)

WTFMove(serverTiming)

> Source/WebCore/loader/ServerTiming.cpp:51
> +    return ServerTiming(this->name.isolatedCopy(), this->duration, this->description.isolatedCopy());

Is "this->" needed?
You should also probably copy the other fields, something like:
ServerTiming { name.isolatedCopy(), duration, description.isolatedCopy(), durationSet, descriptionSet };

> Source/WebCore/loader/ServerTiming.h:48
> +    : name(name)

Use WTFMove here and below.

> Source/WebCore/loader/ServerTimingParser.cpp:56
> +                if (tokenizer.consume('=')) {

So we can have just a parameter name without value and this is valid, right?

> Source/WebCore/loader/ServerTimingParser.cpp:57
> +                    tokenizer.consumeTokenOrQuotedString(value);

We should probably check whether parsing was fine?

> Source/WebCore/loader/ServerTimingParser.h:34
> +class ServerTimingParser {

Do we need that class?
parseServerTiming as a free function seems fine.

> Source/WebCore/loader/WorkerThreadableLoader.h:33
> +#include "NetworkLoadMetrics.h"

Why is it needed?

> Source/WebCore/page/PerformanceResourceTiming.h:37
> +#include "PerformanceServerTiming.h"

If possible, we usually forward reference declarations instead of including header files here.
This can probably be done close to line 42.

> Source/WebCore/page/PerformanceServerTiming.h:37
> +    virtual ~PerformanceServerTiming();

It probably does not need to be virtual.
You might then need in PerformanceServerTiming.idl to add ImplementationLacksVTable keyword.

Created attachment 337383 [details]
Patch

[youenn fablet]

Comment on attachment 337383 [details]
Patch

I was going to say r+, patch is not applying though.
Some comments below.

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

> Source/WebCore/loader/ServerTimingParser.cpp:41
> +    if (!headerValue.isNull()) {

We should early return here.

> Source/WebCore/loader/ServerTimingParser.cpp:59
> +                    value = tokenizer.consumeTokenOrQuotedString();

If value is null here, is it fine skipping it/adding the entry or should we raise some kind of error?

> Source/WebCore/loader/ServerTimingParser.h:36
> +Vector<ServerTiming> parseServerTiming(const String&);

It would probably be good to add some unit tests for this method.
See Tools/TestWebKitAPI/Tests/ for examples.

> Source/WebCore/page/mac/WheelEventDeltaFilterMac.h:31
> +#include <wtf/MonotonicTime.h>

Probably not needed/unrelated?

Created attachment 337385 [details]
Patch

Created attachment 337498 [details]
Patch

Created attachment 337502 [details]
Patch

Created attachment 337505 [details]
Patch

[EWS Watchlist]

Comment on attachment 337505 [details]
Patch

Attachment 337505 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7254958

New failing tests:
imported/w3c/web-platform-tests/server-timing/server_timing_header-parsing.html
imported/w3c/web-platform-tests/server-timing/cross_origin.html

[EWS Watchlist]

Created attachment 337512 [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

[EWS Watchlist]

Comment on attachment 337505 [details]
Patch

Attachment 337505 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7254923

New failing tests:
imported/w3c/web-platform-tests/server-timing/server_timing_header-parsing.html
imported/w3c/web-platform-tests/server-timing/cross_origin.html

[EWS Watchlist]

Created attachment 337513 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

Created attachment 337545 [details]
Patch

Created attachment 337602 [details]
Patch

[EWS Watchlist]

Comment on attachment 337602 [details]
Patch

Attachment 337602 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7268089

New failing tests:
imported/w3c/web-platform-tests/workers/name-property.html

[EWS Watchlist]

Created attachment 337606 [details]
Archive of layout-test-results from ews113 for mac-sierra

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

Created attachment 337607 [details]
Patch

Created attachment 337620 [details]
Patch

Created attachment 337624 [details]
Patch

[youenn fablet]

Comment on attachment 337624 [details]
Patch

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

> Source/WebCore/loader/ResourceTiming.h:30
> +#include "PerformanceServerTiming.h"

Can you try forward-declare PerformanceServerTiming?

> Source/WebCore/loader/ResourceTiming.h:62
> +    ResourceTiming(const URL& url, const String& initiator, const LoadTiming& loadTiming, const NetworkLoadMetrics& networkLoadMetrics, bool allowTimingDetails, Vector<ServerTiming>&& serverTiming)

If you can forward declare PerformanceServerTiming, could you also check whether we can easily change this constructor to take r-values for each meaningful field (provided it is used only/mainly for  ResourceTiming::isolatedCopy).

Created attachment 337670 [details]
Patch

[youenn fablet]

Comment on attachment 337670 [details]
Patch

r=me.

As a side note, LoadTiming has an isolatedCopy method but it seems it does not have anything to isolate.
The isolatedCopy seems equivalent to a copy constructor.

[Ryosuke Niwa]

Comment on attachment 337670 [details]
Patch

r-. This feature needs to be guarded behind a runtime flag.

Created attachment 338227 [details]
Patch

[youenn fablet]

This starts to become a big patch.
I would recommend introducing the runtime flag in a separate patch.

(In reply to youenn fablet from comment #56)
> This starts to become a big patch.
> I would recommend introducing the runtime flag in a separate patch.

Ok, can/should I just delete the latest patch? Or shall I submit a new one with the changes revered?

[EWS Watchlist]

Comment on attachment 338227 [details]
Patch

Attachment 338227 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7360357

New failing tests:
imported/w3c/web-platform-tests/server-timing/server_timing_header-parsing.html
imported/w3c/web-platform-tests/server-timing/cross_origin.html

[EWS Watchlist]

Created attachment 338241 [details]
Archive of layout-test-results from ews102 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 338227 [details]
Patch

Attachment 338227 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7360324

New failing tests:
imported/w3c/web-platform-tests/server-timing/cross_origin.html
imported/w3c/web-platform-tests/server-timing/server_timing_header-parsing.html

[EWS Watchlist]

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

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

[EWS Watchlist]

Comment on attachment 338227 [details]
Patch

Attachment 338227 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7360435

New failing tests:
imported/w3c/web-platform-tests/server-timing/cross_origin.html
imported/w3c/web-platform-tests/server-timing/server_timing_header-parsing.html

[EWS Watchlist]

Created attachment 338246 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[EWS Watchlist]

Comment on attachment 338227 [details]
Patch

Attachment 338227 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7360520

New failing tests:
imported/w3c/web-platform-tests/server-timing/cross_origin.html
imported/w3c/web-platform-tests/server-timing/server_timing_header-parsing.html

[EWS Watchlist]

Created attachment 338248 [details]
Archive of layout-test-results from ews116 for mac-sierra

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

[youenn fablet]

(In reply to cvazac from comment #57)
> (In reply to youenn fablet from comment #56)
> > This starts to become a big patch.
> > I would recommend introducing the runtime flag in a separate patch.
> 
> Ok, can/should I just delete the latest patch? Or shall I submit a new one
> with the changes revered?

I would make the runtime flag patch blocking this particular bug.
When the runtime flag patch lands, it should be a small refresh to your previous patch here.

added: https://bugs.webkit.org/show_bug.cgi?id=184758

Created attachment 340213 [details]
Patch

[EWS Watchlist]

Comment on attachment 340213 [details]
Patch

Attachment 340213 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7654639

New failing tests:
imported/w3c/web-platform-tests/server-timing/server_timing_header-parsing.https.html
imported/w3c/web-platform-tests/server-timing/resource_timing_idl.https.html
imported/w3c/web-platform-tests/server-timing/navigation_timing_idl.html
imported/w3c/web-platform-tests/server-timing/test_server_timing.https.html
imported/w3c/web-platform-tests/server-timing/navigation_timing_idl.https.html

[EWS Watchlist]

Created attachment 340226 [details]
Archive of layout-test-results from ews115 for mac-sierra

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

Created attachment 340311 [details]
Patch

[youenn fablet]

Comment on attachment 340311 [details]
Patch

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

> Source/WebCore/loader/ResourceTiming.cpp:79
>      , m_allowTimingDetails(passesTimingAllowCheck(response, securityOrigin))

Do we have some tests that combine ServerTiming and Timing-Allow-Origin?

(In reply to youenn fablet from comment #72)
> Comment on attachment 340311 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340311&action=review
> 
> > Source/WebCore/loader/ResourceTiming.cpp:79
> >      , m_allowTimingDetails(passesTimingAllowCheck(response, securityOrigin))
> 
> Do we have some tests that combine ServerTiming and Timing-Allow-Origin?

Adding one here: 
https://github.com/w3c/web-platform-tests/pull/10991

Created attachment 340339 [details]
Patch

[EWS Watchlist]

Comment on attachment 340339 [details]
Patch

Attachment 340339 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7680672

New failing tests:
imported/w3c/web-platform-tests/server-timing/service_worker_idl.html

[EWS Watchlist]

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

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

[EWS Watchlist]

Comment on attachment 340339 [details]
Patch

Attachment 340339 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7680705

New failing tests:
imported/w3c/web-platform-tests/server-timing/service_worker_idl.html

[EWS Watchlist]

Created attachment 340356 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

Created attachment 340410 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 340410 [details]
Patch

Clearing flags on attachment: 340410

Committed r231813: <https://trac.webkit.org/changeset/231813>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40266028>

[Simon Fraser (smfr)]

This added ServerTimingParser.h/cpp at the top level of the WebCore Xcode project. Please fix.

[Joseph Pecoraro]

(In reply to Simon Fraser (smfr) from comment #83)
> This added ServerTimingParser.h/cpp at the top level of the WebCore Xcode
> project. Please fix.

I'll take care of this.