Closed Bug 1266747 Opened 8 years ago Closed 7 years ago

service worker clients.matchAll() should return window Client objects in focus order

Categories

(Core :: DOM: Service Workers, defect)

48 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Iteration:
54.3 - Mar 6
Tracking Status
firefox54 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: btpp-fixlater [e10s-multi:+] )

Attachments

(4 files, 14 obsolete files)

1.87 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.33 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
17.91 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
8.16 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
Step 2.4 of Clients.matchAll() says:

  "For each service worker client client in targetClients, in the most recently focused order for window clients"

Here:

  https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#clients-getall

We currently return window Client objects in the order that the Window was created.

Rather than try to resort the entire set of all documents all the time, we should probably just sort the results we know we are going to return from matchAll().
Whiteboard: btpp-fixlater
I'm going to work this in preparation for bug 1293277.  I need some of the underlying support in nsGlobalWindow and the tests.  The duplicate bits that will get thrown away after bug 1293277 will be very small.
Assignee: nobody → bkelly
Blocks: 1293277
Status: NEW → ASSIGNED
See Also: → 1334383
Attachment #8840132 - Attachment is obsolete: true
Attachment #8840144 - Attachment is obsolete: true
Iteration: --- → 54.3 - Mar 6
Work-in-progress on the test.  This is taking the most time since there is some unexpected focus behavior.  For example, frames can't be focused unless an ancestor window is already focused, etc.

Anyway, the guts of the test are written.  I'll add the specific test cases and expectations tomorrow.
Updated test, but some weird behavior to figure out.
Attachment #8840642 - Attachment is obsolete: true
Update the patch to track focus time when the document replaces the about:blank document in an already focused window.
Attachment #8840101 - Attachment is obsolete: true
Fix creation order issues for controlled clients.
Attachment #8840565 - Attachment is obsolete: true
Hmm.  The P4 patch is not updating when I upload it for some reason.
Try build shows the test does not work in non-e10s mode.  I can confirm the same locally.
So, I'm seeing different behavior between e10s and non-e10s.  Is this ok, or should one of them be considered bugged?  Olli, can you help me figure it out?

The difference I am seeing is that the window.focus() methods I'm calling in the test are ignored in non-e10s mode, but honored in e10s mode.

Tracing the code its because in non-e10s the window is considered hidden and hits the short-circuit:

https://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#7511

So, why are the tabs hidden in non-e10s, but not in e10s?

In non-e10s we have a parent frame and fail this condition:

https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#6155

The frame->IsVisibleConsideringAncestors() is failing here:

https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#367

I believe the "deck" selection means the tab is not selected.  This is indeed the case in the test.

In contrast, e10s has no parent.  So we never check the frame deck selection.  Instead we check the tree owner:

https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#6171

I haven't verified this part, but it seems that this code just checks to see if the xul window is visible:

https://dxr.mozilla.org/mozilla-central/source/xpfe/appshell/nsContentTreeOwner.cpp#672

This means that if a browser.xul is visible then we consider *all* the tabs within that xul window visible.

Olli, does this e10s behavior sound correct?  Assuming the non-e10s behavior is more correct, how can I go about testing this kind of API depending on window focus?  Is it possible in wpt at all or must I go to browser chrome?
Flags: needinfo?(bugs)
I guess I can change the test to create frames instead of using window.open().  I still wonder about comment 17, though.  Should I file a bug in e10s focus()?
Needs some more cleanup, but this works in e10s and non-e10s locally.
Attachment #8841051 - Attachment is obsolete: true
Cleaned up test.
Attachment #8841193 - Attachment is obsolete: true
Comment on attachment 8841033 [details] [diff] [review]
P1 Track the last focus time on the nsIDocument. r=smaug

The Clients API requires returning Client objects for windows in "most recently focused" order.  See step 2.5 of:

https://w3c.github.io/ServiceWorker/#clients-getall

This patch is a first step to implement this.  It makes us track the last focus time on nsIDocument objects.
Attachment #8841033 - Flags: review?(bugs)
Comment on attachment 8840140 [details] [diff] [review]
P2 Track last focus time on ServiceWorkerClient. r=smaug

This patch then reads the last focus time from the document and stores it on our ServiceWorkerClientInfo.
Attachment #8840140 - Flags: review?(bugs)
Comment on attachment 8841034 [details] [diff] [review]
P3 Sort output of Clients.matchAll(). r=smaug

This patch changes our ServiceWorkerManager::GetAllClients() method to implement the new sorting.

We have the focus time now, but we still need the Client creation order.  We effectively get this out of the nsIObserverService by virtue of the document calling AddObserver() here:

https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#4708

I plan to replace this bit soon in bug 1293277 by storing a proper creation time for Clients.

Also note that observer service EnumerateObservers() gives us the reverse order because of:

https://dxr.mozilla.org/mozilla-central/source/xpcom/ds/nsObserverList.cpp#76

Note, we only support window Client objects at the moment, so we don't need to worry about Worker objects yet.
Attachment #8841034 - Flags: review?(bugs)
Comment on attachment 8841763 [details] [diff] [review]
P4 Add a WPT test validating Clients.matchAll() result order. a=smaug

This adds a WPT test for the Clients.matchAll() list ordering.

Note, I could only test it for nested iframes because its really hard to use window.open() and window.focus() in a WPT test.
Attachment #8841763 - Flags: review?(bugs)
(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #17)
> So, I'm seeing different behavior between e10s and non-e10s.  Is this ok, or
> should one of them be considered bugged?  Olli, can you help me figure it
> out?
> 
> The difference I am seeing is that the window.focus() methods I'm calling in
> the test are ignored in non-e10s mode, but honored in e10s mode.
What you mean with "ignored"?

> I haven't verified this part, but it seems that this code just checks to see
> if the xul window is visible:
> 
> https://dxr.mozilla.org/mozilla-central/source/xpfe/appshell/
> nsContentTreeOwner.cpp#672
That code doesn't run in child process.
TabChild::GetVisibility is the relevant bit here.
See bug 666365

> 
> This means that if a browser.xul is visible then we consider *all* the tabs
> within that xul window visible.
> 
> Olli, does this e10s behavior sound correct?  Assuming the non-e10s behavior
> is more correct,
I think it does sound.


> how can I go about testing this kind of API depending on
> window focus?  Is it possible in wpt at all or must I go to browser chrome?
Hmm, not sure.
Flags: needinfo?(bugs)
Comment on attachment 8841033 [details] [diff] [review]
P1 Track the last focus time on the nsIDocument. r=smaug

>+  // Update the last focus time on any affected documents
>+  if (aWindow && aWindow != mFocusedWindow) {
>+    const TimeStamp now(TimeStamp::Now());
>+    for (nsIDocument* doc = aWindow->GetDoc();
GetExtantDoc()

>+         doc;
>+         doc = doc->GetParentDocument()) {
>+      doc->SetLastFocusTime(now);
So this means several documents may have the same time stamp. Is that ok per spec?
I don't read it that way.
I would just update aWindow's timestamp.

But the spec is very unclear here. What does "WindowClient objects that have been focused are placed first sorted in the most recently focused order"
mean? ServiceWorker spec refers to HTML spec as if WindowClient was some object HTML spec had anything to do with.
Elsewhere ServiceWorker spec seems to refer to the relevant browsing context, and if so, the timestamp should be in the outerwindow or docshell, not in document.
https://w3c.github.io/ServiceWorker/#dom-windowclient-focus
Attachment #8841033 - Flags: review?(bugs) → review-
Comment on attachment 8840140 [details] [diff] [review]
P2 Track last focus time on ServiceWorkerClient. r=smaug

So, I think you'll need to update this to get mLastFocusTime from outerWindow, assuming outerWindow isn't null, and otherwise set mLastFocusTime to TimeStamp().
That fixed, r+
Attachment #8840140 - Flags: review?(bugs) → review+
Comment on attachment 8841034 [details] [diff] [review]
P3 Sort output of Clients.matchAll(). r=smaug




>+  // The observer service gives us the list in reverse creation order.
>+  // We need to maintain creation order, so reverse the list before
>+  // processing.
>+  docList.Reverse();
This looks a bit scary to rely on observer service implementation.
I would have probably added some counter to nsIDocument to tell the service worker registration order
But I guess this is fine if we have tests for this.
Attachment #8841034 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #27)
> >+         doc;
> >+         doc = doc->GetParentDocument()) {
> >+      doc->SetLastFocusTime(now);
> So this means several documents may have the same time stamp. Is that ok per
> spec?
> I don't read it that way.
> I would just update aWindow's timestamp.

I have an open spec issue about this:

https://github.com/w3c/ServiceWorker/issues/1080

And Jake (spec editor) and I discussed in IRC here:

http://logs.glob.uno/?c=freenode%23whatwg&s=23+Feb+2017&e=23+Feb+2017&h=focus#c1021602

> But the spec is very unclear here. What does "WindowClient objects that have
> been focused are placed first sorted in the most recently focused order"
> mean? ServiceWorker spec refers to HTML spec as if WindowClient was some
> object HTML spec had anything to do with.
> Elsewhere ServiceWorker spec seems to refer to the relevant browsing
> context, and if so, the timestamp should be in the outerwindow or docshell,
> not in document.
> https://w3c.github.io/ServiceWorker/#dom-windowclient-focus

Well, all the focus code is oriented towards documents.  It seems like storing it on other entities would be a bit unusual.  I think we are trying to conform to what document.hasFocus() does.

Also, the client concept corresponds to the environment or environment settings:

https://w3c.github.io/ServiceWorker/#service-worker-client-concept

Navigating a window or calling document.open() should create a new Client.  So I think its better associated with the inner window.

My plan in bug 1293277 is to have the inner window's ClientSource pull the focus time from the document when it calls nsIDocument::HasFocus().  It would feel weird to call nsIDocument::HasFocus(), but then get the focus time from some other object that may or may not have been associated with the document when focus happened.

Does any of this change your mind?
Flags: needinfo?(bugs)
The spec maps focus handling to browsing context, if to anything, so it is not my mind which needs to change but the spec ;)
So at least file a spec bug. Right now it is pretty much unimplementable, plenty of guess-work happening here.
Flags: needinfo?(bugs)
Comment on attachment 8841033 [details] [diff] [review]
P1 Track the last focus time on the nsIDocument. r=smaug

>+  // Update the last focus time on any affected documents
>+  if (aWindow && aWindow != mFocusedWindow) {
>+    const TimeStamp now(TimeStamp::Now());
>+    for (nsIDocument* doc = aWindow->GetDoc();
GetExtantDoc()


But ok, based on the feedback, perhaps we can take this.
Attachment #8841033 - Flags: review- → review+
Comment on attachment 8841763 [details] [diff] [review]
P4 Add a WPT test validating Clients.matchAll() result order. a=smaug

rs+
Attachment #8841763 - Flags: review?(bugs) → review+
> The spec maps focus handling to browsing context, if to anything, so it is
> not my mind which needs to change but the spec ;)

The spec does mention browsing context in various places, but the definition is actually "environment or environment settings object":

https://w3c.github.io/ServiceWorker/#dfn-service-worker-client

Linking to:

https://html.spec.whatwg.org/multipage/webappapis.html#environment
https://html.spec.whatwg.org/multipage/webappapis.html#environment-settings-object

The spec does get the browsing context from Client environment in a number of places.  For example, step 3.1 of:

https://w3c.github.io/ServiceWorker/#client-focus

Says:

  "Let browsingContext be the context object’s associated service worker client's global object's browsing context."

AFAICT there are only two places the spec mistakenly still calls a Client a browsing context:

* Step 10.5 of https://w3c.github.io/ServiceWorker/#service-worker-postmessage
* `clients` comment in https://w3c.github.io/ServiceWorker/#serviceworkerglobalscope-interface

Filed a spec issue:

https://github.com/w3c/ServiceWorker/issues/1083
(In reply to Olli Pettay [:smaug] from comment #29)
> This looks a bit scary to rely on observer service implementation.
> I would have probably added some counter to nsIDocument to tell the service
> worker registration order
> But I guess this is fine if we have tests for this.

I plan to replace this in the coming weeks with Clients API rewrite in bug 1293277.  It didn't seem worth it to add another field on nsIDocument that I would be removing so soon.
Update WPT manifest.
Attachment #8841763 - Attachment is obsolete: true
Attachment #8842005 - Flags: review+
Attachment #8842004 - Flags: review+
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d19ad1c1c214
P1 Track the last focus time on the nsIDocument. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/e54c8d21e4c1
P2 Track last focus time on ServiceWorkerClient. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/274999e28c07
P3 Sort output of Clients.matchAll(). r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/b53d88cb7099
P4 Add a WPT test validating Clients.matchAll() result order. a=smaug
Backed out

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b53d88cb7099db7cf46c687c22b28947d0f4ccc5&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=80655234&repo=mozilla-inbound

[task 2017-02-28T16:21:44.985737Z] 16:21:44     INFO - TEST-START | dom/workers/test/serviceworkers/test_claim.html
[task 2017-02-28T16:21:45.494793Z] 16:21:45     INFO - ERROR: claim_worker_2 failed to capture clients.
[task 2017-02-28T16:27:11.513784Z] 16:27:11     INFO - TEST-INFO | started process screentopng
[task 2017-02-28T16:27:12.154789Z] 16:27:12     INFO - TEST-INFO | screentopng: exit 0
[task 2017-02-28T16:27:12.155204Z] 16:27:12     INFO - Buffered messages logged at 16:21:45
[task 2017-02-28T16:27:12.159517Z] 16:27:12     INFO - TEST-PASS | dom/workers/test/serviceworkers/test_claim.html | parent exists. 
[task 2017-02-28T16:27:12.162596Z] 16:27:12     INFO - TEST-PASS | dom/workers/test/serviceworkers/test_claim.html | controller changed event received. 
[task 2017-02-28T16:27:12.164599Z] 16:27:12     INFO - TEST-PASS | dom/workers/test/serviceworkers/test_claim.html | Client was claimed and received controllerchange event. 
[task 2017-02-28T16:27:12.167559Z] 16:27:12     INFO - TEST-PASS | dom/workers/test/serviceworkers/test_claim.html | Claim should resolve with undefined. 
[task 2017-02-28T16:27:12.169520Z] 16:27:12     INFO - TEST-PASS | dom/workers/test/serviceworkers/test_claim.html | Received message from claiming worker. 
[task 2017-02-28T16:27:12.171471Z] 16:27:12     INFO - TEST-PASS | dom/workers/test/serviceworkers/test_claim.html | Worker doesn't control any client before claim. 
[task 2017-02-28T16:27:12.173537Z] 16:27:12     INFO - TEST-PASS | dom/workers/test/serviceworkers/test_claim.html | Worker should claim 2 clients. 
[task 2017-02-28T16:27:12.175752Z] 16:27:12     INFO - TEST-PASS | dom/workers/test/serviceworkers/test_claim.html | Claim should resolve with undefined. 
[task 2017-02-28T16:27:12.178056Z] 16:27:12     INFO - TEST-PASS | dom/workers/test/serviceworkers/test_claim.html | Client received message from claiming worker. 
[task 2017-02-28T16:27:12.181450Z] 16:27:12     INFO - TEST-PASS | dom/workers/test/serviceworkers/test_claim.html | MatchAll clients count before claim should be 0 
[task 2017-02-28T16:27:12.183393Z] 16:27:12     INFO - TEST-PASS | dom/workers/test/serviceworkers/test_claim.html | MatchAll clients count after claim should be 2 
[task 2017-02-28T16:27:12.185306Z] 16:27:12     INFO - TEST-PASS | dom/workers/test/serviceworkers/test_claim.html | Controlling service worker has the correct url. 
[task 2017-02-28T16:27:12.187335Z] 16:27:12     INFO - TEST-PASS | dom/workers/test/serviceworkers/test_claim.html | Client was claimed and received controllerchange event. 
[task 2017-02-28T16:27:12.189166Z] 16:27:12     INFO - Buffered messages finished
[task 2017-02-28T16:27:12.191486Z] 16:27:12     INFO - TEST-UNEXPECTED-FAIL | dom/workers/test/serviceworkers/test_claim.html | Test timed out. 
[task 2017-02-28T16:27:12.193166Z] 16:27:12     INFO -     reportError@SimpleTest/TestRunner.js:120:7
[task 2017-02-28T16:27:12.194859Z] 16:27:12     INFO -     TestRunner._checkForHangs@SimpleTest/TestRunner.js:141:7
[task 2017-02-28T16:27:12.196556Z] 16:27:12     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5
[task 2017-02-28T16:27:12.198259Z] 16:27:12     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5
[task 2017-02-28T16:27:12.199982Z] 16:27:12     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5
[task 2017-02-28T16:27:12.201902Z] 16:27:12     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5
[task 2017-02-28T16:27:12.203587Z] 16:27:12     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5
[task 2017-02-28T16:27:12.205308Z] 16:27:12     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5
[task 2017-02-28T16:27:12.207178Z] 16:27:12     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5
[task 2017-02-28T16:27:12.208906Z] 16:27:12     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5
[task 2017-02-28T16:27:12.210646Z] 16:27:12     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5
[task 2017-02-28T16:27:12.212393Z] 16:27:12     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5
[task 2017-02-28T16:27:12.214049Z] 16:27:12     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5
[task 2017-02-28T16:27:12.215716Z] 16:27:12     INFO -     TestRunner.runTests@SimpleTest/TestRunner.js:379:5
[task 2017-02-28T16:27:12.217290Z] 16:27:12     INFO -     RunSet.runtests@SimpleTest/setup.js:190:3
[task 2017-02-28T16:27:12.218920Z] 16:27:12     INFO -     RunSet.runall@SimpleTest/setup.js:169:5
[task 2017-02-28T16:27:12.220523Z] 16:27:12     INFO -     hookupTests@SimpleTest/setup.js:262:5
[task 2017-02-28T16:27:12.222252Z] 16:27:12     INFO - parseTestManifest@http://mochi.test:8888/manifestLibrary.js:36:5
[task 2017-02-28T16:27:12.224002Z] 16:27:12     INFO - getTestManifest/req.onload@http://mochi.test:8888/manifestLibrary.js:49:11
[task 2017-02-28T16:27:12.225755Z] 16:27:12     INFO - EventHandlerNonNull*getTestManifest@http://mochi.test:8888/manifestLibrary.js:45:3
[task 2017-02-28T16:27:12.227550Z] 16:27:12     INFO -     hookup@SimpleTest/setup.js:242:5
[task 2017-02-28T16:27:12.229401Z] 16:27:12     INFO - EventHandlerNonNull*@http://mochi.test:8888/tests?autorun=1&closeWhenDone=1&consoleLevel=INFO&manifestFile=tests.json&dumpOutputDirectory=%2Ftmp:11:1

See also https://treeherder.mozilla.org/logviewer.html#?job_id=80657084&repo=mozilla-inbound dom/workers/test/serviceworkers/test_serviceworker_interfaces.html | Test timed out.
Flags: needinfo?(bkelly)
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ee625ebbd40
Backed out changeset b53d88cb7099 
https://hg.mozilla.org/integration/mozilla-inbound/rev/d90cb83b58bf
Backed out changeset 274999e28c07 
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd6fd1b64337
Backed out changeset e54c8d21e4c1 
https://hg.mozilla.org/integration/mozilla-inbound/rev/901d34864971
Backed out changeset d19ad1c1c214 for failing dom/workers/test/serviceworkers/test_claim.html. r=backout
So I accidentally lost the logic that excludes Client objects from other registrations.  I'll add that back in.

It appears, though, that we've never fully implemented step 2.2.3.1 of:

https://w3c.github.io/ServiceWorker/#clients-getall

We should not only be checking the registration, but verifying the Client is controlled by the exact ServiceWorker instance calling `clients.matchAll()`.

I'll file a follow-on bug to address that.  I could just fix it, but it needs a test which might take some work.
Flags: needinfo?(bkelly)
(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #41)
> I'll file a follow-on bug to address that.  I could just fix it, but it
> needs a test which might take some work.

Filed bug 1343308.
See Also: 13343831343308
Attachment #8841034 - Attachment is obsolete: true
Attachment #8842112 - Flags: review+
Blocks: 1343308
See Also: 1343308
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1c10d908957
P1 Track the last focus time on the nsIDocument. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0ef771861c2
P2 Track last focus time on ServiceWorkerClient. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/eda3f225fb73
P3 Sort output of Clients.matchAll(). r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/69a692df37c9
P4 Add a WPT test validating Clients.matchAll() result order. a=smaug
Whiteboard: btpp-fixlater → btpp-fixlater [e10s-multi:+]
Depends on: 1344484
I've made sure that this update is covered on the matchAll() ref doc:

https://developer.mozilla.org/en-US/docs/Web/API/Clients/matchAll#Return_value

I've also added a note to the Fx54 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/54#Web_workers_and_Service_Workers

Let me know if that sounds ok. Thanks!
You need to log in before you can comment on or make changes to this bug.