1144660 - ServiceWorker: client.focus should check if the algorithm is allowed to show popups

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Cătălin Badea (:catalinb)]

The spec is not well defined in regard to service workers' permission to show popups.

This step will be ignored in client.focus for v1.

https://github.com/slightlyoff/ServiceWorker/issues/602
https://html.spec.whatwg.org/multipage/browsers.html#allowed-to-show-a-popup

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1144660 - Make KeepAliveHandler threadsafe refcounted. r?ehsan

Bug 1144660 - client.focus() adds window interaction checks and directly uses DOMWebNotificationClicked to focus window. r?baku,bz

client.focus() now directly uses the DOMWebNotificationClicked event. The
platform popup checking is not available on service workers. Instead each
worker maintains a counter of if it is allowed to interact with windows. This
counter is currently only incremented by the notificationclick event and
dropped after the event has been dispatched.

Since acquiring a client is an async operation most service workers will
perform in notificationclick, an additional extension is granted after the event
during which the service worker may focus a client. This extension is only granted
if the script invokes NotificationEvent.waitUntil() at which point the timer begins.
The extension is terminated when the Promise passed to waitUntil() is fulfilled, or
the timer expires, whichever comes first.

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://reviewboard.mozilla.org/r/14271/#review12907

Boris, I marked you just to make sure that it is ok to 'cheat' in focus() by directly calling DOMWebNotificationClicked which is a chrome event that bypasses the popup checks and other things and directly asks the tab to be focused. I've added a check to make sure this is only allowed from the notificationclick event and for a short time after it.

Andrea for the actual code.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8640010 [details]
MozReview Request: Bug 1144660 - Make KeepAliveHandler threadsafe refcounted. r?ehsan

https://reviewboard.mozilla.org/r/14271/#review12911

::: dom/workers/ServiceWorkerWindowClient.cpp:93
(Diff revision 1)
> -      mozilla::ErrorResult result;
> +      nsContentUtils::DispatchChromeEvent(window->GetExtantDoc(), window->GetOuterWindow(), NS_LITERAL_STRING("DOMWebNotificationClicked"), true, true);

I'm really not OK with this part, because I see no indication that it's guaranteed to work in a different Firefox UI, or indeed even in Firefox if they change some stuff on their end.

Please add an explicit API that does what you want that chrome will promise to implement....

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Do you mean like "DOMFocusTab" ? or a non-chrome event API?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Either an event that is designed to do what we want here or explicit API on the chrometreeowner or whatnot.  I'm not sure what the current e10s-friendly set of integration points like this is.

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8640010 [details]
MozReview Request: Bug 1144660 - Make KeepAliveHandler threadsafe refcounted. r?ehsan

Bug 1144660 - client.focus() adds window interaction checks and directly uses DOMServiceWorkerFocusClient to focus window. r?baku,dao

client.focus() now directly uses the DOMServiceWorkerFocusClient event. The
platform popup checking is not available on service workers. Instead each
worker maintains a counter of if it is allowed to interact with windows. This
counter is currently only incremented by the notificationclick event and
dropped after the event has been dispatched.

Since acquiring a client is an async operation most service workers will
perform in notificationclick, an additional extension is granted after the event
during which the service worker may focus a client. This extension is only granted
if the script invokes NotificationEvent.waitUntil() at which point the timer begins.
The extension is terminated when the Promise passed to waitUntil() is fulfilled, or
the timer expires, whichever comes first.

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://reviewboard.mozilla.org/r/14271/#review12911

> I'm really not OK with this part, because I see no indication that it's guaranteed to work in a different Firefox UI, or indeed even in Firefox if they change some stuff on their end.
> 
> Please add an explicit API that does what you want that chrome will promise to implement....

Boris, I've added another event that has the same behaviour right now, but presumably could change in the future. The browser is the only consumer right now. I'm talking to Robert Bindar who is working on notifications in b2g since in b2g, tapping a notification already brings up the app and there are no 'tabs', so I'm not sure how that will work exactly.

Dao, could you review the new event added? Thanks!

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=115096639caa

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Boris, I've added another event that has the same behaviour right now

Sounds good as long as we document this somewhere because events are incredibly non-discoverable.  :(

Comment 10

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8640010 [details]
MozReview Request: Bug 1144660 - Make KeepAliveHandler threadsafe refcounted. r?ehsan

Ben, could you look at this since Andrea is on vacation.

Dao, we could really use this in 42. Thanks!

Comment 11

[(no longer active)]

Comment on attachment 8640010 [details]
MozReview Request: Bug 1144660 - Make KeepAliveHandler threadsafe refcounted. r?ehsan

https://reviewboard.mozilla.org/r/14271/#review13481

Note that I didn't review the browser parts.

::: dom/workers/ServiceWorkerManager.cpp:107
(Diff revision 2)
> +uint32_t gDOMDisableOpenClickDelay;

Please initialize to 0.  Also, you should make this an atomic.

::: dom/workers/ServiceWorkerManager.cpp:1644
(Diff revision 2)
> -  void
> +  virtual void

Drop the virtual keyword please.

::: dom/workers/ServiceWorkerManager.cpp:1654
(Diff revision 2)
> -  void
> +  virtual void

Drop the virtual keyword please.

::: dom/workers/ServiceWorkerManager.cpp:1678
(Diff revision 2)
> +  explicit ClearWindowAllowedRunnable(WorkerPrivate* aWorkerPrivate,

This doesn't need to be explicit.

::: dom/workers/ServiceWorkerManager.cpp:1685
(Diff revision 2)
> +  virtual bool

No virtual keyword where override is present (here and elsewhere).

::: dom/workers/ServiceWorkerManager.cpp:1688
(Diff revision 2)
> +    // Silence bad assertions.

Bad assertions being what?  This needs clarification.

::: dom/workers/ServiceWorkerManager.cpp:1696
(Diff revision 2)
> +    // Silence bad assertions.

Ditto.

::: dom/workers/ServiceWorkerManager.cpp:1775
(Diff revision 2)
> +  explicit AllowWindowInteractionKeepAliveHandler(const nsMainThreadPtrHandle<ServiceWorker>& aServiceWorker,

Drop explicit.

::: dom/workers/WorkerScope.h:160
(Diff revision 2)
> +  WindowInteractionAllowed()

Nit: const.

::: dom/workers/test/serviceworkers/mochitest.ini:232
(Diff revision 2)
> +[test_notificationclick_focus.html]

Given the browser specific parts, this cannot pass anywhere except on desktop, right?  If so, you should mark it as such.

Comment 12

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3dcb1748036

Comment 13

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/13ed6f60d3f9569b56d3d18584573d4481c500b9
changeset:  13ed6f60d3f9569b56d3d18584573d4481c500b9
user:       Nikhil Marathe <[e-mail]>
date:       Thu Jul 23 08:30:27 2015 -0700
description:
Bug 1144660 - client.focus() adds window interaction checks and directly uses DOMServiceWorkerFocusClient to focus window. r=ehsan,dao

client.focus() now directly uses the DOMServiceWorkerFocusClient event. The
platform popup checking is not available on service workers. Instead each
worker maintains a counter of if it is allowed to interact with windows. This
counter is currently only incremented by the notificationclick event and
dropped after the event has been dispatched.

Since acquiring a client is an async operation most service workers will
perform in notificationclick, an additional extension is granted after the event
during which the service worker may focus a client. This extension is only granted
if the script invokes NotificationEvent.waitUntil() at which point the timer begins.
The extension is terminated when the Promise passed to waitUntil() is fulfilled, or
the timer expires, whichever comes first.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This (and everything else in that push) backed out for mochitest-push crashes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/15512b2f6f41

https://treeherder.mozilla.org/logviewer.html#?job_id=12539301&repo=mozilla-inbound

Comment 15

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8640010 [details]
MozReview Request: Bug 1144660 - Make KeepAliveHandler threadsafe refcounted. r?ehsan

Bug 1144660 - Make KeepAliveHandler threadsafe refcounted. r?ehsan

This works because KeepAliveHandler only holds nsMainThreadPtrHolder<> which is
itself thread safe.  It has assertions that the PromiseNativeHandler overrides
are only called on the worker and these assertions are satisfied.

Comment 16

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1144660 - client.focus() adds window interaction checks and directly uses DOMServiceWorkerFocusClient to focus window. r=ehsan,dao

Bug 1144660 - client.focus() adds window interaction checks and directly uses DOMServiceWorkerFocusClient to focus window. r=ehsan,dao

client.focus() now directly uses the DOMServiceWorkerFocusClient event. The
platform popup checking is not available on service workers. Instead each
worker maintains a counter of if it is allowed to interact with windows. This
counter is currently only incremented by the notificationclick event and
dropped after the event has been dispatched.

Since acquiring a client is an async operation most service workers will
perform in notificationclick, an additional extension is granted after the event
during which the service worker may focus a client. This extension is only granted
if the script invokes NotificationEvent.waitUntil() at which point the timer begins.
The extension is terminated when the Promise passed to waitUntil() is fulfilled, or
the timer expires, whichever comes first.

Comment 17

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8640010 [details]
MozReview Request: Bug 1144660 - Make KeepAliveHandler threadsafe refcounted. r?ehsan

Sorry, mozreview interprets r=foo as approval. Could you review this change?

Comment 18

[(no longer active)]

Comment on attachment 8640010 [details]
MozReview Request: Bug 1144660 - Make KeepAliveHandler threadsafe refcounted. r?ehsan

https://reviewboard.mozilla.org/r/14271/#review13675

Ship It!

Comment 19

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/c6eb267a1004e04edd08e19a14466698cd0e1e82
changeset:  c6eb267a1004e04edd08e19a14466698cd0e1e82
user:       Nikhil Marathe <[e-mail]>
date:       Wed Aug 05 20:58:10 2015 -0700
description:
Bug 1144660 - Make KeepAliveHandler threadsafe refcounted. r=ehsan

This works because KeepAliveHandler only holds nsMainThreadPtrHolder<> which is
itself thread safe.  It has assertions that the PromiseNativeHandler overrides
are only called on the worker and these assertions are satisfied.

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/23e43cedb8e9d778fdcdb7c033b2b57505b2f728
changeset:  23e43cedb8e9d778fdcdb7c033b2b57505b2f728
user:       Nikhil Marathe <[e-mail]>
date:       Thu Jul 23 08:30:27 2015 -0700
description:
Bug 1144660 - client.focus() adds window interaction checks and directly uses DOMServiceWorkerFocusClient to focus window. r=ehsan,dao

client.focus() now directly uses the DOMServiceWorkerFocusClient event. The
platform popup checking is not available on service workers. Instead each
worker maintains a counter of if it is allowed to interact with windows. This
counter is currently only incremented by the notificationclick event and
dropped after the event has been dispatched.

Since acquiring a client is an async operation most service workers will
perform in notificationclick, an additional extension is granted after the event
during which the service worker may focus a client. This extension is only granted
if the script invokes NotificationEvent.waitUntil() at which point the timer begins.
The extension is terminated when the Promise passed to waitUntil() is fulfilled, or
the timer expires, whichever comes first.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c6eb267a1004
https://hg.mozilla.org/mozilla-central/rev/23e43cedb8e9