Closed Bug 1423495 Opened 6 years ago Closed 6 years ago

Implement PerformanceServerTiming interface

Categories

(Core :: DOM: Core & HTML, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: kershaw, Assigned: valentin)

References

Details

(Keywords: dev-doc-complete)

Attachments

(10 files, 3 obsolete files)

14.90 KB, patch
kershaw
: review+
Details | Diff | Splinter Review
5.02 KB, patch
kershaw
: review+
Details | Diff | Splinter Review
3.92 KB, patch
kershaw
: review+
baku
: review+
Details | Diff | Splinter Review
11.62 KB, patch
valentin
: review+
Details | Diff | Splinter Review
59 bytes, text/x-review-board-request
baku
: review+
Details
59 bytes, text/x-review-board-request
baku
: review+
Details
59 bytes, text/x-review-board-request
baku
: review+
Details
59 bytes, text/x-review-board-request
baku
: review+
Details
59 bytes, text/x-review-board-request
baku
: review+
Details
59 bytes, text/x-review-board-request
u408661
: review+
Details
Depends on: 1413999
Priority: -- → P2
Assignee: nobody → kechang
Summary:
 - Add PerformanceServerTiming.webidl.
 - We already have nsIServerTiming interface defined in nsITimedChannel.idl, so this patch basically convert nsIServerTiming to PerformanceServerTiming.

Please take a look. Thanks.
Attachment #8941012 - Flags: review?(amarchesini)
Attached patch Part2: Test (obsolete) — Splinter Review
Test steps:
1. Create a XHR to get serverTiming.sjs.
2. In serverTiming.sjs, add server-timing data to both response and trailer headers.
3. Check if we can get the data from PerformanceResourceTiming and if the data is correct.

Thanks.
Attachment #8941015 - Flags: review?(amarchesini)
Comment on attachment 8941012 [details] [diff] [review]
Part1: Implement PerformanceServerTiming

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

::: dom/performance/PerformanceResourceTiming.cpp
@@ +121,5 @@
> +    return;
> +  }
> +
> +  uint32_t length = 0;
> +  serverTimingArray->GetLength(&length);

This can fail.

::: dom/performance/PerformanceServerTiming.h
@@ +20,5 @@
> +class PerformanceServerTiming final : public nsISupports,
> +                                      public nsWrapperCache
> +{
> +public:
> +  explicit PerformanceServerTiming(nsISupports* aParent,

no explicit with more than 1 param.

@@ +31,5 @@
> +
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(PerformanceServerTiming)
> +
> +  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;

1. 80chars max.
2. no virtual for final classes
Attachment #8941012 - Flags: review?(amarchesini) → review+
Attachment #8941015 - Flags: review?(amarchesini) → review+
Please take a look. Thanks.
Attachment #8941363 - Flags: review?(amarchesini)
Attachment #8941363 - Flags: review?(amarchesini) → review+
Rebase and carry r+.

Thanks for the prompt review!
Attachment #8941012 - Attachment is obsolete: true
Attachment #8941015 - Attachment is obsolete: true
Attachment #8941363 - Attachment is obsolete: true
Attachment #8941388 - Flags: review+
Carry r+.
Attachment #8941389 - Flags: review+
Keywords: checkin-needed
I'm going to fold Part 3 into Part 1 so bisection doesn't become a problem.
Blocks: 1424341
No longer blocks: 1424341
(In reply to Cristina Coroiu [:ccoroiu] from comment #11)
> Backed out 2 changesets (bug 1423495) for wpt failures at
> /server-timing/test_server_timing.html r=backout on a CLOSED TREE 
> 
> Failure push:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=f7667f5a63af3664632c9c175ebb2b07b65bda89
> 
> Failure log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=155369745&repo=mozilla-
> inbound&lineNumber=6699
> 

The failure is about not getting server timing entry from the load of test_server_timing.html [1].
However, according to [2], we don't report resource timing entry for document load and server timing is a part of resource timing.
The spec [3] says that "All resources fetched by the current browsing [HTML5] or worker [WORKERS] context's MUST be included as PerformanceResourceTiming objects." It seems that our implementation about resource timing entry is correct.

Valentin, since you wrote the code in [2], could you confirm that we really don't have to report resource timing entry for document load?

Thanks.


[1] https://searchfox.org/mozilla-central/rev/88439dc6c5b02e167032374071cdc697a056efa1/testing/web-platform/tests/server-timing/test_server_timing.html.sub.headers
[2] https://searchfox.org/mozilla-central/rev/88439dc6c5b02e167032374071cdc697a056efa1/netwerk/protocol/http/HttpBaseChannel.cpp#4168-4170
[3] https://w3c.github.io/resource-timing/#resources-included
Flags: needinfo?(kechang) → needinfo?(valentin.gosu)
The document load does not get reported as a PerformanceResourceTiming entry, but as a PerformanceNavigationTiming entry (name="document", entryType: "navigation" - it extends PerformanceResourceTiming). It is the first entry in performance.getEntries().

The entry for the document is constructed here: https://searchfox.org/mozilla-central/rev/88439dc6c5b02e167032374071cdc697a056efa1/dom/performance/PerformanceMainThread.cpp#325
Flags: needinfo?(valentin.gosu)
Why for this change:
I found in test_server_timing.html [1], the test expects to get server timing data from PerformanceNavigationTiming. But, our current code creates PerformanceNavigationTiming quite late until JS code wants to access it. So, we have to create the document entry eariler if we found server timing headers.

[1] https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/testing/web-platform/tests/server-timing/test_server_timing.html#13

Thanks.
Attachment #8942132 - Flags: review?(valentin.gosu)
Comment on attachment 8942132 [details] [diff] [review]
Create doc entry from http channel

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

r=me provided that this passes all unit tests
Also make sure to run attachment 8932225 [details] [diff] [review] locally.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +4170,4 @@
>      return nullptr;
>    }
>  
> +  nsCOMPtr<nsIDocument> loadingDocument = innerWindow->GetDoc();

Are you sure this is the same as loadingDocument?
Attachment #8942132 - Flags: review?(valentin.gosu) → review+
(In reply to Valentin Gosu [:valentin] from comment #15)
> Comment on attachment 8942132 [details] [diff] [review]
> Create doc entry from http channel
> 
> Review of attachment 8942132 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me provided that this passes all unit tests
> Also make sure to run attachment 8932225 [details] [diff] [review] locally.
> 
> ::: netwerk/protocol/http/HttpBaseChannel.cpp
> @@ +4170,4 @@
> >      return nullptr;
> >    }
> >  
> > +  nsCOMPtr<nsIDocument> loadingDocument = innerWindow->GetDoc();
> 
> Are you sure this is the same as loadingDocument?
Assignee: kershaw → nobody
(In reply to Valentin Gosu [:valentin] from comment #15)
> Comment on attachment 8942132 [details] [diff] [review]
> Create doc entry from http channel
> 
> Review of attachment 8942132 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me provided that this passes all unit tests
> Also make sure to run attachment 8932225 [details] [diff] [review] locally.
> 
> ::: netwerk/protocol/http/HttpBaseChannel.cpp
> @@ +4170,4 @@
> >      return nullptr;
> >    }
> >  
> > +  nsCOMPtr<nsIDocument> loadingDocument = innerWindow->GetDoc();
> 
> Are you sure this is the same as loadingDocument?

Yes, this is not the same as loadingDocument. But, this seems to be the only way to pass test_server_timing.html.

It's sad that I am not able to look it deeper for now.
(In reply to Kershaw Chang [:kershaw] from comment #17)
> (In reply to Valentin Gosu [:valentin] from comment #15)
> > Are you sure this is the same as loadingDocument?
> 
> Yes, this is not the same as loadingDocument. But, this seems to be the only
> way to pass test_server_timing.html.

I'll take a look at it in the next few days. Thanks a lot for working on this!

> It's sad that I am not able to look it deeper for now.

:(
Flags: needinfo?(valentin.gosu)
Assignee: nobody → valentin.gosu
Flags: needinfo?(valentin.gosu)
The main changes involve:
- rebased the patches on the latest m-c
- made the test run under HTTPS (since we restricted server timing support to secure origins in bug 1436517)
- Made sure to check !TimingAllowed() in PerformanceTimingData::GetServerTiming
- added a test to make sure we return an empty list for .serverTiming under plain-text HTTP
- Added WPT meta info for expected failures (the tests run under plain-text HTTP at the moment).
- The WPT tests reveal no big issues when run manually with HTTPS: https://w3c-test.org/server-timing/
- Fixed a few typos or otherwise minor issues.
Comment on attachment 8962919 [details]
Bug 1423495 - Part1: Implement PerformanceServerTiming,

https://reviewboard.mozilla.org/r/231726/#review237408

::: dom/performance/PerformanceResourceTiming.cpp:97
(Diff revision 1)
> +  if (!serverTimingArray) {
> +    return;
> +  }
> +
> +  uint32_t length = 0;
> +  if (NS_FAILED(serverTimingArray->GetLength(&length))) {

NS_WARN_IF

::: dom/performance/PerformanceServerTiming.cpp:34
(Diff revision 1)
> +  return mozilla::dom::PerformanceServerTimingBinding::Wrap(aCx, this, aGivenProto);
> +}
> +
> +void
> +PerformanceServerTiming::GetName(nsAString& aName) const
> +{

aName.Truncate()

::: dom/performance/PerformanceServerTiming.cpp:40
(Diff revision 1)
> +  if (!mServerTiming) {
> +    return;
> +  }
> +
> +  nsAutoCString name;
> +  Unused << mServerTiming->GetName(name);

I would do:

if (NS_WARN_IF(NS_FAILED(...)) {
  return;
}

aName.Assign(...)

::: dom/performance/PerformanceServerTiming.cpp:52
(Diff revision 1)
> +  if (!mServerTiming) {
> +    return 0;
> +  }
> +
> +  double duration = 0;
> +  Unused << mServerTiming->GetDuration(&duration);

same here

::: dom/performance/PerformanceServerTiming.cpp:64
(Diff revision 1)
> +  if (!mServerTiming) {
> +    return;
> +  }
> +
> +  nsAutoCString description;
> +  Unused << mServerTiming->GetDescription(description);

and here
Attachment #8962919 - Flags: review?(amarchesini) → review+
Comment on attachment 8962920 [details]
Bug 1423495 - Part2: Test case,

https://reviewboard.mozilla.org/r/231728/#review237410

::: dom/performance/tests/serverTiming.sjs:2
(Diff revision 1)
> +
> +var respnseServerTiming = [{metric:"metric1", duration:"123.4", description:"description1"},

responseServerTiming

::: dom/performance/tests/serverTiming.sjs:24
(Diff revision 1)
> +  var body = "c\r\ndata reached\r\n3\r\nhej\r\n0\r\n";
> +
> +  response.seizePower();
> +  response.write("HTTP/1.1 200 OK\r\n");
> +  response.write("Content-Type: text/plain\r\n");
> +  response.write(createServerTimingHeader(respnseServerTiming));

typo
Attachment #8962920 - Flags: review?(amarchesini) → review+
Comment on attachment 8962921 [details]
Bug 1423495 - Part3: Add PerformanceServerTiming to test_interface.js,

https://reviewboard.mozilla.org/r/231730/#review237412

::: dom/tests/mochitest/general/test_interfaces.js:775
(Diff revision 1)
>      {name: "PerformanceObserverEntryList", insecureContext: true},
>  // IMPORTANT: Do not change this list without review from a DOM peer!
>      {name: "PerformanceResourceTiming", insecureContext: true},
>  // IMPORTANT: Do not change this list without review from a DOM peer!
> +    {name: "PerformanceServerTiming", insecureContext: true},
> +// IMPORTANT: Do not change this list without review from a DOM peer!

This interface is exposed to worker as well. You should touch dom/workers/test/*interfaces.js and the same file in dom/serviceworkers/test
Attachment #8962921 - Flags: review?(amarchesini)
Comment on attachment 8962922 [details]
Bug 1423495 - Part4: Create doc entry form http channel if server timing headers are found for a document load

https://reviewboard.mozilla.org/r/231732/#review237414
Attachment #8962922 - Flags: review?(amarchesini) → review+
Comment on attachment 8962923 [details]
Bug 1423495 - Part5: Fix WPT metadata to expect failures

https://reviewboard.mozilla.org/r/231734/#review237416

::: testing/web-platform/meta/server-timing/test_server_timing.html.ini:3
(Diff revision 1)
>  [test_server_timing.html]
>    [Untitled]
>      expected: FAIL

What else is missing to make these test pass?
Attachment #8962923 - Flags: review?(amarchesini) → review+
(In reply to Andrea Marchesini [:baku] from comment #29)
> Comment on attachment 8962923 [details]
> Bug 1423495 - Part5: Fix WPT metadata to expect failures
> >  [test_server_timing.html]
> >    [Untitled]
> >      expected: FAIL
> 
> What else is missing to make these test pass?

As it stands these tests can't pass. I filed a bug on the spec https://github.com/w3c/server-timing/issues/54
I intend to change the tests in a followup bug.
Comment on attachment 8964893 [details]
Bug 1423495 - Part6: Use threadsafe refcounting for nsServerTiming

https://reviewboard.mozilla.org/r/233620/#review239254

I guess it's because necko passes this interface around?
Attachment #8964893 - Flags: review+
Comment on attachment 8964893 [details]
Bug 1423495 - Part6: Use threadsafe refcounting for nsServerTiming

https://reviewboard.mozilla.org/r/233620/#review239424
Attachment #8964893 - Flags: review?(hurley) → review+
Comment on attachment 8962921 [details]
Bug 1423495 - Part3: Add PerformanceServerTiming to test_interface.js,

https://reviewboard.mozilla.org/r/231730/#review239530
Attachment #8962921 - Flags: review?(amarchesini) → review+
Valentin, any word here?
Flags: needinfo?(valentin.gosu)
I have all the tests passing now.
Andrea, do you mind taking another look at Part 3?
I also had to add the interface to test_worker_interfaces.js

Most of the other changes are in attachment 8964893 [details] - I ended up keeping readonly attribute nsIArray serverTiming - we need it in the xpcshell tests.
Flags: needinfo?(valentin.gosu) → needinfo?(amarchesini)
Attachment #8941390 - Flags: review+
All the 2 patches 3 are OK.
Flags: needinfo?(amarchesini)
Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ecf85551434a
Part1: Implement PerformanceServerTiming, r=baku
https://hg.mozilla.org/integration/autoland/rev/ca970fc93a32
Part2: Test case, r=baku
https://hg.mozilla.org/integration/autoland/rev/d06281128204
Part3: Add PerformanceServerTiming to test_interface.js, r=baku
https://hg.mozilla.org/integration/autoland/rev/11804549931f
Part4: Create doc entry form http channel if server timing headers are found for a document load r=baku
https://hg.mozilla.org/integration/autoland/rev/26e3cf345802
Part5: Fix WPT metadata to expect failures r=baku
https://hg.mozilla.org/integration/autoland/rev/fa8202661c16
Part6: Use threadsafe refcounting for nsServerTiming r=baku,nwgh
Depends on: 1457401
Depends on: 1460226
Depends on: 1460590
Depends on: 1460594
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.