Closed Bug 1425458 Opened 6 years ago Closed 6 years ago

Resource timing not available in web worker

Categories

(Core :: DOM: Workers, defect, P2)

58 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: sajal83, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(9 files, 9 obsolete files)

13.17 KB, patch
smaug
: review+
Details | Diff | Splinter Review
55.46 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.47 KB, patch
smaug
: review+
Details | Diff | Splinter Review
943 bytes, patch
smaug
: review+
Details | Diff | Splinter Review
5.90 KB, patch
smaug
: review+
Details | Diff | Splinter Review
90.05 KB, patch
smaug
: review+
Details | Diff | Splinter Review
24.19 KB, patch
smaug
: review+
Details | Diff | Splinter Review
6.30 KB, patch
Details | Diff | Splinter Review
1.41 KB, patch
smaug
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.84 Safari/537.36

Steps to reproduce:

open console in developer tools
visit: https://db.sajalkayan.com/ff-worker.html



Actual results:

performance API did not have data about the fetched resource.


Expected results:

performance object should have gotten an entry of type resource.

When running the worker function on main thread, it can get the resource timing object.
Component: Untriaged → DOM: Workers
Product: Firefox → Core
qDot, please take a look after holidays.
Flags: needinfo?(kyle)
Priority: -- → P3
I forgot that Valentin had worked on this, too.

https://w3c.github.io/resource-timing/#idl-def-performanceresourcetiming says it should be exposed to workers.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(valentin.gosu)
I don't have a lot of insight into this. From what I can tell, it is exposed to workers, but the resources don't get added there.
Andrea, any idea what might be happening here?
Flags: needinfo?(valentin.gosu) → needinfo?(amarchesini)
> Andrea, any idea what might be happening here?

Performance doesn't have entries in workers because PerformanceWorker::AddEntry is never called and this happens because AddEntry() happens on main-thread. Currently nsHttpChannel takes the Performance object from the loading document, but that doesn't exist in case of workers. 

We should see if there is a easy and clean way to attach PerformanceWorker to nsIHttpChannel.
Flags: needinfo?(amarchesini)
qdot, are you going to work on this?
If I'm supposed to, I have no idea where to start. I haven't worked with Workers or this portion of Performance at all. I think I just got tagged because I vaguely touched Performance at one point.
Flags: needinfo?(kyle)
Priority: P3 → P2
Assignee: nobody → amarchesini
:Baku, want to take a look at this?
Flags: needinfo?(amarchesini)
Attached patch part 0 - NS_NewChannel (obsolete) — Splinter Review
In this first patch I add the support for a PerformanceStorage object in NS_NewChannel methods. It's null everywhere except in ScriptLoader, fetch and XHR.

PerformanceStorage is a class that will be implemented in the following patches.
Flags: needinfo?(amarchesini)
Attachment #8944442 - Flags: review?(bugs)
On main-thread PerformanceMainThread implements PerformanceStorage.
Attachment #8944443 - Flags: review?(bugs)
In order to move PerformanceEntries from the main-thread to workers, I need a thread-safe class. Here I introduce a PerformanceTimingData where everything is stored. It's mainly moving code from PerformanceTiming to this new class.
Attachment #8944444 - Flags: review?(bugs)
Here the PerformanceStorageWorker that sends PerformanceTimingData from the main-thread to the worker thread into the Performance object.
Attachment #8944445 - Flags: review?(bugs)
A partial interface must be exposed to workers.
Attachment #8944446 - Flags: review?(bugs)
resourcetimingbufferfull must be dispatched differently on workers because we use ControlRunnables and we don't want to execute JS code from one of them. Otherwise we will be able to run code during a sync event loop.
Attachment #8944447 - Flags: review?(bugs)
Attached patch part 6 - tests (obsolete) — Splinter Review
Attachment #8944448 - Flags: review?(bugs)
Attachment #8944447 - Attachment is obsolete: true
Attachment #8944447 - Flags: review?(bugs)
Attachment #8944456 - Flags: review?(bugs)
Comment on attachment 8944442 [details] [diff] [review]
part 0 - NS_NewChannel

Also other NS_NewChannel* variants need the new param. Otherwise it is pretty much guaranteed that it won't be used properly in all the cases.
Or perhaps the variants of the method which also take nsINode* as a param can be guaranteed to be mainthread only so they don't need the new param?
Attachment #8944442 - Flags: review?(bugs) → review-
Attachment #8944443 - Flags: review?(bugs) → review+
Comment on attachment 8944448 [details] [diff] [review]
part 6 - tests

We really want wpt tests here, if there aren't such yet.

The webidl change should be in a different patch.
Attachment #8944448 - Flags: review?(bugs) → review-
(I don't see worker specific wpt tests in https://wpt.fyi/resource-timing)
Comment on attachment 8944456 [details] [diff] [review]
part 5 - dispatch resourcetimingbufferfull on workers

>+namespace {
>+
>+class DispatchBufferFullEventRunnable final : public WorkerRunnable
>+{
>+  RefPtr<PerformanceWorker> mPerformance;
Per Mozilla coding style, member variables go to the end of the class.
Attachment #8944456 - Flags: review?(bugs) → review+
Attachment #8944446 - Flags: review?(bugs) → review+
Comment on attachment 8944445 [details] [diff] [review]
part 3 - PerformanceStorageWorker


>+// Here we use control runnable because this code must be executed also when in
>+// a sync event loop
>+class InitializeRunnable final : public WorkerControlRunnable
Initialize what? Could you rename this to something which hints what this actually does?


>+{
>+  RefPtr<PerformanceStorageWorker> mStorage;
>+
Again, nit, per coding style, member variables go to the end of the class


>+// Here we use control runnable because this code must be executed also when in
>+// a sync event loop
>+class AddRunnable final : public WorkerControlRunnable
Also this, perhaps rename to hint what this adds.


>+private:
>+  PerformanceStorageWorker(workers::WorkerPrivate* aWorkerPrivate);
>+  ~PerformanceStorageWorker();
>+
>+  Mutex mMutex;
>+
>+  // Protected by mutex.
>+  workers::WorkerPrivate* mWorkerPrivate;
This needs a comment why use of raw pointer is fine.

r-, because I feel I'll need to re-read this to ensure mutex handling is ok and such. But could you do the renaming and add comments.
Attachment #8944445 - Flags: review?(bugs) → review-
Attachment #8944444 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #16)
> Comment on attachment 8944442 [details] [diff] [review]
> part 0 - NS_NewChannel
> 
> Also other NS_NewChannel* variants need the new param. Otherwise it is
> pretty much guaranteed that it won't be used properly in all the cases.
> Or perhaps the variants of the method which also take nsINode* as a param
> can be guaranteed to be mainthread only so they don't need the new param?

Right, they are not related to workers anyhow. I don't think we need to pass this extra parameter: it would be null everywhere.
Flags: needinfo?(bugs)
Attachment #8944445 - Attachment is obsolete: true
Attachment #8944656 - Flags: review?(bugs)
Attachment #8944448 - Attachment is obsolete: true
Attachment #8944657 - Flags: review?(bugs)
Attached patch part 7 - mochitests (obsolete) — Splinter Review
I'm going to write WPT as well. But it's nice to have mochitests too.
Attachment #8944659 - Flags: review?(bugs)
I think it's nice to have this documented on MDN.
Keywords: dev-doc-needed
Attached patch part 7 - mochitests (obsolete) — Splinter Review
Redirect test included
Attachment #8944659 - Attachment is obsolete: true
Attachment #8944659 - Flags: review?(bugs)
Attachment #8944675 - Flags: review?(bugs)
Attached patch part 8 - WPTSplinter Review
Attachment #8944720 - Flags: review?(bugs)
Comment on attachment 8944675 [details] [diff] [review]
part 7 - mochitests

>+  is(entries[0].initiatorType, initiatorType, "Correct initiatorType");
>+  is(entries[0].nextHopProtocol, protocol, "Correct protocol");
>+
>+  performance.clearResourceTimings();
Perhaps check here that there are no entries anymore

>+}
>+
>+function simple_checks() {
>+  ok("performance" in self, "We have self.performance");
>+  performance.clearResourceTimings();
>+  next();
>+}
>+
>+function fetch_request() {
>+  fetch("test_worker_performance_entries.sjs")
>+  .then(r => r.blob())
>+  .then(blob => {
>+    check("test_worker_performance_entries.sjs", "other", "http/1.1");
>+  })
>+  .then(next);
>+}
>+
>+function xhr_request() {
>+  let xhr = new XMLHttpRequest();
>+  xhr.open("GET", "test_worker_performance_entries.sjs");
>+  xhr.send();
>+  xhr.onload = () => {
>+    check("test_worker_performance_entries.sjs", "xmlhttprequest", "http/1.1");
>+    next();
>+  }
>+}
>+
>+function sync_xhr_request() {
>+  let xhr = new XMLHttpRequest();
>+  xhr.open("GET", "test_worker_performance_entries.sjs", false);
>+  xhr.send();
>+  check("test_worker_performance_entries.sjs", "xmlhttprequest", "http/1.1");
>+  next();
>+}
>+
>+function import_script() {
>+  importScripts(["empty.js"]);
>+  check("empty.js", "other", "http/1.1");
>+  next();
>+}
>+
>+function buffer_full() {
>+  performance.setResourceTimingBufferSize(1);
>+  performance.onresourcetimingbufferfull = e => {
>+    ok(true, "Message received!");
>+    performance.clearResourceTimings();
>+    performance.onresourcetimingbufferfull = null;
>+    next();
>+  };
>+
>+  fetch("test_worker_performance_entries.sjs")
>+  .then(r => r.blob())
>+  .then(blob => {});
>+}
>+
>+function sync_buffer_full() {
>+  let received = false;
>+  performance.setResourceTimingBufferSize(1);
>+  performance.onresourcetimingbufferfull = e => {
>+    ok(!received, "Message received!");
>+    received = true;
>+    performance.clearResourceTimings();
>+    performance.onresourcetimingbufferfull = null;
>+    next();
>+  };
>+
>+  let xhr = new XMLHttpRequest();
>+  xhr.open("GET", "test_worker_performance_entries.sjs", false);
>+  xhr.send();
>+  ok(!received, "Message not received yet");
>+}
>+
>+function redirect() {
>+  fetch("test_worker_performance_entries.sjs?redirect")
>+  .then(r => r.text())
>+  .then(text => {
>+    is(text, "Hello world \\o/", "The redirect worked correctly");
>+    check("test_worker_performance_entries.sjs?redirect", "other", "http/1.1");
>+  })
>+  .then(next);
>+}
>+
>+let tests = [
>+  simple_checks,
>+  fetch_request,
>+  xhr_request,
>+  sync_xhr_request,
>+  import_script,
>+  buffer_full,
>+  sync_buffer_full,
>+  redirect,
>+];
>+
>+function next() {
>+  if (!tests.length) {
>+    finish();
>+    return;
>+  }
>+
>+  let test = tests.shift();
>+  test();
>+}
>+
>+next();
>diff --git a/dom/performance/tests/test_worker_performance_entries.sjs b/dom/performance/tests/test_worker_performance_entries.sjs
>new file mode 100644
>--- /dev/null
>+++ b/dom/performance/tests/test_worker_performance_entries.sjs
>@@ -0,0 +1,12 @@
>+function handleRequest(request, response)
>+{
>+  response.setHeader("Content-Type", "text/html");
>+
>+  if (request.queryString == "redirect") {
>+    response.setStatusLine(request.httpVersion, 302, "See Other");
>+    response.setHeader("Location", "test_worker_performance_entries.sjs?ok");
>+  } else {
>+    response.setStatusLine(request.httpVersion, 200, "OK");
>+    response.write("Hello world \\o/");
>+  }
>+}
Flags: needinfo?(bugs)
Attachment #8944675 - Flags: review?(bugs) → review+
Attachment #8944720 - Flags: review?(bugs) → review+
Did you try to run the wpt on Chrome?
Attachment #8944657 - Flags: review?(bugs) → review+
Comment on attachment 8944656 [details] [diff] [review]
part 3 - PerformanceStorageWorker

>+class InitializeStorageOnWorkerRunnable final : public WorkerControlRunnable
Still doesn't IMO quite express the meaning of the class.
PerformanceStorageInitializer or some such?


>+// Here we use control runnable because this code must be executed also when in
>+// a sync event loop
>+class AddEntryOnWorkerRunnable final : public WorkerControlRunnable
PerformanceEntryAdder?

>+
>+  // The nsITimedChannel argument will be used to gather all the timings.
>+  // The nsIHttpChannel argument will be used to check if any cross-origin
>+  // redirects occurred.
>+  // The last argument is the "zero time" (offset). Since we don't want
>+  // any offset for the resource timing, this will be set to "0" - the
>+  // resource timing returns a relative timing (no offset).
>+  UniquePtr<PerformanceTimingData> performanceTimingData(
>+    new PerformanceTimingData(aTimedChannel, aChannel, 0));
>+
>+  UniquePtr<PerformanceProxyData> data(
>+    new PerformanceProxyData(Move(performanceTimingData), initiatorType,
>+                             entryName));
Around here we duplicate quite a bit the logic from main thread. Can you think of a way to not do that? Perhaps another patch to de-duplicate?
At least both places need to have comment that if one changes one place, also the other place needs to be modified.

>+PerformanceStorageWorker::InitializeOnWorker()
>+{
>+  {
>+    MutexAutoLock lock(mMutex);
>+    MOZ_ASSERT(mState == eInitializing);
>+  }
This block should be inside #ifdef DEBUG, or make MOZ_ASSERT MOZ_RELEASE_ASSERT
Could you add some thread check to this method. Looks like this shouldn't be called on the parent/main thread.


>+
>+  mWorkerHolder.reset(new PerformanceStorageWorkerHolder(this));
>+  if (!mWorkerHolder->HoldWorker(mWorkerPrivate, Canceling)) {
Per documentation mWorkerPrivate is protected by the mutex, but here you don't have the lock.
What is then right, documentation or code?

r- because either there is some documentation missing on mWorkerPrivate + mMutex usage, or code is wrong or documentation is wrong.
Attachment #8944656 - Flags: review?(bugs) → review-
Comment on attachment 8944456 [details] [diff] [review]
part 5 - dispatch resourcetimingbufferfull on workers

Now that I look at this, the spec doesn't talk about the event being async.
Attachment #8944456 - Flags: review+ → review-
Attachment #8944442 - Attachment is obsolete: true
Attachment #8944761 - Flags: review?(bugs)
Attachment #8944456 - Attachment is obsolete: true
Attachment #8944787 - Flags: review?(bugs)
Attachment #8944656 - Attachment is obsolete: true
Attachment #8944788 - Flags: review?(bugs)
Mochitest updated to check the sync dispatching.
Attachment #8944675 - Attachment is obsolete: true
Comment on attachment 8944787 [details] [diff] [review]
part 5 - dispatch resourcetimingbufferfull on workers

Could you explain resourcetimingbufferfull dispatching now, in case of sync XHR. 
Doesn't this end up dispatching the event while we're still spinning the sync loop in worker?

(we should fix also the main thread case, but perhaps not in this bug)

I filed https://github.com/w3c/resource-timing/issues/141
Attachment #8944787 - Flags: review?(bugs) → review-
Comment on attachment 8944788 [details] [diff] [review]
part 3 - PerformanceStorageWorker


>+void
>+PerformanceStorageWorker::ShutdownOnWorker()
>+{
>+  MutexAutoLock lock(mMutex);
>+  mWorkerPrivate->AssertIsOnWorkerThread();
What guarantees mWorkerPrivate is non-null here? ShutdownOnWorker is called in several places, like PerformanceStorageInitializer::Cancel and PerformanceEntryAdder::Cancel and 
PerformanceStorageWorkerHolder::Notify
Please explain why mWorkerPrivate can't be null here, or add a null check and if null, return early.
Attachment #8944788 - Flags: review?(bugs) → review+
Attachment #8944787 - Attachment is obsolete: true
Attachment #8945051 - Flags: review?(bugs)
Blocks: 1432758
Attachment #8945051 - Flags: review?(bugs) → review+
Comment on attachment 8944761 [details] [diff] [review]
part 0 - NS_NewChannel

It is not clear to me how CompareNetwork::Initialize should work, but that looks like an existing issue. I pinged bkelly about it.
Attachment #8944761 - Flags: review?(bugs) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e875f3702a5f
Resource timing entries Workers - part 0 - NS_NewChannel, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/07e781e37451
Resource timing entries Workers - part 1 - PerformanceStorage on main-thread, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/2efb375a8ffc
Resource timing entries Workers - part 2 - PerformanceTimingData, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/793bbfc23257
Resource timing entries Workers - part 3 - PerformanceStorageWorker, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/f70500179140
Resource timing entries Workers - part 4 - exposing partial interface, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/7034af4332e4
Resource timing entries Workers - part 5 - dispatch resourcetimingbufferfull on workers, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/af56400233d9
Resource timing entries Workers - part 6 - PerformanceResourceTiming exposed, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/f140da44ba68
Resource timing entries Workers - part 7 - mochitests, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/b96d58fd945c
Resource timing entries Workers - part 8 - WPT, r=smaug
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a29e9dbb8c42
Resource timing entries Workers - part 9 - Fixing a compilation issue, r=me CLOSED TREE
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/100b9d4f36bc
Resource timing entries Workers - part 10 - Correct parameters in NS_NewChannel in nsDataObj.cpp, r=me
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/11997de13778
Resource timing entries Workers - part 11 - WPT, r=me CLOSED TREE
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3758a68f961f
Resource timing entries Workers - part 0 - NS_NewChannel, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/8101a66c3180
Resource timing entries Workers - part 1 - PerformanceStorage on main-thread, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd25cfc3defe
Resource timing entries Workers - part 2 - PerformanceTimingData, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/619d2fe88e78
Resource timing entries Workers - part 3 - PerformanceStorageWorker, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/4186a931f07c
Resource timing entries Workers - part 4 - exposing partial interface, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/08827b793aff
Resource timing entries Workers - part 5 - dispatch resourcetimingbufferfull on workers, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfd991faa7cb
Resource timing entries Workers - part 6 - PerformanceResourceTiming exposed, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/e08739c23a5e
Resource timing entries Workers - part 7 - mochitests, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f292fa4e4a3
Resource timing entries Workers - part 8 - WPT, r=smaug
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc833e0fe686
Resource timing entries Workers - fixing a WPT failure, r=me CLOSED TREE
Depends on: 1434213
Flags: needinfo?(amarchesini)
I think I've got this right, documentation-wise:

All relevant interface (and member) pages have had a "available in workers" banner and compat data added to them as appropriate:
https://developer.mozilla.org/en-US/docs/Web/API/Performance
https://developer.mozilla.org/en-US/docs/Web/API/PerformanceEntry
https://developer.mozilla.org/en-US/docs/Web/API/PerformanceResourceTiming
https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Functions_and_classes_available_to_workers

Note added to Fx60 rel notes too:
https://developer.mozilla.org/en-US/Firefox/Releases/60#DOM

Let me know if that's OK. Thanks!
Flags: needinfo?(amarchesini)
> https://developer.mozilla.org/en-US/docs/Web/API/PerformanceEntry

In the browser compatibility table, there is "Avaialable on workers" and "Available to web workers". We should remove the first one.

> https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/
> Functions_and_classes_available_to_workers

Should we show here also PerformanceEntry, PerformanceMeasure, PerformanceMark, PerformanceObserver too?

Thanks!
Flags: needinfo?(amarchesini)
(In reply to Andrea Marchesini [:baku] from comment #49)
> > https://developer.mozilla.org/en-US/docs/Web/API/PerformanceEntry
> 
> In the browser compatibility table, there is "Avaialable on workers" and
> "Available to web workers". We should remove the first one.

D'oh! I have actually corrected the spelling mistakes and deleted the second one, as the first one is more accurate in terms of info. I added the updated Fx support info back in.

> 
> > https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/
> > Functions_and_classes_available_to_workers
> 
> Should we show here also PerformanceEntry, PerformanceMeasure,
> PerformanceMark, PerformanceObserver too?

Good point — I've added them.
Depends on: 1437897
You need to log in before you can comment on or make changes to this bug.