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

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: catalinb, Assigned: nsm)

Tracking

Trunk
mozilla42
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(2 attachments)

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
Depends on: 1130686
Depends on: 1130687
Blocks: 1130687
No longer depends on: 1130687
Component: DOM → DOM: Service Workers
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.
Attachment #8640010 - Flags: review?(bzbarsky)
Attachment #8640010 - Flags: review?(amarchesini)
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.
Attachment #8640010 - Flags: review?(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....
Do you mean like "DOMFocusTab" ? or a non-chrome event API?
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 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.
Attachment #8640010 - Attachment description: MozReview Request: Bug 1144660 - client.focus() adds window interaction checks and directly uses DOMWebNotificationClicked to focus window. r?baku,bz → MozReview Request: Bug 1144660 - client.focus() adds window interaction checks and directly uses DOMServiceWorkerFocusClient to focus window. r?baku,dao
Attachment #8640010 - Flags: review?(dao)
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!
> 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.  :(
Assignee: nobody → nsm.nikhil
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!
Attachment #8640010 - Flags: review?(amarchesini) → review?(bkelly)
Attachment #8640010 - Flags: review?(bkelly) → review?(ehsan)
Attachment #8640010 - Flags: review?(ehsan) → review+
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.
Flags: needinfo?(dao)
Attachment #8640010 - Flags: review?(dao) → review+
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/13ed6f60d3f9569b56d3d18584573d4481c500b9
changeset:  13ed6f60d3f9569b56d3d18584573d4481c500b9
user:       Nikhil Marathe <nsm.nikhil@gmail.com>
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.
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
Flags: needinfo?(nsm.nikhil)
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.
Attachment #8640010 - Attachment description: MozReview Request: Bug 1144660 - client.focus() adds window interaction checks and directly uses DOMServiceWorkerFocusClient to focus window. r?baku,dao → MozReview Request: Bug 1144660 - Make KeepAliveHandler threadsafe refcounted. r?ehsan
Attachment #8640010 - Flags: review+
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 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?
Flags: needinfo?(nsm.nikhil)
Attachment #8640010 - Flags: review+ → review?(ehsan)
Attachment #8640010 - Flags: review?(ehsan) → review+
Comment on attachment 8640010 [details]
MozReview Request: Bug 1144660 - Make KeepAliveHandler threadsafe refcounted. r?ehsan

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

Ship It!
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/c6eb267a1004e04edd08e19a14466698cd0e1e82
changeset:  c6eb267a1004e04edd08e19a14466698cd0e1e82
user:       Nikhil Marathe <nsm.nikhil@gmail.com>
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 <nsm.nikhil@gmail.com>
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.
https://hg.mozilla.org/mozilla-central/rev/c6eb267a1004
https://hg.mozilla.org/mozilla-central/rev/23e43cedb8e9
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.