967264 - ServiceWorker installation

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

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

Bug for the installation procedure for ServiceWorkers.

Comment 1

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

Attachment: ServiceWorker installation bits.

Bent,

Could you look at the general architecture.
NavigatorServiceWorker is per document, while ServiceWorkerManager is shared across the (child) process. Right now in-memory only.

I didn't understand how this is going to share state across seperate browser tabs (in a multiprocess system) without moving it to the parent process. I'm probably missing something from last Wednesday.

Comment 2

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

Attachment: ServiceWorker installation bits.

Now with some controlling scope checking tests!

Comment 3

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

Attachment: Patch 1: InstallPhaseEvent and InstallEvent.

Comment 4

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

Attachment: Patch 2: ServiceWorkerGlobalScope stub.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8370248 [details] [diff] [review]
ServiceWorker installation bits.

Review of attachment 8370248 [details] [diff] [review]:
-----------------------------------------------------------------

I think this generally looks fine. You'll probably want khuey to do your full review though.

::: configure.in
@@ +1437,5 @@
>      # Turn off the following warnings that -Wall turns on:
>      # -Wno-invalid-offsetof - we use offsetof on non-POD types frequently
>      #
>      MOZ_CXX_SUPPORTS_WARNING(-Wno-, invalid-offsetof, ac_cxx_has_wno_invalid_offsetof)
> +    MOZ_CXX_SUPPORTS_WARNING(-Wno-, inline-new-delete, ac_cxx_has_wno_inline_new_delete)

Eh?

::: content/base/src/nsDocument.cpp
@@ +2480,5 @@
>  
> +  bool dummy;
> +  nsCOMPtr<nsIServiceWorkerManager> swm = do_GetService(SERVICEWORKERMANAGER_CONTRACTID);
> +  if (!swm) {
> +    fprintf(stderr, "NSM No ServiceWorkerManager to control this document.\n");

All these fprintfs should be replaced with normal NS_WARNING, or maybe NS_WARN_IF

::: dom/workers/NavigatorServiceWorker.cpp
@@ +68,5 @@
> +    aRv.Throw(rv);
> +    return nullptr;
> +  }
> +
> +  nsRefPtr<Promise> ret = static_cast<Promise*>(promise.get());

There should be an unwrap function for each DOM binding object that you should use. Manually casting is not a good idea.

::: dom/workers/ServiceWorkers.manifest
@@ +1,3 @@
> +# This component drives ServiceWorker invocation and keeps records (both disk and in-memory).
> +# Some details are handled in C++.
> +component {387bb741-4d82-4b66-9f52-28ed3604c18e} ServiceWorkerManager.js

This file is missing?

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8376200 [details] [diff] [review]
Patch 2: ServiceWorkerGlobalScope stub.

This binding stuff is better reviewed by khuey.

Comment 7

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

Attachment: Patch 1: InstallPhaseEvent and InstallEvent.

Doesn't do anything. Fleshes out WebIDL.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8381243 [details] [diff] [review]
Patch 1: InstallPhaseEvent and InstallEvent.

Review of attachment 8381243 [details] [diff] [review]:
-----------------------------------------------------------------

Kyle, can you look over the bindings.conf changes?

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8381243 [details] [diff] [review]
Patch 1: InstallPhaseEvent and InstallEvent.

Review of attachment 8381243 [details] [diff] [review]:
-----------------------------------------------------------------

This need some cleanup before I can review properly.

::: dom/workers/ServiceWorkerEvents.cpp
@@ +10,5 @@
> +
> +using namespace mozilla::dom;
> +USING_WORKERS_NAMESPACE
> +
> +BEGIN_WORKERS_NAMESPACE

You don't need this as long as you've got the 'using' above.

::: dom/workers/ServiceWorkerEvents.h
@@ +9,5 @@
> +#include "nsDOMEvent.h"
> +#include "mozilla/dom/InstallPhaseEventBinding.h"
> +#include "mozilla/dom/InstallEventBinding.h"
> +#include "mozilla/dom/Promise.h"
> +#include "mozilla/dom/workers/bindings/ServiceWorker.h"

Please restructure this header to only include the absolute minimum necessary, forward declare everything else.

@@ +38,5 @@
> +              const nsAString& aType,
> +              const InstallPhaseEventInit& aOptions,
> +              ErrorResult& aRv)
> +  {
> +    //nsCOMPtr<EventTarget> target = do_QueryInterface(aGlobal.GetAsSupports());

?

@@ +119,5 @@
> +  void
> +  Replace()
> +  {
> +    //TODO(nsm)
> +    MOZ_ASSERT(false);

?

Comment 10

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

Attachment: Patch 2: ServiceWorkerGlobalScope stub.

Comment 11

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

Attachment: Very simple installation tests

Comment 12

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

Attachment: Patch 1: InstallPhaseEvent and InstallEvent.

InstallEvent and InstallPhaseEvent. Again according to https://github.com/slightlyoff/ServiceWorker/blob/master/spec/service_worker/index.html

Comment 13

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

Attachment: Patch 2: ServiceWorkerGlobalScope stub.

Comment 14

[(no longer active)]

Comment on attachment 8400297 [details] [diff] [review]
Patch 1: InstallPhaseEvent and InstallEvent.

Review of attachment 8400297 [details] [diff] [review]:
-----------------------------------------------------------------

So before I look at things further, I think I'm looking at the wrong spec or something?  Please see below.  Can you please clarify what I'm doing wrong?

::: dom/webidl/InstallEvent.webidl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/.
> + *
> + * For more information on this interface, please see
> + * FIXME(nsm)

???

@@ +6,5 @@
> + * For more information on this interface, please see
> + * FIXME(nsm)
> + */
> +
> +[Constructor(DOMString type, optional InstallEventInit eventInitDict)]

Not in the spec either.

@@ +10,5 @@
> +[Constructor(DOMString type, optional InstallEventInit eventInitDict)]
> +interface InstallEvent : InstallPhaseEvent {
> +  // The currently active worker for this scope when this worker is asked to
> +  // install itself.
> +  readonly attribute ServiceWorker? activeWorker;

This should be non-nullable.

@@ +15,5 @@
> +  void replace();
> +  Promise reloadAll();
> +};
> +
> +dictionary InstallEventInit : InstallPhaseEventInit {

This doesn't even appear in the spec!

::: dom/webidl/InstallPhaseEvent.webidl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/.
> + *
> + * For more information on this interface, please see
> + * FIXME(nsm)

???

@@ +6,5 @@
> + * For more information on this interface, please see
> + * FIXME(nsm)
> + */
> +
> +[Constructor(DOMString type, optional InstallPhaseEventInit eventInitDict)]

That's not in the spec... <http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#install-phase-event>

@@ +7,5 @@
> + * FIXME(nsm)
> + */
> +
> +[Constructor(DOMString type, optional InstallPhaseEventInit eventInitDict)]
> +interface InstallPhaseEvent : Event {

You wanna add this stuff to test_interfaces.html.

Comment 15

[(no longer active)]

Comment on attachment 8400297 [details] [diff] [review]
Patch 1: InstallPhaseEvent and InstallEvent.

I've talked to Nikhil offline, this is not ready for review yet.

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8400298 [details] [diff] [review]
Patch 2: ServiceWorkerGlobalScope stub.

Review of attachment 8400298 [details] [diff] [review]:
-----------------------------------------------------------------

The API and the spec needs a super-review.

::: dom/webidl/ServiceWorkerGlobalScope.webidl
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + *
> + * The origin of this IDL file is FIXME(nsm)

Fix that.

::: dom/workers/WorkerPrivate.cpp
@@ +5818,5 @@
>    nsRefPtr<WorkerGlobalScope> globalScope;
>    if (IsSharedWorker()) {
>      globalScope = new SharedWorkerGlobalScope(this, SharedWorkerName());
> +  } else if (IsServiceWorker()) {
> +    globalScope = new ServiceWorkerGlobalScope(this, NS_ConvertUTF8toUTF16(SharedWorkerName()));

Convert the string in the constructor, not here.

Comment 17

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

Attachment: Patch 1: ServiceWorkerGlobalScope stub.

Reordered series to introduce global scope first so that install events can be restricted to it.
khuey's suggested changes fixed.
While ServiceWorkerGlobalScope is not explicitly preffed off, nothing creates it.

Comment 18

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

Attachment: Patch 2: InstallPhaseEvent and InstallEvent.

Ehsan, I think this should be landable now.

jst for super-review.

Comment 19

[(no longer active)]

Comment on attachment 8413970 [details] [diff] [review]
Patch 2: InstallPhaseEvent and InstallEvent.

Review of attachment 8413970 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/InstallEvent.webidl
@@ +14,5 @@
> +interface InstallEvent : InstallPhaseEvent {
> +  // The currently active worker for this scope when this worker is asked to
> +  // install itself.
> +  // This may be null when a ServiceWorker is being installed for a previously
> +  // uncontrolled scope.

Please raise a spec issue since this is not nullable in the spec.

::: dom/webidl/InstallPhaseEvent.webidl
@@ +9,5 @@
> +
> +// While not explicitly restricted to ServiceWorkerGlobalScope, it probably
> +// should be. https://github.com/slightlyoff/ServiceWorker/issues/254
> +[Constructor(DOMString type, optional EventInit eventInitDict),
> + Func="mozilla::dom::workers::ServiceWorkerEventsVisible"]

I'm not 100% sure if this is the best way to do this.  Check with bz please?

Also, I know that this is implied, but please add a [Pref] here too to make it easier for people to grep for SW-related APIs.

::: dom/workers/ServiceWorkerEvents.cpp
@@ +17,5 @@
> +
> +BEGIN_WORKERS_NAMESPACE
> +
> +bool
> +ServiceWorkerEventsVisible(JSContext* aCx, JSObject* aObj)

This isn't declared in the header...  Not sure how this patch compiles.  ;-)

@@ +37,5 @@
> +{
> +  MOZ_ASSERT(!NS_IsMainThread());
> +
> +  // Only first caller counts.
> +  if (EventPhase() == AT_TARGET && !mWaitingOnPromise) {

Is there any situation where mPromise can be non-null but mWaitingOnPromise is false?  I think not, so you can drop mWaitingOnPromise.

@@ +38,5 @@
> +  MOZ_ASSERT(!NS_IsMainThread());
> +
> +  // Only first caller counts.
> +  if (EventPhase() == AT_TARGET && !mWaitingOnPromise) {
> +    mPromise = &aPromise;

Not to others reading this code.  Nikhil confirmed on IRC that future parts of this patch are supposed to use the mPromise passed here.

@@ +44,5 @@
> +  }
> +
> +  JSContext* cx = nsContentUtils::GetDefaultJSContextForThread();
> +  ErrorResult rv;
> +  return Promise::Resolve(nullptr, cx, JS::UndefinedHandleValue, rv);

This is not very useful.  I'd say make the method void for now until the spec figures out what this promise is supposed to do.

::: dom/workers/ServiceWorkerEvents.h
@@ +63,5 @@
> +  already_AddRefed<Promise>
> +  WaitUntil(Promise& aPromise);
> +
> +  bool
> +  WaitingOnPromise()

Nit: const

@@ +69,5 @@
> +    return mWaitingOnPromise;
> +  }
> +
> +  already_AddRefed<Promise>
> +  GetPromise()

Nit: const.

@@ +118,5 @@
> +    return Constructor(owner, aType, aOptions);
> +  }
> +
> +  already_AddRefed<ServiceWorker>
> +  GetActiveWorker()

here too.

Comment 20

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

::: dom/webidl/InstallPhaseEvent.webidl
@@ +9,5 @@
> +
> +// While not explicitly restricted to ServiceWorkerGlobalScope, it probably
> +// should be. https://github.com/slightlyoff/ServiceWorker/issues/254
> +[Constructor(DOMString type, optional EventInit eventInitDict),
> + Func="mozilla::dom::workers::ServiceWorkerEventsVisible"]

bz,

Ehsan says:"""
I'm not 100% sure if this is the best way to do this.  Check with bz please?"""

The objective is to restrict the two interfaces to the ServiceWorkerGlobalScope global.

Do I need to add these restricted interfaces to test_interfaces.html?

Comment 21

[Boris Zbarsky [:bzbarsky]]

That seems fine, though you could use UNWRAP_OBJECT to save a bit of typing.

We should really work on implementing [Exposed] annotations...

You don't need to add to test_interfaces.html, since that runs in Window contexts only.

Comment 22

[Johnny Stenback (:jst)]

Comment on attachment 8413898 [details] [diff] [review]
Patch 1: ServiceWorkerGlobalScope stub.

- In ServiceWorkerGlobalScope::WrapGlobalObject(JSContext* aCx):

+  // We're wrapping the global, so the scope is undefined.
+  JS::Rooted<JSObject*> scope(aCx);

This is unused, please remove. And please do the same cleanup in DedicatedWorkerGlobalScope::WrapGlobalObject() and  SharedWorkerGlobalScope::WrapGlobalObject() too (this is leftovers from the fix for bug 983620).

r=jst with that.

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 8413970 [details] [diff] [review]
Patch 2: InstallPhaseEvent and InstallEvent.

Looks good!

Comment 24

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7e5f00e3f775
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/97b869b9ff95

Comment 25

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

https://hg.mozilla.org/mozilla-central/rev/7e5f00e3f775
https://hg.mozilla.org/mozilla-central/rev/97b869b9ff95

Comment 27

[Chris Mills [:cmills]]

The service workers API has all been documented (and needs a tech review.)

You can find the install-related stuff in there, for example https://developer.mozilla.org/en-US/docs/Web/API/InstallEvent