Closed Bug 1154309 Opened 5 years ago Closed 4 years ago

Add missing fields to ResourceTiming objects

Categories

(Core :: DOM: Core & HTML, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: valentin, Assigned: valentin, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 16 obsolete files)

4.73 KB, patch
Details | Diff | Splinter Review
40.68 KB, patch
u408661
: review+
Details | Diff | Splinter Review
The spec has changed and a few new fields need to be added
http://www.w3.org/TR/resource-timing/#performanceresourcetiming

The missing fields are:
nextHopProtocol, transferSize, encodedBodySize, decodedBodySize
Assignee: nobody → nhughes
Mentor: hurley
Update: workerStart is also a missing resource timing field
Attached patch resource_timing.diff (obsolete) — Splinter Review
Hey Valentin, I was hoping to get some quick feedback on my progress toward this patch. Specifically, I was hoping you could comment on my implementation for the EncodedBodySize field. Thanks :)
Attachment #8635590 - Flags: feedback?(valentin.gosu)
Comment on attachment 8635590 [details] [diff] [review]
resource_timing.diff

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

The general direction looks good.
ContentLength and Protocol you can get right away.
TransferSize can be a bit more difficult, and for DecodedBodySize I guess we ought to change nsHttpCompressConv to do that.

::: dom/base/PerformanceEntry.h
@@ +91,5 @@
>  protected:
>    nsCOMPtr<nsISupports> mParent;
>    nsString mName;
>    nsString mEntryType;
> +  unsigned short mContentLength;

Not sure this needs to be here. My guess is that you can move it and its set/get methods to  PerformanceResourceTiming.h

::: dom/base/PerformanceResourceTiming.h
@@ +58,5 @@
> +  }
> +
> +  void SetNextHopProtocol(const nsAString& aNextHopProtocol)
> +  {
> +    mNextHopProtocol = aNextHopProtocol;

Presumably you can get this from a header as well.
See nsHttpChannel::ProcessAltService()
You need to parse that header value to get the protocol-id, but that shouldn't be too hard. Ask Nick if there's a better way.

@@ +139,5 @@
>    }
>  
> +  unsigned short TransferSize() const
> +  {
> +    //return mContentLength + ***size of headers***;

I can't seem to find a way to get the size of the headers right now :)
I think we may need to add some counting to nsHttpTransaction - to count every byte that is received (we currently do that, but in a weird way, and only for Gonk)

::: dom/base/nsPerformance.cpp
@@ +571,5 @@
>      InsertResourceEntry(performanceEntry);
> +
> +    nsAutoCString retval;
> +    performanceEntry->SetContentLength(
> +      (unsigned short)channel->GetRequestHeader(name, retval));

channel->GetRequestHeader returns an nsresult, which will usually be 0 (NS_OK)
What you probably want to do is call GetRequestHeader, then you can probably call
  int32_t length = retval.ToInteger(&rv);
  performanceEntry->SetContentLength(length);
Attachment #8635590 - Flags: feedback?(valentin.gosu) → feedback+
(In reply to Valentin Gosu [:valentin] from comment #4)
> Comment on attachment 8635590 [details] [diff] [review]
> resource_timing.diff
> 
> Review of attachment 8635590 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/PerformanceResourceTiming.h
> @@ +58,5 @@
> > +  }
> > +
> > +  void SetNextHopProtocol(const nsAString& aNextHopProtocol)
> > +  {
> > +    mNextHopProtocol = aNextHopProtocol;
> 
> Presumably you can get this from a header as well.
> See nsHttpChannel::ProcessAltService()
> You need to parse that header value to get the protocol-id, but that
> shouldn't be too hard. Ask Nick if there's a better way.

Nick - just hoping to get your thought on this approach; should we parse the header for this value? Thanks!
Flags: needinfo?(hurley)
(In reply to nhughes from comment #5)
> (In reply to Valentin Gosu [:valentin] from comment #4)
> > Comment on attachment 8635590 [details] [diff] [review]
> > resource_timing.diff
> > 
> > Review of attachment 8635590 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/base/PerformanceResourceTiming.h
> > @@ +58,5 @@
> > > +  }
> > > +
> > > +  void SetNextHopProtocol(const nsAString& aNextHopProtocol)
> > > +  {
> > > +    mNextHopProtocol = aNextHopProtocol;
> > 
> > Presumably you can get this from a header as well.
> > See nsHttpChannel::ProcessAltService()
> > You need to parse that header value to get the protocol-id, but that
> > shouldn't be too hard. Ask Nick if there's a better way.
> 
> Nick - just hoping to get your thought on this approach; should we parse the
> header for this value? Thanks!

If I'm understanding the spec correctly (this field is supposed to be the actual protocol used), then no, there is no header you can parse that will give you the proper answer (the Alt-Svc header field is there to say "hey, you *could* use this protocol on this port for this resource *if you want to*"; it's not a "here's what protocol we're talking now" thing).

What you really need to do is get the negotiated token from an nsISSLSocketControl. You can get to one of those by QI'ing the security info from an nsHttpConnection (Http2Session::ConfirmTLSProfile has an example of getting an nsISSLSocketControl from an nsHttpConnection - I don't know where you'll get the nsHttpConnection from, though). Once you have that, you can just call GetNegotiatedNPN on it:

nsCString npnToken;
nsresult rv = sslSocketControl->GetNegotiatedNPN(npnToken);

And then npnToken will have the appropriate value in it (assuming rv == NS_OK).

If any part of this process fails, then your best bet is to fall back to using "http/1.1" as the token.
Flags: needinfo?(hurley)
Actually, let me contradict myself - you *could* parse the X-Firefox-Spdy header (if it exists), and that will give you the NPN token. If that header doesn't exist, then "http/1.1" is the correct value. The X-Firefox-Spdy header is really simple:

X-Firefox-Spdy: <npn token>

for example, for HTTP/2, it's:

X-Firefox-Spdy: h2
Attached patch resource_timing.diff (obsolete) — Splinter Review
Hi Valentin, could I please get some feedback on workerStart? I'm still a bit confused as to how to calculate this value. I know it isn't relative to another time, similar to navigationStart, but I'm not sure when to mark that the worker started... Any information is appreciated. Thanks!
Attachment #8635590 - Attachment is obsolete: true
Attachment #8636804 - Flags: feedback?(valentin.gosu)
(In reply to nhughes from comment #8)
> Created attachment 8636804 [details] [diff] [review]
> resource_timing.diff
> 
> Hi Valentin, could I please get some feedback on workerStart? I'm still a
> bit confused as to how to calculate this value. I know it isn't relative to
> another time, similar to navigationStart, but I'm not sure when to mark that
> the worker started... Any information is appreciated. Thanks!

For the second part of the workerStart definition (immediately before the user agent fires an event named `fetch`) I think it should be in Fetch.cpp:FetchRequest(), but I'm not totally sure.

Boris, do you know where would we find an appropriate place to record workerStart ?
Flags: needinfo?(bzbarsky)
No idea; you should probably ask someone familiar with serviceworkers....
Flags: needinfo?(bzbarsky)
Hi Blake,
We'd like to add the workerStart timestamp to the list of resource timing fields we report according to the spec [1]
Do you know where's the best place to record the timestamp for worker start?

[1] http://www.w3.org/TR/resource-timing/#widl-PerformanceResourceTiming-workerStart
Flags: needinfo?(mrbkap)
Valentin, the best place would be in ServiceWorkerManager::DispatchFetchEvent.
Flags: needinfo?(mrbkap)
Attached patch resource_timing.diff (obsolete) — Splinter Review
Nick - does it look like I'm accessing the NPN correctly, in nsPerformance::addEntry?

Valentin - is the nextHopProtocol attribute being set at the correct time -- just after initiatorType in nsPerformance::addEntry?

Thanks for the feedback guys!
Attachment #8636804 - Attachment is obsolete: true
Attachment #8636804 - Flags: feedback?(valentin.gosu)
Attachment #8638588 - Flags: feedback?(valentin.gosu)
Attachment #8638588 - Flags: feedback?(hurley)
Comment on attachment 8638588 [details] [diff] [review]
resource_timing.diff

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

It looks good, but it would be a bit better if you did that before InsertResourceEntry.
Attachment #8638588 - Flags: feedback?(valentin.gosu) → feedback+
Comment on attachment 8638588 [details] [diff] [review]
resource_timing.diff

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

Looks pretty good. You need to add null and error checking, and a fallback to using "http/1.1" as the npn token if something fails, is null, or the answer comes back an empty string from GetNegotiatedNPN, but that's the right way to use GetNegotiatedNPN in the case when nothing goes wrong, yeah :)
Attachment #8638588 - Flags: feedback?(hurley) → feedback+
Comment on attachment 8638588 [details] [diff] [review]
resource_timing.diff

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

::: dom/base/nsPerformance.cpp
@@ +583,5 @@
> +    if (result == NS_OK)
> +    {
> +      int32_t length = 0;
> +      rv.ToInteger(&result, length);
> +      performanceEntry->SetContentLength(length);

Also, the usual pattern is rv = channel->GetResponseHeader("content-length", value).
So make rv be a nsresult, and use the _response_ header value, since that's what we are interested in.
Nathan, you might need to know a few things, especially if we want this to work for e10s as well.

So, as we established, encodedBodySize is contentLength.
Come to think about it, you're much better using channel->GetContentLength(&length) instead of parsing the response header. (length should be int64_t)

---

transferSize is something that we have to instrument nsHttpTransaction to get (I can't think of a better way).

So you should add an mTotalReceivedBytes to nsHttpTransaction, and a GetReceivedData() method. (similar to GetConnectStart)
At the end of nsHttpTransaction::WriteSegments add *countWritten to mTotalReceivedBytes.

Add a "readonly attribute int64_t receivedData" to nsIHttpChannel.
Implement it in HttpBaseChannel - add a member mReceivedData, and a GetReceivedData() getter.
In nsHttpChannel::OnStopRequest, do mReceivedData = mTransaction->GetReceivedData() since the transaction is going away.

Now, to make this work for e10s, we need to send this value from the parentProcess to the childProcess.
So add a transferSize fields to ResourceTimingStruct (in TimingStruct.h), then update the Read and Write functions in NeckoMessageUtils.h

Now what's left to do, in HttpChannelParent::OnStopRequest do mChannel->GetReceivedData(&timing.transferSize); and in HttpChannelChild::OnStopRequest do mReceivedData = timing.transferSize. Then in nsPerformance::AddEntry you have access to channel->GetReceivedData

---

decodedBodySize is the trickiest one to do.
You have to add it to nsIHttpChannel, and do all of the things above to make it work for e10s.
To be able to get it, you must change nsHttpCompressConv a bit.
First of all, you should add a new interface - lets say nsICompressConv with a decodedDataLength method.
Make nsHttpCompressConv extend the interface, add an implementation of GetDecodedDataLength() backed by mDecodedDataLenght which you increment by count in do_OnDataAvailable.
Then in nsHttpChannel::OnStopRequest, do
nsCOMPtr<nsICompressConv> conv = do_QueryInterface(mListener) and conv->GetDecodedDataLenght(mDecodedLen)

---

For nextHopProtocol you are already on the right path, and we'll see what we can do with workerStart.
Attached patch resource_timing.diff (obsolete) — Splinter Review
Hi Valentin, channel->GetRequestHeader(name, rv) seems to be returning NS_ERROR_NOT_AVAILABLE (ln 157 of the diff). Do you see an issue with how I am retrieving the encodedBodySize?
Attachment #8638588 - Attachment is obsolete: true
Attachment #8639461 - Flags: feedback?(valentin.gosu)
Comment on attachment 8639461 [details] [diff] [review]
resource_timing.diff

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

You can use channel->GetContentLength for that, instead of parsing the header.

::: dom/base/nsPerformance.cpp
@@ +567,5 @@
> +    if (channel->GetSecurityInfo(getter_AddRefs(securityInfo)) == NS_OK)
> +    {
> +      nsCOMPtr<nsISSLSocketControl> ssl;
> +      nsresult* result = nullptr;
> +      do_QueryInterface(securityInfo, result);

The pattern is:
nsresult rv; // it's important to use this name
ssl = do_QueryInterface(securityInfo, &rv);
Then you can use
NS_SUCCEEDED(rv) // operation succeeded
NS_FAILED(rv) // operation failed
You could also use ssl= do_QueryInterface(securityInfo); if (ssl) ... if you don't really care about the error.

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

@@ +589,5 @@
>      if (initiatorType.IsEmpty()) {
>        initiatorType = NS_LITERAL_STRING("other");
>      }
>      performanceEntry->SetInitiatorType(initiatorType);
>      InsertResourceEntry(performanceEntry);

Put everything you do _before_ this.
Attachment #8639461 - Flags: feedback?(valentin.gosu) → feedback+
(In reply to nhughes from comment #18)
> Created attachment 8639461 [details] [diff] [review]
> resource_timing.diff
> 
> Hi Valentin, channel->GetRequestHeader(name, rv) seems to be returning
> NS_ERROR_NOT_AVAILABLE (ln 157 of the diff). Do you see an issue with how I
> am retrieving the encodedBodySize?

I'm using channel->GetContentLength(&length) now, which assigns a value to length. How can I check the size of a resource *before* it gets decoded? In the mochitest, I want to compare a known encoded resource size with the value of length that I'm getting.
Attached patch resource_timing.diff (obsolete) — Splinter Review
Hey Valentin, I'm getting a linker error since implementing transferSize. It seems the issue is with the HttpBaseChannel implementation of the nsIHttpChannel::GetReceivedData() methods. Could you possibly take a quick look to see if something stands out to you?
Attachment #8639461 - Attachment is obsolete: true
Attachment #8640636 - Flags: feedback?(valentin.gosu)
Comment on attachment 8640636 [details] [diff] [review]
resource_timing.diff

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

Also, you remove a bunch of white space. While that's OK for some reviewers, most prefer that you do it in a separate patch.

The linking problem seems to be caused by the fact that you re-declare the methods in HttpBaseChannel.h. See comments below.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +917,5 @@
>  // HttpBaseChannel::nsIHttpChannel
>  //-----------------------------------------------------------------------------
>  
>  NS_IMETHODIMP
> +HttpBaseChannel::GetReceivedData(int64_t aReceivedData, int64_t *_retval)

You don't need this method. Remove it entirely.

@@ +927,5 @@
> +
> +NS_IMETHODIMP
> +HttpBaseChannel::GetReceivedData(int64_t *aReceivedData)
> +{
> +  aReceivedData = &mReceivedData;

*aReceivedData = mReceivedData;

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +160,5 @@
>    NS_IMETHOD GetResponseStatusText(nsACString& aValue) override;
>    NS_IMETHOD GetRequestSucceeded(bool *aValue) override;
>    NS_IMETHOD RedirectTo(nsIURI *newURI) override;
> +  NS_IMETHOD GetReceivedData(int64_t *aReceivedData) override;
> +  NS_IMETHOD GetReceivedData(int64_t aReceivedData, int64_t *_retval) override;

You don't need to declare these here. They are declared by NS_DECL_NSIHTTPCHANNEL. See nsIHttpChannel.h

::: netwerk/protocol/http/nsHttpTransaction.h
@@ +158,5 @@
>      mozilla::TimeStamp GetRequestStart();
>      mozilla::TimeStamp GetResponseStart();
>      mozilla::TimeStamp GetResponseEnd();
>  
> +    int64_t GetReceivedData() {return mReceivedData;}

Use mTotalReceivedBytes. mReceivedData is used for something else.

@@ +240,5 @@
>      nsCString                       mLineBuf;         // may contain a partial line
>  
>      int64_t                         mContentLength;   // equals -1 if unknown
>      int64_t                         mContentRead;     // count of consumed content bytes
> +    int64_t                         mTotalReceivedBytes;

Add comment to say what this is used for.

::: netwerk/protocol/http/nsIHttpChannel.idl
@@ +84,5 @@
>       */
>      void setReferrerWithPolicy(in nsIURI referrer, in unsigned long referrerPolicy);
>  
> +    readonly attribute int64_t receivedData;
> +    int64_t getReceivedData(in int64_t aReceivedData);

You don't need this method.
When you declare readonly attribute int64_t receivedData; in a .idl, it will automatically generate NS_IMETHOD GetReceivedData(int64_t *aReceivedData) in nsIHttpChannel.h
Attachment #8640636 - Flags: feedback?(valentin.gosu) → feedback+
Attached patch resource_timing.diff (obsolete) — Splinter Review
Clang is now complaining that GetReceivedData is an unimplemented pure virtual method; however it's being implemented in a derived class of nsIHttpChannel, HttpBaseChannel (as you can see in the updated patch). I've spent some time looking over this and can't seem to figure out the issue here.
Attachment #8640636 - Attachment is obsolete: true
Attachment #8640751 - Flags: feedback?(valentin.gosu)
Attached patch resource_timing.diff (obsolete) — Splinter Review
Please ignore the last patch/comment; that patch is outdated.
Attachment #8640751 - Attachment is obsolete: true
Attachment #8640751 - Flags: feedback?(valentin.gosu)
Attachment #8640756 - Flags: feedback?(valentin.gosu)
>+  aReceivedData = &mReceivedData;

That doesn't do anything useful, right?

What you want to do is:

  *aReceivedData = mReceivedData;

and in the caller pass in a pointer to an actual integer on the stack, not a pointer to nowhere.
(In reply to Boris Zbarsky [:bz] from comment #25)
> >+  aReceivedData = &mReceivedData;
> 
> That doesn't do anything useful, right?
> 
> What you want to do is:
> 
>   *aReceivedData = mReceivedData;
> 
> and in the caller pass in a pointer to an actual integer on the stack, not a
> pointer to nowhere.

Got it, that makes sense. I've made that change. Thanks for the feedback :)
Attached patch resource_timing.diff (obsolete) — Splinter Review
There are two unimplemented fields in this patch: workerStart and decodedBodySize. I'm hoping to get some feedback on the other fields in this patch as I start a first iteration on the decodedBodySize implementation :)
Attachment #8640756 - Attachment is obsolete: true
Attachment #8640756 - Flags: feedback?(valentin.gosu)
Attachment #8642620 - Flags: feedback?(valentin.gosu)
Comment on attachment 8642620 [details] [diff] [review]
resource_timing.diff

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

It looks good so far.
I'd also recommend discarding all of the 'whitespace-only' changes.
Attachment #8642620 - Flags: feedback?(valentin.gosu) → feedback+
Attached patch resource_timing.diff (obsolete) — Splinter Review
Attachment #8642620 - Attachment is obsolete: true
Attachment #8644005 - Flags: feedback?(valentin.gosu)
workerStart has been filed as a separate bug, Bug 1191943.
Attached patch resource_timing.diff (obsolete) — Splinter Review
Attachment #8644005 - Attachment is obsolete: true
Attachment #8644005 - Flags: feedback?(valentin.gosu)
Attachment #8644540 - Flags: review?(valentin.gosu)
Comment on attachment 8644540 [details] [diff] [review]
resource_timing.diff

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

This looks good, apart from the things below.
Build and run this on a couple of websites, and check that it works properly.
Also run it on try. There are a few unit tests that might fail.

::: dom/base/PerformanceResourceTiming.h
@@ +144,5 @@
> +  }
> +
> +  unsigned short DecodedBodySize() const
> +  {
> +    return 0; // stubbed

This should return something. Save the value you get from nsICompressConv and return it here.
You need to pass it from HttpChannelParent to HttpChannelChild, similar to transfer size.

::: dom/base/nsPerformance.cpp
@@ +576,5 @@
> +    {
> +      nsresult rv;
> +      nsCOMPtr<nsISSLSocketControl> ssl = do_QueryInterface(securityInfo, &rv);
> +      if (NS_SUCCEEDED(rv) && (NS_SUCCEEDED(ssl->GetNegotiatedNPN(protocol))) &&
> +        (!protocol.Compare("")))

You can use protocol.IsEmpty(). You can also skip these parentheses, and the ones surrounding NS_SUCCEEDED.
Also, instead of usingDefaultProtocol, assign "http/1.1" to protocol. You don't need the extra usingDefaultProtocol and defaultProtocol.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5930,5 @@
>      CleanRedirectCacheChainIfNecessary();
>  
>      ReleaseListeners();
>  
> +    mReceivedData = mTransaction->GetReceivedData();

mTransaction is nulled out somewhere in the middle of :OnStopRequest. Move the lines before that.

@@ +5935,5 @@
> +
> +    nsresult rv;
> +    nsCOMPtr<nsICompressConv> conv = do_QueryInterface(mListener, &rv);
> +    conv->GetDecodedDataLength(&mDecodedBodySize);
> +

Either check the result, or that conv != nullptr.
Also, these values should be set before we call AddEntry()

::: netwerk/protocol/http/nsIHttpChannel.idl
@@ +86,5 @@
>  
> +    readonly attribute int64_t receivedData;
> +
> +    readonly attribute int64_t decodedBodySize;
> +

You need to update the uuid for nsIHttpChannel.

::: netwerk/streamconv/converters/nsHTTPCompressConv.h
@@ +39,5 @@
>          HTTP_COMPRESS_IDENTITY
>      }   CompressMode;
>  
> +class nsHTTPCompressConv	: public nsIStreamConverter	
> +                            , public nsICompressConv

Move both extended classes to the next line. And no trailing whitespace. Like so:
| class nsHttpCompressConv
|   : public nsIStreamConverter
|   , public nsICompressConv
| {
Attachment #8644540 - Flags: review?(valentin.gosu) → feedback+
Attached patch resource_timing.diff (obsolete) — Splinter Review
try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9e045a2bd06
Attachment #8644540 - Attachment is obsolete: true
Attachment #8649580 - Flags: review?(valentin.gosu)
Comment on attachment 8649580 [details] [diff] [review]
resource_timing.diff

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

This looks great! Fix these nits, and have Nick or Patrick review these changes.
Good job!

::: dom/base/PerformanceResourceTiming.h
@@ +186,4 @@
>    nsRefPtr<nsPerformanceTiming> mTiming;
> +  unsigned short mContentLength;
> +  int64_t mTransferSize;
> +  int64_t mDecodedBodySize;

Initialize to 0 / EmptyString() in constructor.

::: dom/base/nsPerformance.cpp
@@ +567,5 @@
>  
> +
> +    nsCOMPtr<nsISupports> securityInfo;
> +    nsAutoCString protocol;
> +    protocol.AssignLiteral("");

This line is unnecessary. protocol is already an empty string.

@@ +574,5 @@
> +    {
> +      nsresult rv;
> +      nsCOMPtr<nsISSLSocketControl> ssl = do_QueryInterface(securityInfo, &rv);
> +      if (NS_FAILED(rv) | (ssl != nullptr && 
> +        NS_FAILED(ssl->GetNegotiatedNPN(protocol))) | protocol.IsEmpty())

use || instead of |
if (ssl && ... ) - no need to write != nullptr
Trailing whitespace

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +74,5 @@
>  protected:
>    virtual ~HttpBaseChannel();
>  
> +  int64_t mReceivedData;
> +  int64_t mDecodedBodySize;

Initialize to 0 in constructor.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5801,5 @@
>  
> +        mReceivedData = mTransaction->GetReceivedData();
> +
> +        nsCOMPtr<nsICompressConv> conv = do_QueryInterface(mListener);
> +        if(conv != nullptr)

if (conv) {
Also, don't forget to leave a space after if

::: netwerk/protocol/http/nsHttpTransaction.h
@@ +240,5 @@
>      nsCString                       mLineBuf;         // may contain a partial line
>  
>      int64_t                         mContentLength;   // equals -1 if unknown
>      int64_t                         mContentRead;     // count of consumed content bytes
> +    int64_t                         mTotalReceivedBytes; // count of received bytes

Initialize to 0 in constructor.

::: netwerk/protocol/http/nsIHttpChannel.idl
@@ +85,5 @@
>      void setReferrerWithPolicy(in nsIURI referrer, in unsigned long referrerPolicy);
>  
> +    readonly attribute int64_t receivedData;
> +
> +    readonly attribute int64_t decodedBodySize;

Add comments saying what these fields are and what they are used for.

::: netwerk/streamconv/converters/nsHTTPCompressConv.cpp
@@ +27,5 @@
>      : mMode(HTTP_COMPRESS_IDENTITY)
>      , mOutBuffer(nullptr)
>      , mInpBuffer(nullptr)
>      , mOutBufferLen(0)
>      , mInpBufferLen(0)

Explicitly initialize mDecodedDataLength to 0 here.

@@ +44,5 @@
>          mFailUncleanStops = false;
>      }
>  }
>  
> +NS_IMETHODIMP nsHTTPCompressConv::GetDecodedDataLength(int64_t *aDecodedDataLength)

put NS_IMETHODIMP on a separate line.
Attachment #8649580 - Flags: review?(valentin.gosu) → feedback+
Attached patch resource_timing.diff (obsolete) — Splinter Review
Attachment #8649580 - Attachment is obsolete: true
Attachment #8650164 - Flags: review?(hurley)
Comment on attachment 8650164 [details] [diff] [review]
resource_timing.diff

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

This is looking pretty good, though some changes need to be made. You'll need a DOM peer to review the bits under dom/

::: dom/base/PerformanceResourceTiming.cpp
@@ +27,5 @@
>                                                       nsPerformance* aPerformance,
>                                                       const nsAString& aName)
>  : PerformanceEntry(aPerformance, aName, NS_LITERAL_STRING("resource")),
> +  mTiming(aPerformanceTiming),
> +  mNextHopProtocol(EmptyString()),

I think this is the default, so you don't have to explicitly initialize it (mInitiatorType isn't explicitly initialized...)

::: dom/base/PerformanceResourceTiming.h
@@ +53,5 @@
>    }
>  
> +  void GetNextHopProtocol(nsAString& aNextHopProtocol) const
> +  {
> +    aNextHopProtocol = mInitiatorType;

Shouldn't this be aNextHopProtocol = mNextHopProtocol?

::: dom/base/nsPerformance.cpp
@@ +573,5 @@
> +    {
> +      nsresult rv;
> +      nsCOMPtr<nsISSLSocketControl> ssl = do_QueryInterface(securityInfo, &rv);
> +      if (NS_FAILED(rv) || (ssl &&
> +        NS_FAILED(ssl->GetNegotiatedNPN(protocol))) || protocol.IsEmpty())

The indentation on this line should make the |NS_FAILED| line up with the |ssl| on the above line. If it does in your editor, then it's likely you've got a tab in there somewhere instead of all spaces.

Even better for readability would be something like this:

if (NS_FAILED(rv) || !ssl ||
    NS_FAILED(ssl->GetNegotiatedNPN(protocol)) || protocol.IsEmpty())

You probably technically only need to check one of NS_FAILED(rv) and !ssl, but it doesn't hurt to be double-sure :)

::: netwerk/protocol/http/NullHttpChannel.cpp
@@ +58,5 @@
>  
>  NS_IMETHODIMP
> +NullHttpChannel::GetReceivedData(int64_t *aReceivedData)
> +{
> +    *aReceivedData = 0;

I think this (and GetDecodedBodySize) should return NS_ERROR_NOT_IMPLEMENTED and not set the argument to anything.

::: netwerk/protocol/http/nsIHttpChannel.idl
@@ +83,5 @@
>       * Set the HTTP referrer URI with a referrer policy.
>       */
>      void setReferrerWithPolicy(in nsIURI referrer, in unsigned long referrerPolicy);
>  
> +    /* size consumed by the response header fields and the response payload body */

These comments you added should follow the doc comment format you see on other attributes in this interface.

::: netwerk/protocol/viewsource/nsViewSourceChannel.cpp
@@ +138,5 @@
>  
>  NS_IMETHODIMP
> +nsViewSourceChannel::GetReceivedData(int64_t *aReceivedData)
> +{
> +    *aReceivedData = 0;

Again, this and GetDecodedBodySize should return NS_ERROR_NOT_IMPLEMENTED

::: netwerk/streamconv/converters/nsICompressConv.idl
@@ +5,5 @@
> +
> +#include "nsISupports.idl"
> +
> +/**
> + * nsICompressConv

I'm not wild about calling this nsICompressConv, as it's not actually a converter. Maybe nsICompressConvStats (or something similar) would be fine? Just so people don't see the name and expect to be able to do any conversions with it :)

@@ +7,5 @@
> +
> +/**
> + * nsICompressConv
> + *
> + * This interface allows for the observation of encoded resource sizes

s/encoded/decoded/
Attachment #8650164 - Flags: review?(hurley) → feedback+
Attached patch resource_timing.diff (obsolete) — Splinter Review
Attachment #8650164 - Attachment is obsolete: true
Attachment #8651144 - Flags: review?(hurley)
Comment on attachment 8651144 [details] [diff] [review]
resource_timing.diff

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

Getting there, though I noticed something that I believe is a pretty big omission (I could just not be understanding things right, though).

Also, are there tests for resource timing anywhere? If so, you should add these elements to the test. If not, we really should add some (though perhaps as a followup).

::: netwerk/protocol/http/TimingStruct.h
@@ +24,5 @@
>    TimeStamp fetchStart;
>    TimeStamp redirectStart;
>    TimeStamp redirectEnd;
> +  int64_t transferSize;
> +  int64_t decodedBodySize;

It's quite possible I'm missing something, but it looks like this is missing, for example, the negotiated protocol and the encodedBodySize (which means those would never be available to the child process, which means that data, though gathered, would never actually be exposed to content, at least in an e10s world).

::: netwerk/protocol/http/nsIHttpChannel.idl
@@ +84,5 @@
>       */
>      void setReferrerWithPolicy(in nsIURI referrer, in unsigned long referrerPolicy);
>  
>      /**
> +     * size consumed by the response header fields and the response payload body 

You have trailing whitespace on this line. Also, please use proper sentence format (capital letter at the beginning, full stop at the end).

@@ +88,5 @@
> +     * size consumed by the response header fields and the response payload body 
> +     */
> +    readonly attribute int64_t receivedData;
> +
> +    /** 

Trailing whitespace here, as well

@@ +89,5 @@
> +     */
> +    readonly attribute int64_t receivedData;
> +
> +    /** 
> +     * size of the message body received by the client, after removing any applied content-codings 

Trailing whitespace and sentence formatting here, as well. Also, limit the line to 80 characters, and wrap appropriately, starting the new line with " * " as well.
Attachment #8651144 - Flags: review?(hurley)
(In reply to Nicholas Hurley [:hurley, :nwgh] from comment #40)
> >    TimeStamp redirectEnd;
> > +  int64_t transferSize;
> > +  int64_t decodedBodySize;
> 
> It's quite possible I'm missing something, but it looks like this is
> missing, for example, the negotiated protocol and the encodedBodySize (which
> means those would never be available to the child process, which means that
> data, though gathered, would never actually be exposed to content, at least
> in an e10s world).

I think encodedBodySize is equivalent to contentLength, and the negociatedProtocol we get from the securityInfo. So we should have both of these, even with an e10s channel child.
I talked to Nathan today and he'll try to get this done over the weekend. It seems very close to landing.
Attached patch resource_timing.diff (obsolete) — Splinter Review
Attachment #8663421 - Flags: review?(valentin.gosu)
Comment on attachment 8663421 [details] [diff] [review]
resource_timing.diff

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

It looks good to me. If Nick agrees, I think we can land this.
Attachment #8663421 - Flags: review?(valentin.gosu)
Attachment #8663421 - Flags: review?(hurley)
Attachment #8663421 - Flags: review+
Also, we may need a DOM peer to review the DOM changes.
Flags: needinfo?(bzbarsky)
(In reply to Valentin Gosu [:valentin] from comment #45)
> Also, we may need a DOM peer to review the DOM changes.

I had contaced a couple of people on DOM about reviewing theses changes, but had no luck getting a response. Do you have someone in mind who would be willing to review?
(In reply to Nathaniel Hughes from comment #46)
> (In reply to Valentin Gosu [:valentin] from comment #45)
> > Also, we may need a DOM peer to review the DOM changes.
> 
> I had contaced a couple of people on DOM about reviewing theses changes, but
> had no luck getting a response. Do you have someone in mind who would be
> willing to review?

I needinfo'd :bz. Hopefully he'll let us know if anything seems off about the patch.
Comment on attachment 8663421 [details] [diff] [review]
resource_timing.diff

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

LGTM, but yes, we need a DOM peer's approval for the bits under dom/ before landing this, as well as a green try run on this latest version of the patch.
Attachment #8663421 - Flags: review?(hurley) → review+
Comment on attachment 8663421 [details] [diff] [review]
resource_timing.diff

I you're asking me to review, just ask me to review?  ;)

>+++ b/dom/base/PerformanceResourceTiming.h
>+  void SetContentLength(const unsigned short& aContentLength)

That should be:

  void setContentLength(unsigned short aContentLength)

without the random reference tossed in.

>+  void GetContentLength(unsigned short& aContentLength) const

This seems to be unused.  But even if we keep it, it should be called "ContentLength" and return the value as a return value, instead of using a reference outparam....

Also, "short" here is nonsense; more on this below.

>+  void SetTransferSize(const int64_t& aTransferSize)

Again, just use a value arg type, not a const reference.

>+  void GetTransferSize(int64_t& aTransferSize) const

This should just not exist, since we have a perfectly good TransferSize() getter already.

>+  void SetDecodedBodySize(const int64_t& aDecodedBodySize)
>+  void GetDecodedBodySize(int64_t& aDecodedBodySize) const

As above.

>+++ b/dom/webidl/PerformanceResourceTiming.webidl
>+  readonly attribute unsigned short transferSize;
>+  readonly attribute unsigned short encodedBodySize;
>+  readonly attribute unsigned short decodedBodySize;

I know that's what the spec says, but it's complete nonsense.  I hear tell sometimes some things that are bigger than 65KB get transferred over the network on some websites.  ;)

Please raise a spec issue, make all these unsigned long, and use uint64_t or int64_t for them internally (you already do for some of them but not others).

I see nothing here, or in the spec, that would prevent this information from appearing in the performance timing for cross-origin requests.  Leaking size information like this for cross-origin requests is not ok, I'm pretty sure.  Again, please raise a spec issue and make sure that our implementation doesn't leak such information.  Add tests for that, please.

Speaking of which, this needs tests in general.
Flags: needinfo?(bzbarsky)
Attachment #8663421 - Flags: review-
(In reply to Boris Zbarsky [:bz] from comment #49)
> Comment on attachment 8663421 [details] [diff] [review]
> resource_timing.diff
> 
> I you're asking me to review, just ask me to review?  ;)
> 
> >+++ b/dom/base/PerformanceResourceTiming.h
> >+  void SetContentLength(const unsigned short& aContentLength)
> 
> That should be:
> 
>   void setContentLength(unsigned short aContentLength)
> 
> without the random reference tossed in.
> 
> >+  void GetContentLength(unsigned short& aContentLength) const
> 
> This seems to be unused.  But even if we keep it, it should be called
> "ContentLength" and return the value as a return value, instead of using a
> reference outparam....
> 
> Also, "short" here is nonsense; more on this below.
> 
> >+  void SetTransferSize(const int64_t& aTransferSize)
> 
> Again, just use a value arg type, not a const reference.
> 
> >+  void GetTransferSize(int64_t& aTransferSize) const
> 
> This should just not exist, since we have a perfectly good TransferSize()
> getter already.
> 
> >+  void SetDecodedBodySize(const int64_t& aDecodedBodySize)
> >+  void GetDecodedBodySize(int64_t& aDecodedBodySize) const
> 
> As above.
> 
> >+++ b/dom/webidl/PerformanceResourceTiming.webidl
> >+  readonly attribute unsigned short transferSize;
> >+  readonly attribute unsigned short encodedBodySize;
> >+  readonly attribute unsigned short decodedBodySize;
> 
> I know that's what the spec says, but it's complete nonsense.  I hear tell
> sometimes some things that are bigger than 65KB get transferred over the
> network on some websites.  ;)
> 
> Please raise a spec issue, make all these unsigned long, and use uint64_t or
> int64_t for them internally (you already do for some of them but not others).
> 
> I see nothing here, or in the spec, that would prevent this information from
> appearing in the performance timing for cross-origin requests.  Leaking size
> information like this for cross-origin requests is not ok, I'm pretty sure. 
> Again, please raise a spec issue and make sure that our implementation
> doesn't leak such information.  Add tests for that, please.
> 
> Speaking of which, this needs tests in general.

Thanks, Boris. 

Could someone go into some more detail about how I should implement testing? Or direct me to some documentation I could look at?

I had been told that the way to go about testing this patch would be to simply use the Web Console on different sites, verifying that these new fields contained expected results.
I don't know whether web-platform-tests gives enough control over headers and compression to test this.  If it does, then that's the way to go; James may have pointers.

If it does not, mochitest certainly does.  See https://developer.mozilla.org/en-US/docs/Mochitest and in particular https://developer.mozilla.org/en-US/docs/Mochitest#How_do_I_change_the_HTTP_headers_or_status_sent_with_a_file_used_in_a_Mochitest.3F which should allow you to set headers for some gzipped data that cause the browser to gunzip it on the fly, and then you can see whether the numbers came out right.  Mochitest also supports doing cross-origin requests (see <https://developer.mozilla.org/en-US/docs/Mochitest#SSL> though there are also various non-SSL origins available) so you cn test that part of it.
Flags: needinfo?(james)
Yes, web-platform-tests offer full control over the response. You can write python handlers that deliver arbitrary bytes on the wires. See [1] for some documentation and [2] for an example of a handler that returns gzip data. For cross-origin requests you have several ports and subdomains available, see [3] for details. https is also supported; if the test itself is to be served over https (rather than any subresources) it must be named filename.https.ext

Please ask if you have specific questions.

[1] http://wptserve.readthedocs.org/en/latest/handlers.html#python-handlers
[2] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/XMLHttpRequest/resources/gzip.py
[3] http://testthewebforward.org/docs/test-format-guidelines.html#tests-involving-multiple-origins
Flags: needinfo?(james)
richard - should this be an https only feature (exposed to web content)?
Flags: needinfo?(rlb)
(In reply to Boris Zbarsky [:bz] from comment #49)
> 
> >+++ b/dom/webidl/PerformanceResourceTiming.webidl
> >+  readonly attribute unsigned short transferSize;
> >+  readonly attribute unsigned short encodedBodySize;
> >+  readonly attribute unsigned short decodedBodySize;
> 
> I know that's what the spec says, but it's complete nonsense.  I hear tell
> sometimes some things that are bigger than 65KB get transferred over the
> network on some websites.  ;)
> 
> Please raise a spec issue, make all these unsigned long, and use uint64_t or
> int64_t for them internally (you already do for some of them but not others).
> 
> I see nothing here, or in the spec, that would prevent this information from
> appearing in the performance timing for cross-origin requests.  Leaking size
> information like this for cross-origin requests is not ok, I'm pretty sure. 

PTAL: https://github.com/w3c/resource-timing/pull/33.
Assignee: nhughes → valentin.gosu
This patch addresses review comments and fixes conflicts with m-c.
It also makes the user agent conform to the behaviour established in https://github.com/w3c/resource-timing/issues/40
Tests for the feature will be submitted in a separate patch.
Attachment #8678326 - Flags: review?(mcmanus)
Attachment #8678326 - Flags: review?(bzbarsky)
Attachment #8678326 - Flags: review?(mcmanus) → review?(hurley)
Is this last patch against m-c or on top of the last patch I reviewed here?
Flags: needinfo?(valentin.gosu)
Attached patch Add Resource Timing Fields (obsolete) — Splinter Review
This is the full patch
Attachment #8651144 - Attachment is obsolete: true
Attachment #8663421 - Attachment is obsolete: true
(In reply to Boris Zbarsky [:bz] from comment #56)
> Is this last patch against m-c or on top of the last patch I reviewed here?
It was on top of the last patch. I've also just uploaded the full version.
Thanks!
Flags: needinfo?(valentin.gosu)
Thanks.  I should be able to look at this on Monday.
I submitted a pull request adding web-platform-tests for these fields:
https://github.com/w3c/web-platform-tests/pull/2282
The xmlhttprequest request now returns a gzip response, in order to check that decodedBodySize also works correctly.
Comment on attachment 8678408 [details] [diff] [review]
Add Resource Timing Fields

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

This looks pretty good, though I have a bit of concern around the "ALPN identifiers" - see comment inline below.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1760,5 @@
> +  nsresult rv;
> +  nsCOMPtr<nsISSLSocketControl> ssl = do_QueryInterface(mSecurityInfo, &rv);
> +  nsAutoCString protocol;
> +  if (NS_SUCCEEDED(rv) && ssl &&
> +    NS_SUCCEEDED(ssl->GetNegotiatedNPN(protocol)) &&

nit: indentation on the continuation lines here is incorrect

@@ +1762,5 @@
> +  nsAutoCString protocol;
> +  if (NS_SUCCEEDED(rv) && ssl &&
> +    NS_SUCCEEDED(ssl->GetNegotiatedNPN(protocol)) &&
> +    !protocol.IsEmpty()) {
> +    // The negociated protocol was not empty so we can use it.

s/negociated/negotiated/

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +1172,5 @@
>    mChannel->GetAsyncOpen(&timing.fetchStart);
>    mChannel->GetRedirectStart(&timing.redirectStart);
>    mChannel->GetRedirectEnd(&timing.redirectEnd);
> +  mChannel->GetTransferSize(&timing.transferSize);
> +  mChannel->GetEncodedBodySize(&timing.encodedBodySize);

Am I right in assuming that we don't have a line for decoded body size here because we have that information available directly on the child? (Might be good to call it out in a comment, if that's the case.)

::: netwerk/protocol/http/nsHttp.cpp
@@ +243,5 @@
> +        return "http/1.0";
> +    case NS_HTTP_VERSION_1_1:
> +        return "http/1.1";
> +    case NS_HTTP_VERSION_2_0:
> +        return "http/2.0";

This should be the same as HTTP_VERSION_2 above ("h2" is the appropriate ALPN identifier).

I'm also torn on ever returning anything other than h2, spdy, or http/1.1 from this - strictly speaking, the ALPN registry doesn't have anything in it from this set other than the 3 I listed above. Then again, according to telemetry, http/1.0 is STILL 1.5% of all our responses (http/0.9 does not show up on telemetry).

I think, in the interest of both real-world flexibility *and* an attempt to adhere to the spec, we should return h2, spdy, http/1.1, or http/1.0, and default to http/1.1 (instead of "unknown").
Attachment #8678408 - Flags: feedback+
Attachment #8678326 - Flags: review?(hurley)
Comment on attachment 8678326 [details] [diff] [review]
imported patch -resource-timing-diff.patch

>+  void SetEncodedBodySize(const uint64_t aEncodedBodySize)
>+  void SetTransferSize(const int64_t aTransferSize)
>+  void SetDecodedBodySize(const int64_t aDecodedBodySize)

No need for the const in any of those, since you're passing by value anyway, right?

>+    // The negociated protocol was not empty so we can use it.

"negotiated"

>+  nsCOMPtr<nsIStreamListener>       mCompressListener;

You need to make sure this is nulled out anywhere mListener is nulled out.  Otherwise, chances are you will leak.

>-    readonly attribute int64_t receivedData;

I see uses in our tree that you're not fixing in this patch.  What's the story with that?

r=me with the nulling out fixed and either receivedData restored or its consumers fixed.
Attachment #8678326 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #62)
> Comment on attachment 8678326 [details] [diff] [review]
> >-    readonly attribute int64_t receivedData;
> 
> I see uses in our tree that you're not fixing in this patch.  What's the
> story with that?
> 
> r=me with the nulling out fixed and either receivedData restored or its
> consumers fixed.

receivedData was added to nsIHttpChannel.idl in the first patch, then renamed to transferSize in the interdiff. Indeed, the fact that it's close to nsHttpTransaction::mReceivedData made it quite confusing. 

Thanks for the review.
Ah, I guess the channel.receivedData in browser_LoopRooms_channel.js is some other beastie...
Attachment #8678326 - Attachment is obsolete: true
Attachment #8679724 - Flags: review?(hurley)
Attachment #8678408 - Attachment is obsolete: true
Attachment #8679724 - Flags: review?(hurley) → review+
https://hg.mozilla.org/mozilla-central/rev/54a4387f2db9
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
Sorry I'm so late to this.  I see that it's in Aurora currently; is it too late to discuss whether it's restricted to secure origins?

ISTM that the restriction would be appropriate.  At a coarse level, it's a new feature, which pretty much means that it should be HTTPS-only.

Even if these attributes aren't restricted to secure origins, we should expose a pref to disable them.  Since they provide more bits for fingerprinting, I expect folks like Tor will want to disable them.

If it doesn't make sense to restrict just these attributes, would it make sense to ask about restricting the resource timing API overall?
Flags: needinfo?(rlb) → needinfo?(mcmanus)
(In reply to Richard Barnes [:rbarnes] from comment #72)
> Even if these attributes aren't restricted to secure origins, we should
> expose a pref to disable them.  Since they provide more bits for
> fingerprinting, I expect folks like Tor will want to disable them.
> 

I suspect TOR would prefer to turn off resource timing completely. The pref dom.enable_resource_timing is available for that. When set to false, performance.getEntries() will simply return an empty array.

However, for unsecured connections these attributes don't reveal any other sensitive info than you would get from inspecting the network requests, so I don't know if it's worth restricting _just_ the new attributes.
this ship has sailed. let's catch the next one.
Flags: needinfo?(mcmanus)
You need to log in before you can comment on or make changes to this bug.