Add support for Server-Timing HTTP trailing header

RESOLVED FIXED in Firefox 59

Status

()

P2
normal
RESOLVED FIXED
a year ago
4 months ago

People

(Reporter: jduell.mcbugs, Assigned: kershaw, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

(Whiteboard: [necko-triaged])

Attachments

(4 attachments, 12 obsolete attachments)

13.86 KB, patch
kershaw
: review+
Details | Diff | Splinter Review
25.62 KB, patch
kershaw
: review+
Details | Diff | Splinter Review
17.12 KB, patch
kershaw
: review+
Details | Diff | Splinter Review
15.41 KB, patch
kershaw
: review+
Details | Diff | Splinter Review
(Reporter)

Description

a year ago
This bug is specifically for the necko work involved here, not the devtools (and/or JS exposure) of the Server-timing data (see bug 1403051 and bug 1337684).

  https://w3c.github.io/server-timing/

Server-Timing is an HTTP trailer, not a header. :mcmanus tells me we currently parse trailers, but then silently throw them away.  We don't want to change that behavior in general (we don't want to encourage trailers), so we'll want to whitelist the Server-Timing trailer, store it somewhere (probably even just a mServerTiming header will work for now, since it's the only trailer we support) and then make it available via some new channel.getTrailers() call. That call should fail with NS_NOT_AVAIL unless the channel has hit OnStopRequest().  (I.e we can't provide a trailer header value until we've gotten all the data from the channel).

Note: we're fine having devtools show the Server-Timing intertwined with regular HTTP headers:  we just want a separate API internally for getting trailers (that feels safer than modifying the original HTTP header array that we make available at OnStartRequest time).
(Reporter)

Updated

a year ago
Whiteboard: [necko-triaged]
(Reporter)

Updated

a year ago
Blocks: 1337684
(Reporter)

Updated

a year ago
Blocks: 1403051

Comment 1

a year ago
Server-Timing is a header AND a trailer (https://w3c.github.io/server-timing/#dfn-server-timing-header-field). 

The "MUST" word around the reference to trailers in the spec is a little strong and will likely be downgraded to a "SHOULD" or "MAY". Header parsing is in Chrome behind experimental-flag.

Comment 2

a year ago
Bad Jason, marking this triaged without a priority! :) (I assume you had a priority in mind, so I'll let you fix that.)
Flags: needinfo?(jduell.mcbugs)
(Reporter)

Comment 3

a year ago
Guilty as charged.  Also, thanks for inspiring me to notice that we have 30 other priority-less "triaged" necko bugs :)

Leaving this ni'd to me while I find an owner.
Flags: needinfo?(jduell.mcbugs)
Priority: -- → P2
this is on my plate :)
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
(Reporter)

Comment 5

a year ago
Kershaw is going to code this and Dragana will mentor/review.
Assignee: dd.mozilla → kechang
Mentor: dd.mozilla
(Assignee)

Comment 6

a year ago
It seems there is no existed parser that can be used directly to parse server-timing header, so I think the first step is to create a parser.

Patch summary:
 - A server-timing header parser
 - A test case which is most copied from chromium

Hi Dragana,
Could you take a look at this patch? Thanks.
Attachment #8928947 - Flags: feedback?(dd.mozilla)

Comment 7

a year ago
FYI there is an imminent spec change for shorter named parameters in the response header:
https://github.com/w3c/server-timing/pull/41

`duration` -> `dur`
`description` -> `desc`

Web interface (`PerformanceServerTiming`) remains unchanged.
(In reply to Kershaw Chang [:kershaw] from comment #6)
> Created attachment 8928947 [details] [diff] [review]
> Part1: Serve-Timing header parser
> 
> It seems there is no existed parser that can be used directly to parse
> server-timing header, so I think the first step is to create a parser.
> 
> Patch summary:
>  - A server-timing header parser
>  - A test case which is most copied from chromium
> 
> Hi Dragana,
> Could you take a look at this patch? Thanks.

Can you try to reuse ParsedHeaderValueList and ParsedHeaderValueListList? Maybe extend it if needed? It looks like it is doing a similar parsing.

Comment 9

a year ago
FWIW, the format of the server-timing header is very similar to "Link":
https://tools.ietf.org/html/rfc5988#section-5
(Assignee)

Comment 10

a year ago
> Can you try to reuse ParsedHeaderValueList and ParsedHeaderValueListList?
> Maybe extend it if needed? It looks like it is doing a similar parsing.

Thanks for the suggestion.
Actually, ParsedHeaderValueListList doesn't parse quoted string well. For example, it doesn't take care of backslash escape (RFC 2616 Section 2.2). So, I think I should merge my code with ParsedHeaderValueListList to make it work correctly.

I'll update a patch to do this later.
(Assignee)

Comment 11

a year ago
Summary:
1. Modify ParsedHeaderValueListList
  - Use nsCCharSeparatedTokenizer to split string by ','.
2. Modify ParsedHeaderValueList
  - Use nsCCharSeparatedTokenizer to split string by ';'.
  - Function ParseNameAndValue() is for getting name and value from the string containing '='.
3. Handle backslash escapes for quoted string.
4. Reuse ParsedHeaderValueListList to parse server-timing header.
5. Create a test case.

Thanks.
Attachment #8928947 - Attachment is obsolete: true
Attachment #8928947 - Flags: feedback?(dd.mozilla)
Attachment #8930838 - Flags: review?(dd.mozilla)
(Assignee)

Comment 12

a year ago
(In reply to cvazac from comment #7)
> FYI there is an imminent spec change for shorter named parameters in the
> response header:
> https://github.com/w3c/server-timing/pull/41
> 
> `duration` -> `dur`
> `description` -> `desc`
> 
> Web interface (`PerformanceServerTiming`) remains unchanged.

Thanks for the information.
This change has been included in my patch.

Comment 13

a year ago
Delimiting characters (; and ,) are legal in quoted strings.
See `qdtext` here: https://tools.ietf.org/html/rfc7230#section-3.2.6

This header:
Server-Timing: metric;desc="descr;,=iption";dur=123.4

Should yield entry: 
name = metric
duration = 123.4
description = descr;,=iption

Without running the code, I worry that splitting on commas and semi-colons won't handle this correctly.
(Assignee)

Comment 14

a year ago
(In reply to cvazac from comment #13)
> Delimiting characters (; and ,) are legal in quoted strings.
> See `qdtext` here: https://tools.ietf.org/html/rfc7230#section-3.2.6
> 
> This header:
> Server-Timing: metric;desc="descr;,=iption";dur=123.4
> 
> Should yield entry: 
> name = metric
> duration = 123.4
> description = descr;,=iption
> 
> Without running the code, I worry that splitting on commas and semi-colons
> won't handle this correctly.

Yes, you are right. I should not use tokenizer to split string.
Thanks for your great help!
(Assignee)

Comment 15

a year ago
Comment on attachment 8930838 [details] [diff] [review]
Part1: Serve-Timing header parser - v2

Review of attachment 8930838 [details] [diff] [review]:
-----------------------------------------------------------------

Cancel the review based on comment 14.
Attachment #8930838 - Flags: review?(dd.mozilla)
(Assignee)

Comment 17

a year ago
Summary:
  - Create a custom function to split the input string that contains quoted-string and quoted-pair.
 - The rest is the same as comment #11.

Thanks.
Attachment #8930838 - Attachment is obsolete: true
Attachment #8932033 - Flags: review?(dd.mozilla)
(Assignee)

Comment 18

a year ago
Summary:
1. Save http trailers in the header array in chunk decoder.
2. Take trailers from chunk decoder in OnStopRequest().
3. Add serverTiming attribute in nsITimedChannel.
4. Parse server timing header when GetServerTiming() is called.

Thanks.
Attachment #8932429 - Flags: review?(dd.mozilla)
(Assignee)

Comment 19

a year ago
Posted patch Part3: IPC part (obsolete) — Splinter Review
Summary:
 - Send http trailers to child process

Thanks.
Attachment #8932430 - Flags: review?(dd.mozilla)
(In reply to Kershaw Chang [:kershaw] from comment #10)
> > Can you try to reuse ParsedHeaderValueList and ParsedHeaderValueListList?
> > Maybe extend it if needed? It looks like it is doing a similar parsing.
> 
> Thanks for the suggestion.
> Actually, ParsedHeaderValueListList doesn't parse quoted string well. For
> example, it doesn't take care of backslash escape (RFC 2616 Section 2.2).
> So, I think I should merge my code with ParsedHeaderValueListList to make it
> work correctly.
> 
> I'll update a patch to do this later.

RFC 2616 is deprecated please refer to a new one rfc7231 or one of rfc-s 7230, 7232, 7233, 7234 or 7235 depends what are you looking for.
(Assignee)

Comment 21

a year ago
(In reply to Dragana Damjanovic [:dragana] from comment #20)
> (In reply to Kershaw Chang [:kershaw] from comment #10)
> > > Can you try to reuse ParsedHeaderValueList and ParsedHeaderValueListList?
> > > Maybe extend it if needed? It looks like it is doing a similar parsing.
> > 
> > Thanks for the suggestion.
> > Actually, ParsedHeaderValueListList doesn't parse quoted string well. For
> > example, it doesn't take care of backslash escape (RFC 2616 Section 2.2).
> > So, I think I should merge my code with ParsedHeaderValueListList to make it
> > work correctly.
> > 
> > I'll update a patch to do this later.
> 
> RFC 2616 is deprecated please refer to a new one rfc7231 or one of rfc-s
> 7230, 7232, 7233, 7234 or 7235 depends what are you looking for.

It seems that the rules for quoted-string in RFC 2616 is almost the same as RFC 7230.
So, I think my patch still works.
(In reply to Kershaw Chang [:kershaw] from comment #21)
> (In reply to Dragana Damjanovic [:dragana] from comment #20)
> > (In reply to Kershaw Chang [:kershaw] from comment #10)
> > > > Can you try to reuse ParsedHeaderValueList and ParsedHeaderValueListList?
> > > > Maybe extend it if needed? It looks like it is doing a similar parsing.
> > > 
> > > Thanks for the suggestion.
> > > Actually, ParsedHeaderValueListList doesn't parse quoted string well. For
> > > example, it doesn't take care of backslash escape (RFC 2616 Section 2.2).
> > > So, I think I should merge my code with ParsedHeaderValueListList to make it
> > > work correctly.
> > > 
> > > I'll update a patch to do this later.
> > 
> > RFC 2616 is deprecated please refer to a new one rfc7231 or one of rfc-s
> > 7230, 7232, 7233, 7234 or 7235 depends what are you looking for.
> 
> It seems that the rules for quoted-string in RFC 2616 is almost the same as
> RFC 7230.
> So, I think my patch still works.

yes it is, I was just informing you for a future work, so that you know that is deprecated and not a valid standard.
Comment on attachment 8932033 [details] [diff] [review]
Part1: Serve-Timing header parser - v3

Review of attachment 8932033 [details] [diff] [review]:
-----------------------------------------------------------------

please address my comments.

::: netwerk/protocol/http/nsHttp.cpp
@@ +585,5 @@
> +    if (nameLen > 0) {
> +        mName.Rebind(name, name + nameLen);
> +    }
> +    if (valLen > 0) {
> +        mValue.Rebind(val, val + valLen);

you can avoid Rebind() twice if the string is a quoted string. Rebind only make a dependent string so it is not very expensive. You can decide if you want to change this.

@@ +636,2 @@
>  
> +    const char *last = input;

is 'first' a better name here? While as was reading the code I was wondering why it is called 'last' :)

@@ +694,5 @@
> +    int32_t nameLen = nameEnd - nameStart + 1;
> +
> +    if (!nameLen || nameLen == inputLen) {
> +        mValues.AppendElement(ParsedHeaderPair(nameStart, nameLen,
> +                                               valueStart, 0, false));

use nullpt instead of valueStart, it makes more sense.

@@ +699,5 @@
> +        return;
> +    }
> +
> +    for (; nameEnd >= nameStart; --nameEnd) {
> +        if (!nsCRT::IsAsciiSpace(*nameEnd)) {

a not correctly formatted header, for example 'Server-Timing: something something2'

will return 'something something2' as a token, which is not a token because it has a space in it. I think it is ok, or should we return only the first one?

There is a text in the spec only regarding multiple server-timing-param-name.

Can you add a test for this case?
Also can you add a test for multiple server-timing-param-name as well?

@@ +717,5 @@
> +    if (*input != '"') {
> +        // The value is a token, not a quoted string.
> +        valueStart = input;
> +        for (valueEnd = input;
> +             *valueEnd && !nsCRT::IsAsciiSpace (*valueEnd) &&

you guarantee that this is a 0-ended string or you want to limit iteration until inputLen is reached?

In some cases you are checking inputLen.

@@ +787,5 @@
> +        // According to spec, the first ParsedHeaderPair's name is metric-name.
> +        auto timingHeader = mServerTimingHeaders.AppendElement();
> +        timingHeader->name.Assign(parsedHeader.mValues[index].mValues[0].mName);
> +
> +        if (parsedHeader.mValues[index].mValues.Length() < 1) {

should here be == 1 ?
Attachment #8932033 - Flags: review?(dd.mozilla) → review+
Attachment #8932430 - Flags: review?(dd.mozilla) → review+
Comment on attachment 8932429 [details] [diff] [review]
Part2: Take http trailers from chunk decoder and add server timing attribute

Review of attachment 8932429 [details] [diff] [review]:
-----------------------------------------------------------------

Are you going to implement webIDL as well?

Chrome implements this already? Does Chrome request to have "Trailer: Server-Timing" header?

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +4395,5 @@
>  }
>  
> +namespace {
> +
> +class nsServerTiming final : public nsIServerTiming

do we really need nsServerTiming and ServerTimingStruct?

Thinking of it do we really need ServerTimingParser class. You can also make all of this simpler:
GetServerTiming->ParseServerTimingHeader->ServerTimingParser parser(serverTimingHeader);->parser.Parse();->parser.TakeServerTimingHeaders();->append to an array->convert to another array.

::: netwerk/protocol/http/nsHttpChunkedDecoder.cpp
@@ +125,5 @@
> +                nsAutoCString headerNameOriginal;
> +                nsAutoCString val;
> +                if (NS_SUCCEEDED(mTrailers->ParseHeaderLine(nsDependentCSubstring(buf, count),
> +                                                            &hdr, &headerNameOriginal, &val))) {
> +                    Unused << mTrailers->SetHeaderFromNet(hdr, headerNameOriginal,

Please ignore all other headers except Server-Timing.

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +1758,5 @@
>  
>      // check for end-of-file
>      if ((mContentRead == mContentLength) ||
>          (mChunkedDecoder && mChunkedDecoder->ReachedEOF())) {
> +        mForTakeResponseTrailers = mChunkedDecoder

this should happen under a lock.

::: netwerk/protocol/http/nsHttpTransaction.h
@@ +368,5 @@
>  
>      // protected by nsHttp::GetLock()
>      nsHttpResponseHead             *mForTakeResponseHead;
>      bool                            mResponseHeadTaken;
> +    nsHttpHeaderArray              *mForTakeResponseTrailers;

Can we have a autoPtr here, anything better than raw pointer.

Also you have forgot to delete it in ~nsHttpTransaction() :)
Attachment #8932429 - Flags: review?(dd.mozilla) → feedback+
(Assignee)

Comment 25

a year ago
(In reply to Dragana Damjanovic [:dragana] from comment #24)
> Comment on attachment 8932429 [details] [diff] [review]
> Part2: Take http trailers from chunk decoder and add server timing attribute
> 
> Review of attachment 8932429 [details] [diff] [review]:
> -----------------------------------------------------------------
> 

Thanks for the review.

> Are you going to implement webIDL as well?
> 

I am not sure, but that should be happened in another bug.

> Chrome implements this already? Does Chrome request to have "Trailer:
> Server-Timing" header?
> 

If I read their code correctly, the answer is no. It looks like that they don't even parse trailer in their chunked decoder.
https://cs.chromium.org/chromium/src/net/http/http_chunked_decoder.h?type=cs&sq=package:chromium&l=78

> ::: netwerk/protocol/http/HttpBaseChannel.cpp
> @@ +4395,5 @@
> >  }
> >  
> > +namespace {
> > +
> > +class nsServerTiming final : public nsIServerTiming
> 
> do we really need nsServerTiming and ServerTimingStruct?
> 

nsIServerTiming is just for easily testing server timing headers in JS. Not sure if we want to let httpd.js support sending server timing header in trailers.
Perhaps only ServerTimingStruct is enough, if we don't have to test this in JS.

> Thinking of it do we really need ServerTimingParser class.

I think you mean maybe we can just provide header string to devtool or dom?
IMO, the server timing header string eventually need to be parsed, so I think it would be better only happened once in necko.

> You can also make all of this simpler:
> GetServerTiming->ParseServerTimingHeader->ServerTimingParser
> parser(serverTimingHeader);->parser.Parse();->parser.
> TakeServerTimingHeaders();->append to an array->convert to another array.
> 

Assume we don't need nsIServerTiming. The flow would be like:
GetServerTiming->ParseServerTimingHeader->ServerTimingParser parser(serverTimingHeader);->parser.Parse();
->parser.TakeServerTimingHeaders();->append to an array.

Or I can try to just pass nsHttpHeaderArray to ServerTimingParser, so the flow would be like:
GetServerTiming->ServerTimingParser parser(headerArray);->parser.Parse();
->parser.TakeServerTimingHeaders();->append to an array.

Not sure if there other way to make it simpler.

> ::: netwerk/protocol/http/nsHttpChunkedDecoder.cpp
> @@ +125,5 @@
> > +                nsAutoCString headerNameOriginal;
> > +                nsAutoCString val;
> > +                if (NS_SUCCEEDED(mTrailers->ParseHeaderLine(nsDependentCSubstring(buf, count),
> > +                                                            &hdr, &headerNameOriginal, &val))) {
> > +                    Unused << mTrailers->SetHeaderFromNet(hdr, headerNameOriginal,
> 
> Please ignore all other headers except Server-Timing.
> 

Will do.

> ::: netwerk/protocol/http/nsHttpTransaction.cpp
> @@ +1758,5 @@
> >  
> >      // check for end-of-file
> >      if ((mContentRead == mContentLength) ||
> >          (mChunkedDecoder && mChunkedDecoder->ReachedEOF())) {
> > +        mForTakeResponseTrailers = mChunkedDecoder
> 
> this should happen under a lock.
> 

Will do.

> ::: netwerk/protocol/http/nsHttpTransaction.h
> @@ +368,5 @@
> >  
> >      // protected by nsHttp::GetLock()
> >      nsHttpResponseHead             *mForTakeResponseHead;
> >      bool                            mResponseHeadTaken;
> > +    nsHttpHeaderArray              *mForTakeResponseTrailers;
> 
> Can we have a autoPtr here, anything better than raw pointer.
> 
> Also you have forgot to delete it in ~nsHttpTransaction() :)

Will do.
Flags: needinfo?(dd.mozilla)
(In reply to Kershaw Chang [:kershaw] from comment #25)
> (In reply to Dragana Damjanovic [:dragana] from comment #24)
> > Comment on attachment 8932429 [details] [diff] [review]
> > Part2: Take http trailers from chunk decoder and add server timing attribute
> > 
> > Review of attachment 8932429 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> 
> Thanks for the review.
> 
> > Are you going to implement webIDL as well?
> > 
> 
> I am not sure, but that should be happened in another bug.
> 

As you prefer, but please file a bug if you are not going to implement it here.


> > Chrome implements this already? Does Chrome request to have "Trailer:
> > Server-Timing" header?
> > 
> 
> If I read their code correctly, the answer is no. It looks like that they
> don't even parse trailer in their chunked decoder.
> https://cs.chromium.org/chromium/src/net/http/http_chunked_decoder.
> h?type=cs&sq=package:chromium&l=78
> 


I just saw some tweets and have not look deep to figure out if they have implemented a trailer parsing as well. I think we do not need to enforce it.

> > ::: netwerk/protocol/http/HttpBaseChannel.cpp
> > @@ +4395,5 @@
> > >  }
> > >  
> > > +namespace {
> > > +
> > > +class nsServerTiming final : public nsIServerTiming
> > 
> > do we really need nsServerTiming and ServerTimingStruct?
> > 
> 
> nsIServerTiming is just for easily testing server timing headers in JS. Not
> sure if we want to let httpd.js support sending server timing header in
> trailers.
> Perhaps only ServerTimingStruct is enough, if we don't have to test this in
> JS.

if you implement webIDL interface you can use that for testing.
how would interface to dev-tools look like? you will need an interface to js (the dev-tools patch from bug 1403051 parse s-t header in js, but they do not need to do that if we are doing it for them)? The performance timing dom code uses nsITimedChannel to get timing info. and dev-tools use it as well.

if you do not implement trailers in httpd.js (or use that interface Jason was talking about) how are you planning to automatically test the trailer code?

> 
> > Thinking of it do we really need ServerTimingParser class.
> 
> I think you mean maybe we can just provide header string to devtool or dom?
> IMO, the server timing header string eventually need to be parsed, so I
> think it would be better only happened once in necko.
> 
> > You can also make all of this simpler:
> > GetServerTiming->ParseServerTimingHeader->ServerTimingParser
> > parser(serverTimingHeader);->parser.Parse();->parser.
> > TakeServerTimingHeaders();->append to an array->convert to another array.
> > 
> 
> Assume we don't need nsIServerTiming. The flow would be like:
> GetServerTiming->ParseServerTimingHeader->ServerTimingParser
> parser(serverTimingHeader);->parser.Parse();
> ->parser.TakeServerTimingHeaders();->append to an array.
> 
> Or I can try to just pass nsHttpHeaderArray to ServerTimingParser, so the
> flow would be like:
> GetServerTiming->ServerTimingParser parser(headerArray);->parser.Parse();
> ->parser.TakeServerTimingHeaders();->append to an array.
> 
> Not sure if there other way to make it simpler.

This is only helper function. WebIDL and dev-tools are the only one using it. Therefore it  would be good to implement exactly what they need. Probably our tests will need the same interface.
Flags: needinfo?(dd.mozilla)
(Assignee)

Comment 27

a year ago
> As you prefer, but please file a bug if you are not going to implement it
> here.
> 

I've filed bug 1423495 for this.

> 
> > > Chrome implements this already? Does Chrome request to have "Trailer:
> > > Server-Timing" header?
> > > 
> > 
> > If I read their code correctly, the answer is no. It looks like that they
> > don't even parse trailer in their chunked decoder.
> > https://cs.chromium.org/chromium/src/net/http/http_chunked_decoder.
> > h?type=cs&sq=package:chromium&l=78
> > 
> 
> 
> I just saw some tweets and have not look deep to figure out if they have
> implemented a trailer parsing as well. I think we do not need to enforce it.
> 

Do you mean we don't have to parse trailers or?

> if you implement webIDL interface you can use that for testing.
> how would interface to dev-tools look like? you will need an interface to js
> (the dev-tools patch from bug 1403051 parse s-t header in js, but they do
> not need to do that if we are doing it for them)? The performance timing dom
> code uses nsITimedChannel to get timing info. and dev-tools use it as well.
> 

I already ask Honza about the requirements from dev-tool in bug 1403051.

> if you do not implement trailers in httpd.js (or use that interface Jason
> was talking about) how are you planning to automatically test the trailer
> code?
> 

That's true. It is necessary to implement trailer in httpd.js.

> This is only helper function. WebIDL and dev-tools are the only one using
> it. Therefore it  would be good to implement exactly what they need.
> Probably our tests will need the same interface.

Let's ask them first before deciding what to do.
(Assignee)

Comment 28

a year ago
(In reply to Kershaw Chang [:kershaw] from comment #27)
> > As you prefer, but please file a bug if you are not going to implement it
> > here.
> > 
> 
> I've filed bug 1423495 for this.
> 
> > 
> > > > Chrome implements this already? Does Chrome request to have "Trailer:
> > > > Server-Timing" header?
> > > > 
> > > 
> > > If I read their code correctly, the answer is no. It looks like that they
> > > don't even parse trailer in their chunked decoder.
> > > https://cs.chromium.org/chromium/src/net/http/http_chunked_decoder.
> > > h?type=cs&sq=package:chromium&l=78
> > > 
> > 
> > 
> > I just saw some tweets and have not look deep to figure out if they have
> > implemented a trailer parsing as well. I think we do not need to enforce it.
> > 
> 
> Do you mean we don't have to parse trailers or?
> 
> > if you implement webIDL interface you can use that for testing.
> > how would interface to dev-tools look like? you will need an interface to js
> > (the dev-tools patch from bug 1403051 parse s-t header in js, but they do
> > not need to do that if we are doing it for them)? The performance timing dom
> > code uses nsITimedChannel to get timing info. and dev-tools use it as well.
> > 
> 
> I already ask Honza about the requirements from dev-tool in bug 1403051.
> 

It looks to me that we really need a array of nsIServerTiming in nsITimedChannel, so dev-tool and dom can get server timing info from nsITimedChannel.
I think I can get rid off ServerTimingStruct and let the parser return nsTArray<nsCOMPtr<nsIServerTiming>>. This could make the code a bit simpler. What do you think, Dragana?
Flags: needinfo?(dd.mozilla)
(In reply to Kershaw Chang [:kershaw] from comment #28)
> (In reply to Kershaw Chang [:kershaw] from comment #27)
> 
> It looks to me that we really need a array of nsIServerTiming in
> nsITimedChannel, so dev-tool and dom can get server timing info from
> nsITimedChannel.
> I think I can get rid off ServerTimingStruct and let the parser return
> nsTArray<nsCOMPtr<nsIServerTiming>>. This could make the code a bit simpler.
> What do you think, Dragana?


That is what I was thinking about :)
Flags: needinfo?(dd.mozilla)
Blocks: 1423495

Comment 30

a year ago
(In reply to Dragana Damjanovic [:dragana] from comment #26)
> (In reply to Kershaw Chang [:kershaw] from comment #25)
> > (In reply to Dragana Damjanovic [:dragana] from comment #24)
> > > Comment on attachment 8932429 [details] [diff] [review]
> > > Part2: Take http trailers from chunk decoder and add server timing attribute
> > > 
> 
> > > Chrome implements this already? Does Chrome request to have "Trailer:
> > > Server-Timing" header?
> > > 
> > 
> > If I read their code correctly, the answer is no. It looks like that they
> > don't even parse trailer in their chunked decoder.
> > https://cs.chromium.org/chromium/src/net/http/http_chunked_decoder.
> > h?type=cs&sq=package:chromium&l=78
> > 
> 
> 

The Chromium implementation does not support Server-Timing trailers, only headers. cvazac (from previous comments) is the Chromium implementer as well as the spec editor, so you can probably ping him for questions related to that implementation.
(Assignee)

Comment 31

a year ago
Please see below for the changes between v3 and v4.

> ::: netwerk/protocol/http/nsHttp.cpp
> @@ +585,5 @@
> > +    if (nameLen > 0) {
> > +        mName.Rebind(name, name + nameLen);
> > +    }
> > +    if (valLen > 0) {
> > +        mValue.Rebind(val, val + valLen);
> 
> you can avoid Rebind() twice if the string is a quoted string. Rebind only
> make a dependent string so it is not very expensive. You can decide if you
> want to change this.
> 

Fixed.

> @@ +636,2 @@
> >  
> > +    const char *last = input;
> 
> is 'first' a better name here? While as was reading the code I was wondering
> why it is called 'last' :)
> 

Agree. This should be "first".

> @@ +694,5 @@
> > +    int32_t nameLen = nameEnd - nameStart + 1;
> > +
> > +    if (!nameLen || nameLen == inputLen) {
> > +        mValues.AppendElement(ParsedHeaderPair(nameStart, nameLen,
> > +                                               valueStart, 0, false));
> 
> use nullpt instead of valueStart, it makes more sense.
> 

Done.

> @@ +699,5 @@
> > +        return;
> > +    }
> > +
> > +    for (; nameEnd >= nameStart; --nameEnd) {
> > +        if (!nsCRT::IsAsciiSpace(*nameEnd)) {
> 
> a not correctly formatted header, for example 'Server-Timing: something
> something2'
> 
> will return 'something something2' as a token, which is not a token because
> it has a space in it. I think it is ok, or should we return only the first
> one?
>

I am not sure what is right to do. It seems that this is not specified.
But, I think returning the first one makes more sense, so I also made this change in this patch.
 
> There is a text in the spec only regarding multiple server-timing-param-name.
> 
> Can you add a test for this case?
> Also can you add a test for multiple server-timing-param-name as well?
> 

Already added in TestServerTimingHeader.cpp.

> @@ +717,5 @@
> > +    if (*input != '"') {
> > +        // The value is a token, not a quoted string.
> > +        valueStart = input;
> > +        for (valueEnd = input;
> > +             *valueEnd && !nsCRT::IsAsciiSpace (*valueEnd) &&
> 
> you guarantee that this is a 0-ended string or you want to limit iteration
> until inputLen is reached?
> 
> In some cases you are checking inputLen.
> 

Right now I check 0-ended string for all places.

> @@ +787,5 @@
> > +        // According to spec, the first ParsedHeaderPair's name is metric-name.
> > +        auto timingHeader = mServerTimingHeaders.AppendElement();
> > +        timingHeader->name.Assign(parsedHeader.mValues[index].mValues[0].mName);
> > +
> > +        if (parsedHeader.mValues[index].mValues.Length() < 1) {
> 
> should here be == 1 ?

Yes.

Please note that I also copy all test cases from the Chromium [1] to TestServerTimingHeader.cpp.
The only failed test case is:
testServerTimingHeader("metric=foo;dur;dur=123.4,metric2",
                         {{"metric", "0", ""}});

In my current patch, the result is {{"metric", "123.4", ""}, {"metric2", "0", ""}}.
I think this should be fine, since this is not covered by spec?

[1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/network/HTTPParsersTest.cpp?type=cs&sq=package:chromium&l=510
Attachment #8932033 - Attachment is obsolete: true
Attachment #8936932 - Flags: review?(dd.mozilla)
(Assignee)

Comment 32

a year ago
Posted patch Diff for v3 and v4 of part1. (obsolete) — Splinter Review
Just in case you might want to compare.
(Assignee)

Comment 33

a year ago
Summary:
 - Address previous comments.
 - Move ServerTimingParser from nsHttp.h to nsSerTiming.h.

Thanks.
Attachment #8932429 - Attachment is obsolete: true
Attachment #8937382 - Flags: review?(dd.mozilla)
(Assignee)

Comment 34

a year ago
Posted patch Part4: Test case (obsolete) — Splinter Review
Summary:
 - To check whether the serving timing header can be parsed successfully from response and trailer headers.

Thanks.
Attachment #8937383 - Flags: review?(dd.mozilla)
Attachment #8937383 - Flags: review?(dd.mozilla) → review+
(In reply to Kershaw Chang [:kershaw] from comment #31)
> Created attachment 8936932 [details] [diff] [review]
> Part1: Serve-Timing header parser - v4
> 
> Please note that I also copy all test cases from the Chromium [1] to
> TestServerTimingHeader.cpp.
> The only failed test case is:
> testServerTimingHeader("metric=foo;dur;dur=123.4,metric2",
>                          {{"metric", "0", ""}});
> 
> In my current patch, the result is {{"metric", "123.4", ""}, {"metric2",
> "0", ""}}.

you accept 'metric=foo' as a name 'metric' that is ok, but you then have 'dur' 2 times and the spec says that you should only take the first one... although 'dur' is not a valid server-timing-param it is missing '=" and server-timing-param-value.

The spec says "individual server-timing-param-name SHOULD NOT appear multiple times within a server-timing-metric. If any parameter is specified more than once, only the first instance is to be considered."

it is talking only about the multiple server-timing-param-name, so the result should be {{"metric", "0", ""}, {"metric2", "0", ""}}


> I think this should be fine, since this is not covered by spec?
> 
> [1]
> https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/
> network/HTTPParsersTest.cpp?type=cs&sq=package:chromium&l=510
Attachment #8937382 - Flags: review?(dd.mozilla) → review+
Attachment #8936932 - Flags: review?(dd.mozilla) → review+
(Assignee)

Comment 37

a year ago
(In reply to Dragana Damjanovic [:dragana] from comment #36)
> (In reply to Kershaw Chang [:kershaw] from comment #31)
> > Created attachment 8936932 [details] [diff] [review]
> > Part1: Serve-Timing header parser - v4
> > 
> > Please note that I also copy all test cases from the Chromium [1] to
> > TestServerTimingHeader.cpp.
> > The only failed test case is:
> > testServerTimingHeader("metric=foo;dur;dur=123.4,metric2",
> >                          {{"metric", "0", ""}});
> > 
> > In my current patch, the result is {{"metric", "123.4", ""}, {"metric2",
> > "0", ""}}.
> 
> you accept 'metric=foo' as a name 'metric' that is ok, but you then have
> 'dur' 2 times and the spec says that you should only take the first one...
> although 'dur' is not a valid server-timing-param it is missing '=" and
> server-timing-param-value.
> 
> The spec says "individual server-timing-param-name SHOULD NOT appear
> multiple times within a server-timing-metric. If any parameter is specified
> more than once, only the first instance is to be considered."
> 
> it is talking only about the multiple server-timing-param-name, so the
> result should be {{"metric", "0", ""}, {"metric2", "0", ""}}
> 

According to the tests [1] from chromium, the result of "metric;dur;dur=123.4" should be {"metric", "123.4", ""}.
I think it makes sense to take the first "valid" server-timing-param.


[1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/network/HTTPParsersTest.cpp?type=cs&sq=package:chromium&l=651-653
(Assignee)

Comment 38

a year ago
Posted patch Part5: Fix-test_altsvc.js (obsolete) — Splinter Review
Summary:
 - Use quoted-string as the header value of "Alt-Svc".
 - For example, the original header value of Alt-Svc in test_altsvc.js h2=foo.example.com:62933 is wrong. The correct value should be h2="foo.example.com:62933".

Thanks.
Attachment #8937925 - Flags: review?(dd.mozilla)
(In reply to Kershaw Chang [:kershaw] from comment #38)
> Created attachment 8937925 [details] [diff] [review]
> Part5: Fix-test_altsvc.js
> 
> Summary:
>  - Use quoted-string as the header value of "Alt-Svc".
>  - For example, the original header value of Alt-Svc in test_altsvc.js
> h2=foo.example.com:62933 is wrong. The correct value should be
> h2="foo.example.com:62933".
> 
> Thanks.

Does the old version, h2=foo.example.com:62933, still works?

Let's try not to regress alt-svc.
(Assignee)

Comment 40

a year ago
(In reply to Dragana Damjanovic [:dragana] from comment #39)
> (In reply to Kershaw Chang [:kershaw] from comment #38)
> > Created attachment 8937925 [details] [diff] [review]
> > Part5: Fix-test_altsvc.js
> > 
> > Summary:
> >  - Use quoted-string as the header value of "Alt-Svc".
> >  - For example, the original header value of Alt-Svc in test_altsvc.js
> > h2=foo.example.com:62933 is wrong. The correct value should be
> > h2="foo.example.com:62933".
> > 
> > Thanks.
> 
> Does the old version, h2=foo.example.com:62933, still works?
> 
> Let's try not to regress alt-svc.

Unfortunately, "h2=foo.example.com:62933" does not work.
So, should we allow invalid token for parsing "Alt-Svc" and "Server-Timing" headers? Or only allow "Alt-Svc" to have an invalid token?

IMO, we should allow for both headers. This makes the code look a bit clear.
Flags: needinfo?(dd.mozilla)
(In reply to Kershaw Chang [:kershaw] from comment #40)
> (In reply to Dragana Damjanovic [:dragana] from comment #39)
> > (In reply to Kershaw Chang [:kershaw] from comment #38)
> > > Created attachment 8937925 [details] [diff] [review]
> > > Part5: Fix-test_altsvc.js
> > > 
> > > Summary:
> > >  - Use quoted-string as the header value of "Alt-Svc".
> > >  - For example, the original header value of Alt-Svc in test_altsvc.js
> > > h2=foo.example.com:62933 is wrong. The correct value should be
> > > h2="foo.example.com:62933".
> > > 
> > > Thanks.
> > 
> > Does the old version, h2=foo.example.com:62933, still works?
> > 
> > Let's try not to regress alt-svc.
> 
> Unfortunately, "h2=foo.example.com:62933" does not work.
> So, should we allow invalid token for parsing "Alt-Svc" and "Server-Timing"
> headers? Or only allow "Alt-Svc" to have an invalid token?
> 
> IMO, we should allow for both headers. This makes the code look a bit clear.


Let's ask Patrick, it is his draft and he have seen more of alt-svc headers.

Patrick, the current code allows alt-svc header:
Alt-svc: h2=foo.example.com:62933
without '"'
The spec says the value of "alt-authority" should be quoted-string, i.e. Alt-svc: h2="foo.example.com:62933"

The new code will enforce quoted-string. Do you think that will cause a regression?
Flags: needinfo?(dd.mozilla) → needinfo?(mcmanus)
you should not regress alt-svc but implement server-timings the stricter way. alt-svc should have been done that way initially but fixing it is all risk and no reward
Flags: needinfo?(mcmanus)

Comment 43

a year ago
I opened up this issue against the spec to clarify the handling of params with unspecified or invalid values. Would love your participation!

https://github.com/w3c/server-timing/issues/46
(Assignee)

Comment 44

a year ago
As Patrick said, this patch adds a flag to allow invalid token value when parsing Alt-Svc header.
Attachment #8937925 - Attachment is obsolete: true
Attachment #8937925 - Flags: review?(dd.mozilla)
Attachment #8938308 - Flags: review?(dd.mozilla)

Comment 45

a year ago
Note that the tendancy on the spec issue and in the related PR[1] is to modify the parsing to not check for the values' validity.
Patrick - do you think this is a mistake and we should take validity into account? (that's how I read #42, but would love clarifications)

[1] https://github.com/w3c/server-timing/pull/47
(In reply to Yoav Weiss from comment #45)
> Note that the tendancy on the spec issue and in the related PR[1] is to
> modify the parsing to not check for the values' validity.
> Patrick - do you think this is a mistake and we should take validity into
> account? (that's how I read #42, but would love clarifications)
> 
> [1] https://github.com/w3c/server-timing/pull/47


For Service-Timing - this the first implementation in Firefox and it would be good to well define everything in the spec and have appropriate tests so that there is no confusion afterwords.

In my opinion the PR looks good, just one thing

"If any server-timing-param-name is specified more than once, only the first instance is to be considered, even if it is incomplete..."

actually "even if it is incomplete.." should it be "even if its specification is..." in the example server-timing-params are incomplete  or invalid not server-timing-param-names.

Just a note:
Firefox will interpret 'dur=123.4 567.8;desc=...' (this is invalid) in the same way as 'dur=123.4;desc=...'

similar 'desc=something1 something2;' (note a non-quoted string and it is not a token, therefore invalid) will be the same as 'desc=something1;'
Comment on attachment 8938308 [details] [diff] [review]
Add a flag to allow invalid token value

Review of attachment 8938308 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttp.h
@@ +292,5 @@
>  
>  class ParsedHeaderValueList
>  {
>  public:
> +    ParsedHeaderValueList(const char *t, uint32_t len, bool allowInvalidValue);

Can you please add a longer comment explaining what allowInvalidValue does and where and why it is used.
Attachment #8938308 - Flags: review?(dd.mozilla) → review+

Comment 48

a year ago
A related but different issue for invalid params:
https://github.com/w3c/server-timing/issues/48
(Assignee)

Comment 49

a year ago
unbitrotting and carry r+.
Attachment #8932430 - Attachment is obsolete: true
Attachment #8936932 - Attachment is obsolete: true
Attachment #8936934 - Attachment is obsolete: true
Attachment #8937382 - Attachment is obsolete: true
Attachment #8937383 - Attachment is obsolete: true
Attachment #8938308 - Attachment is obsolete: true
Attachment #8939507 - Flags: review+
(Assignee)

Comment 51

a year ago
Carry r+.
Attachment #8939509 - Flags: review+
(Assignee)

Comment 52

a year ago
Posted patch Part4: Test case, r=dragana (obsolete) — Splinter Review
Carry r+.
Attachment #8939510 - Flags: review+
(Assignee)

Comment 54

a year ago
Fix the compiler error of gtest.
Attachment #8939510 - Attachment is obsolete: true
Attachment #8939714 - Flags: review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 56

a year ago
Pushed by shindli@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b226b0cff55a
Part1: Parse Serve-Timing header r=dragana
https://hg.mozilla.org/integration/mozilla-inbound/rev/c92b27c7f6bd
Part2: Take http trailers from chunk decoder  and add serverTiming attribute in nsITimedChannel r=dragana
https://hg.mozilla.org/integration/mozilla-inbound/rev/c29cf020715f
Part3: IPC part, send trailers to child  process r=dragana
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6ad893078b2
Part4: Test case r=dragana
Keywords: checkin-needed

Comment 57

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b226b0cff55a
https://hg.mozilla.org/mozilla-central/rev/c92b27c7f6bd
https://hg.mozilla.org/mozilla-central/rev/c29cf020715f
https://hg.mozilla.org/mozilla-central/rev/d6ad893078b2
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59

Comment 58

a year ago
So I'm fairly certain this won't work for resources served over h2 - see https://searchfox.org/mozilla-central/rev/03877052c151a8f062eea177f684a2743cd7b1d5/netwerk/protocol/http/Http2Session.cpp#1436 - h2 just flat-out ignores trailers right now (it silently drops them on the floor after decompressing them, in order to keep our compression context in sync with the server).
(In reply to Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) from comment #58)
> So I'm fairly certain this won't work for resources served over h2 - see
> https://searchfox.org/mozilla-central/rev/
> 03877052c151a8f062eea177f684a2743cd7b1d5/netwerk/protocol/http/Http2Session.
> cpp#1436 - h2 just flat-out ignores trailers right now (it silently drops
> them on the floor after decompressing them, in order to keep our compression
> context in sync with the server).

Kershaw, can you take a look and open another bug for this?
Flags: needinfo?(kechang)
(Assignee)

Comment 60

a year ago
(In reply to Dragana Damjanovic [:dragana] from comment #59)
> (In reply to Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org)
> from comment #58)
> > So I'm fairly certain this won't work for resources served over h2 - see
> > https://searchfox.org/mozilla-central/rev/
> > 03877052c151a8f062eea177f684a2743cd7b1d5/netwerk/protocol/http/Http2Session.
> > cpp#1436 - h2 just flat-out ignores trailers right now (it silently drops
> > them on the floor after decompressing them, in order to keep our compression
> > context in sync with the server).
> 
> Kershaw, can you take a look and open another bug for this?

Thanks. I've filed bug 1429973 for this.
Flags: needinfo?(kechang)

Updated

a year ago
Depends on: 1436517
Depends on: 1504512
You need to log in before you can comment on or make changes to this bug.