Implement PerformanceServerTiming interface

RESOLVED FIXED in Firefox 61

Status

()

P2
normal
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: kershaw, Assigned: valentin)

Tracking

(Depends on: 1 bug, {dev-doc-complete})

Trunk
mozilla61
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 wontfix, firefox60 wontfix, firefox61 fixed)

Details

Attachments

(10 attachments, 3 obsolete attachments)

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
(Reporter)

Updated

a year ago
Assignee: nobody → kechang
(Reporter)

Comment 1

a year ago
Created attachment 8941012 [details] [diff] [review]
Part1: Implement PerformanceServerTiming

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)
(Reporter)

Comment 2

a year ago
Created attachment 8941015 [details] [diff] [review]
Part2: Test

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+
(Reporter)

Comment 4

a year ago
Created attachment 8941363 [details] [diff] [review]
Part3: Add PerformanceServerTiming to test_interface.js

Please take a look. Thanks.
Attachment #8941363 - Flags: review?(amarchesini)
Attachment #8941363 - Flags: review?(amarchesini) → review+
(Reporter)

Comment 6

a year ago
Created attachment 8941388 [details] [diff] [review]
Part1: Implement PerformanceServerTiming, r=baku

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+
(Reporter)

Comment 7

a year ago
Created attachment 8941389 [details] [diff] [review]
Part2: Test, r=baku

Carry r+.
Attachment #8941389 - Flags: review+
(Reporter)

Comment 8

a year ago
Created attachment 8941390 [details] [diff] [review]
Part3: Add PerformanceServerTiming to test_interface.js, r=baku

Carry r+.
Attachment #8941390 - Flags: review+
(Reporter)

Updated

a year ago
Keywords: checkin-needed
I'm going to fold Part 3 into Part 1 so bisection doesn't become a problem.

Comment 10

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aca79713b901
Part 1: Implement PerformanceServerTiming. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7667f5a63af
Part 2: Test case. r=baku
Keywords: checkin-needed

Updated

a year ago
Blocks: 1424341

Updated

a year ago
No longer blocks: 1424341
(Reporter)

Comment 12

a year ago
(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)
(Assignee)

Comment 13

a year ago
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)
(Reporter)

Comment 14

a year ago
Created attachment 8942132 [details] [diff] [review]
Create doc entry from http channel

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)
(Assignee)

Comment 15

a year ago
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+
Keywords: dev-doc-needed
(Reporter)

Comment 16

a year ago
(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
(Reporter)

Comment 17

a year ago
(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.
(Assignee)

Comment 18

a year ago
(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)

Updated

10 months ago
Assignee: nobody → valentin.gosu
Flags: needinfo?(valentin.gosu)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 24

10 months ago
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 25

10 months ago
mozreview-review
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 26

10 months ago
mozreview-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 27

10 months ago
mozreview-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 28

10 months ago
mozreview-review
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 29

10 months ago
mozreview-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+
(Assignee)

Comment 30

10 months ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 41

10 months ago
mozreview-review
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 42

10 months ago
mozreview-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 43

10 months ago
mozreview-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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 50

9 months ago
Valentin, any word here?
Flags: needinfo?(valentin.gosu)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 57

9 months ago
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)
Comment hidden (mozreview-request)

Comment 60

9 months ago
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
(Assignee)

Updated

8 months ago
Depends on: 1460226
(Assignee)

Updated

8 months ago
Depends on: 1460590
(Assignee)

Updated

8 months ago
Depends on: 1460594
status-firefox59: affected → wontfix
status-firefox60: --- → wontfix
You need to log in before you can comment on or make changes to this bug.