Closed Bug 984048 Opened 10 years ago Closed 10 years ago

Implement ServiceWorkerManager: registration and installation

Categories

(Core :: DOM: Workers, defect)

Product:
Core
Component:
DOM: Workers
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla34

People

(Reporter: nsm, Assigned: nsm)

References

(
URL
)

Details

(Keywords: dev-doc-complete)

Attachments

(10 files, 21 obsolete files)

Part 1 - ServiceWorkerManager Register()
32.34 KB, patch
ehsan.akhgari
: review+
nsm
: checkin+
Details | Diff | Splinter Review
Patch 6 - Scope ordering and utility functions.
14.32 KB, patch
ehsan.akhgari
: review+
nsm
: checkin+
Details | Diff | Splinter Review
Part 8 - Activate algorithm.
20.39 KB, patch
bent.mozilla
: review+
ehsan.akhgari
: review+
nsm
: checkin+
Details | Diff | Splinter Review
Part 1.1 - Register should ignore scope fragment.
1.11 KB, patch
ehsan.akhgari
: review+
nsm
: checkin+
Details | Diff | Splinter Review
Part 3 - Update algorithm.
27.22 KB, patch
khuey
: review+
nsm
: checkin+
Details | Diff | Splinter Review
Patch 5: Install() algorithm.
22.41 KB, patch
khuey
: review+
nsm
: checkin+
Details | Diff | Splinter Review
Patch 4 - Handle parse and uncaught errors.
17.89 KB, patch
khuey
: review+
jorendorff
: review+
nsm
: checkin+
Details | Diff | Splinter Review
Patch 4 interdiff
11.52 KB, patch
Details | Diff | Splinter Review
Part 7 - Documents register themselves with corresponding ServiceWorkerInfo
20.88 KB, patch
bzbarsky
: review+
nsm
: checkin+
Details | Diff | Splinter Review
Add non-null installing worker check to FinishInstall
3.62 KB, patch
ehsan.akhgari
: review+
nsm
: checkin+
Details | Diff | Splinter Review 
The ServiceWorkerManager is a per-process service that will manage ServiceWorker information and spinning up instances of workers for that process. Basically, book-keeping.
I should've broken this up into smaller patches, but it was too late :(. Fortunately I stopped at what's below and the rest is in different patches.

This patch implements register() and the series of steps that occur from there, including update and install. Activation and unregistration is not implemented, neither is sending events to NavigatorServiceWorker.
Assignee: nobody → nsm.nikhil
Comment on attachment 8400309 [details] [diff] [review]
ServiceWorkerManager register and installation

Ehsan for the majority of the code.
Ben for CreateServiceWorkerForWindow(), CreateServiceWorkerForDocument(), creation of workers in ResolveRegisterPromises()

Latest algorithms are usually at https://github.com/slightlyoff/ServiceWorker/blob/master/spec/service_worker/algorithms.md
This patch may be slightly outdated.

Activation and ServiceWorkerContainer event firing is in different bugs.
Attachment #8400309 - Flags: feedback?(ehsan)
Attachment #8400309 - Flags: feedback?(bent.mozilla)
Keywords: dev-doc-needed
Comment on attachment 8400309 [details] [diff] [review]
ServiceWorkerManager register and installation

I've talked to Nikhil offline, this is not ready for review yet.
Attachment #8400309 - Flags: feedback?(ehsan)
Attachment #8400309 - Flags: feedback?(bent.mozilla)
Introduces the ServiceWorkerManager, which tracks ServiceWorker registrations.
Implements the Register() algorithm, with blank spaces where Update() and other algorithms will fill them in.
Attachment #8416031 - Flags: review?(ehsan)
Comment on attachment 8416031 [details] [diff] [review]
Part 1 - ServiceWorkerManager Register()

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

::: dom/workers/ServiceWorkerManager.h
@@ +150,5 @@
> +  // }) and then another page calls register("/bar_worker.js", { scope: "/*" })
> +  // while the install is in progress. The async install steps for register
> +  // bar_worker.js could finish before foo_worker.js, but bar_worker still has
> +  // to be the winning controller.
> +  // FIXME(nsm): Move this into per domain?

Please ignore this entire comment, it's no longer relevant. Removed in the update() patch.
Moved this here from Bug 931249 since the patch application order requires it to be between register() and update() so it makes more sense to keep it in the same bug.
Attachment #8416101 - Flags: review?(khuey)
Comment on attachment 8416031 [details] [diff] [review]
Part 1 - ServiceWorkerManager Register()

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

::: dom/interfaces/base/nsIServiceWorkerManager.idl
@@ +6,5 @@
> +#include "domstubs.idl"
> +
> +interface nsIURI;
> +interface nsIDOMWindow;
> +interface nsIDocument;

These are all unneeded.

@@ +8,5 @@
> +interface nsIURI;
> +interface nsIDOMWindow;
> +interface nsIDocument;
> +
> +[scriptable, uuid(d9539ecb-6665-452c-bae7-4e42f25d23aa)]

This doesn't need to be scriptable, right?

::: dom/workers/ServiceWorkerContainer.cpp
@@ +40,5 @@
>  {
> +  nsCOMPtr<nsISupports> promise;
> +
> +  nsCOMPtr<nsIServiceWorkerManager> swm = do_GetService(SERVICEWORKERMANAGER_CONTRACTID);
> +  MOZ_ASSERT(swm);

I don't think you can just assert success here.  :-)  You should handle the failure properly.

@@ +59,5 @@
>  {
> +  nsCOMPtr<nsISupports> promise;
> +
> +  nsCOMPtr<nsIServiceWorkerManager> swm = do_GetService(SERVICEWORKERMANAGER_CONTRACTID);
> +  MOZ_ASSERT(swm);

Same here.

::: dom/workers/ServiceWorkerManager.cpp
@@ +82,5 @@
> +      swm->mDomainMap.Get(domain);
> +    // FIXME(nsm): Refactor this pattern.
> +    if (!swm->mDomainMap.Get(domain, &domainInfo)) {
> +      domainInfo = new ServiceWorkerManager::ServiceWorkerDomainInfo;
> +      domainInfo->mDomain = domain;

It would be nice if ServiceWorkerDomainInfo had a real ctor.

@@ +97,5 @@
> +    }
> +
> +    if (registration) {
> +      registration->mPendingUninstall = false;
> +      if (spec.Equals(registration->mScriptSpec)) {

So if this condition evaluates to false, we change an existing registration.  That's the wrong behavior if I'm reading the spec correctly.

@@ +155,5 @@
> +  // ServiceWorkers.
> +  MOZ_ASSERT(!nsContentUtils::IsCallerChrome());
> +
> +  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aWindow);
> +  nsCOMPtr<nsIGlobalObject> sgo = do_QueryInterface(window);

nsPIDOMWindow is not builtinclass, so these could fail.

@@ +181,5 @@
> +    }
> +  }
> +
> +  nsCOMPtr<nsIPrincipal> documentPrincipal =
> +    do_CreateInstance("@mozilla.org/nullprincipal;1");

Maybe create the null principal in an else branch?

@@ +192,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  // allowIfInheritsPrincipal: allow data: URLs

The spec doesn't really say what we should do here, right?  That should be clarified.

@@ +200,5 @@
> +    return NS_ERROR_DOM_SECURITY_ERR;
> +  }
> +
> +  nsCOMPtr<nsIURI> scopeURI;
> +  rv = NS_NewURI(getter_AddRefs(scopeURI), aScope, nullptr, documentURI);

XXX Do we handle <http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#path-expression> somewhere?

@@ +219,5 @@
> +  }
> +
> +  promise.forget(aPromise);
> +  nsRefPtr<nsIRunnable> registerRunnable =
> +    new RegisterRunnable(window, cleanedScope, scriptURI, static_cast<Promise*>(*aPromise));

If you reorder these statements, you won't need this ugly cast.

::: dom/workers/ServiceWorkerManager.h
@@ +40,5 @@
> +  { }
> +};
> +
> +/*
> + * NOTE: At any time, ownership of a ServiceWorkerRegistration is owned

s/ownership of //

@@ +41,5 @@
> +};
> +
> +/*
> + * NOTE: At any time, ownership of a ServiceWorkerRegistration is owned
> + * solely by this hashtable.

_which_ hashtable? ;)

@@ +47,5 @@
> + */
> +class ServiceWorkerRegistration
> +{
> +public:
> +  nsCString mScope;

It would be nice if we could avoid storing this once as the key to the hashtable and once here.  Is that an easy change to make?

@@ +81,5 @@
> +      mHasUpdatePromise(false),
> +      mPendingUninstall(false)
> +  { }
> +
> +  ServiceWorkerInfo* Newest()

It's not clear to me what guarantees proper lifetime management of the objects we return here.

Nit: this method can be const.

@@ +120,5 @@
> +   * RuntimeService for execution and lifetime management.
> +   */
> +  struct ServiceWorkerDomainInfo
> +  {
> +    nsCString mDomain;

We're storing this twice as well.

@@ +152,5 @@
> +  // bar_worker.js could finish before foo_worker.js, but bar_worker still has
> +  // to be the winning controller.
> +  // FIXME(nsm): Move this into per domain?
> +
> +  static ServiceWorkerManager* GetInstance();

This should return an already_AddRefed.
Attachment #8416031 - Flags: review?(ehsan) → review-
Notes for previous review:
Me and Ehsan agreed that the interpretation of the spec for an existing registration is correct.
The registration's scriptspec is only used for the next update and so should be replaced.

Filed issue on data: URLs and continued to allow it for now.

The XXX comment was meant to be removed from the review and is not pertinent.

Made ServiceWorkerRegistration refcounted.

mScope still has to be stored twice because we only pass the registration around to various consumers and they won't know the scope.

Made ServiceWorkerInfo a value type.

Removed mDomain from ServiceWorkerDomainInfo.

All other suggestions from the review have been incorporated.
Attachment #8416695 - Flags: review?(ehsan)
Comment on attachment 8416695 [details] [diff] [review]
Part 1 - ServiceWorkerManager Register()

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

Looks mostly good, but I have one further question.

::: dom/workers/ServiceWorkerManager.h
@@ +29,5 @@
> + * as the same thing; i.e. "Resolve foo with
> + * _GetNewestWorker(serviceWorkerRegistration)", we represent the description
> + * by this class and spawn a ServiceWorker in the right global when required.
> + */
> +class ServiceWorkerInfo

So, what does this buy us that |typedef nsCString ServiceWorkerInfo;| won't?  Do you expect to extend this in the future?
Flags: needinfo?(nsm.nikhil)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #12)
> Comment on attachment 8416695 [details] [diff] [review]
> Part 1 - ServiceWorkerManager Register()
> 
> Review of attachment 8416695 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks mostly good, but I have one further question.
> 
> ::: dom/workers/ServiceWorkerManager.h
> @@ +29,5 @@
> > + * as the same thing; i.e. "Resolve foo with
> > + * _GetNewestWorker(serviceWorkerRegistration)", we represent the description
> > + * by this class and spawn a ServiceWorker in the right global when required.
> > + */
> > +class ServiceWorkerInfo
> 
> So, what does this buy us that |typedef nsCString ServiceWorkerInfo;| won't?
> Do you expect to extend this in the future?

That's a good point :) I'm not sure. I have a feeling that when we need to do 'versioning' of the scripts this will have to be extended to hold that since we have to disambiguate between

Current worker: /foo/service.js @{last updated 6 days ago}
Waiting worker: /foo/service.js @{last updated yesterday during automatic update}
Installing worker: /foo/service.js @{right now}
Flags: needinfo?(nsm.nikhil)
Comment on attachment 8416695 [details] [diff] [review]
Part 1 - ServiceWorkerManager Register()

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

::: dom/workers/ServiceWorkerManager.h
@@ +40,5 @@
> +  {
> +    return !mScriptSpec.IsVoid();
> +  }
> +
> +  nsCString

Return a const nsCString& to avoid the copy.
Attachment #8416695 - Flags: review?(ehsan) → review+
Sorry for the delay in reviewing the update patch...  This requires too much of my brain for tonight, I'll finish reviewing it tomorrow.
Comment on attachment 8416735 [details] [diff] [review]
Part 3 - Update() algorithm.

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

I haven't reviewed the test yet...

::: dom/workers/ServiceWorkerManager.cpp
@@ +47,5 @@
> +  mPromises.AppendElement(aPromise);
> +}
> +
> +void
> +UpdatePromise::Resolve(const nsACString& aScriptSpec, const nsACString& aScope)

I really wish you didn't call this Resolve... Can you call it ResolveAllPromises or something that looks sufficiently different than Resolve?

@@ +48,5 @@
> +}
> +
> +void
> +UpdatePromise::Resolve(const nsACString& aScriptSpec, const nsACString& aScope)
> +{

Can we assert the expected thread here?

@@ +79,5 @@
> +        // Since ServiceWorkerContainer is only exposed to windows we can be
> +        // certain about this cast.
> +        nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(go);
> +        MOZ_ASSERT(window);
> +        nsRefPtr<DOMError> domError = new DOMError(window, NS_ERROR_FAILURE);

I hear we shouldn't use DOMError these days, check with Boris please?

@@ +91,5 @@
> +}
> +
> +void
> +UpdatePromise::Reject(nsresult aRv)
> +{

Can we assert the expected thread here?

@@ +130,5 @@
> + * the promises.
> + */
> +class ResolvePromisesOnMainThreadRunnable : public nsRunnable
> +{
> +  ServiceWorkerRegistration* mRegistration;

Shouldn't this be a refptr?

@@ +135,5 @@
> +  const nsCString mScriptSpec;
> +public:
> +  ResolvePromisesOnMainThreadRunnable(ServiceWorkerRegistration* aRegistration,
> +                                      const nsACString& aScriptSpec)
> +    : mRegistration(aRegistration), mScriptSpec(aScriptSpec)

Nit: some linebreaks would be nice!

@@ +141,5 @@
> +
> +  NS_IMETHODIMP
> +  Run()
> +  {
> +    nsRefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();

What guarantees swm not being null here?

@@ +149,5 @@
> +};
> +
> +class ResolvePromisesOnWorkerParseRunnable : public WorkerRunnable
> +{
> +  ServiceWorkerRegistration* mRegistration;

refptr?

@@ +161,5 @@
> +  bool
> +  WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate)
> +  {
> +    if (aWorkerPrivate->GetStatus() > Running) {
> +      // parse failure will handle rejecting the promises.

Hmm, not sure if I understand this condition.  Can you elaborate please?

@@ +237,5 @@
>        swm->mDomainMap.Put(domain, domainInfo);
>      }
>  
> +    nsRefPtr<ServiceWorkerRegistration> registration =
> +      domainInfo->GetRegistration(mScope);

Nit: please avoid including whitespace changes alongside real changes in the same patch in the future, that would make it easier on the reviewer.  Thanks!

@@ +391,5 @@
> +  aRegistration->mUpdatePromise->Reject(aRv);
> +}
> +
> +/*
> + * aPromise is the promise that reflects the latest Register() call. Rather

Nit: You meant aUpdateResult.

@@ +431,5 @@
> +  aRegistration->mFetcher = new ServiceWorkerFetcher();
> +  nsresult rv = aRegistration->mFetcher->asyncFetch(aRegistration, aWindow);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    localUpdatePromise->Reject(rv);
> +    return rv;

Here you're leaking localUpdatePromise.  Please see why this wasn't caught by your test and add a test for it.  Also, perhaps stick localUpdatePromise in an nsAutoPtr and forget() it below or something?

@@ +492,5 @@
> +  MOZ_ASSERT(aFetcher);
> +
> +  ServiceWorkerRegistration* registration = aFetcher->mRegistration;
> +  nsRefPtr<ServiceWorkerFetcher> kungFuDeathGrip =
> +    registration->mFetcher.forget();

Why did you call this kungFuDeathGrip?  If you're trying to keep the object alive, the .forget() part doesn't make sense to me.

@@ +496,5 @@
> +    registration->mFetcher.forget();
> +
> +  MOZ_ASSERT(registration->HasUpdatePromise());
> +  if (registration->mUpdatePromise->IsRejected()) {
> +    registration->mUpdatePromise = nullptr;

Who's responsible for freeing mUpdatePromise?

@@ +500,5 @@
> +    registration->mUpdatePromise = nullptr;
> +    return;
> +  }
> +
> +  if (NS_FAILED(aStatus)) {

The spec says:

"if fetching the script fails due to the server returning a 4xx response or a 5xx response, or there is a DNS error, or the connection times out"

I don't understand why we don't want to do this for other errors, so I think what you have here is good, please raise a spec issue about this.

@@ +503,5 @@
> +
> +  if (NS_FAILED(aStatus)) {
> +    RejectUpdatePromiseObservers(registration,
> +                                 NS_ERROR_DOM_NETWORK_ERR);
> +    registration->mUpdatePromise = nullptr;

Ditto.

@@ +506,5 @@
> +                                 NS_ERROR_DOM_NETWORK_ERR);
> +    registration->mUpdatePromise = nullptr;
> +    return;
> +  }
> +

Don't you need to handle the redirect case?

@@ +510,5 @@
> +
> +  ServiceWorkerInfo newest = registration->Newest();
> +  if (newest.IsValid() &&
> +      newest.GetScriptSpec().Equals(registration->mScriptSpec)) {
> +    // FIXME(nsm): Bug 931249 Check byte wise

Hmm, for future reference, doing this on the main thread seems very bad, what if the script is a multi-MB file?  That would be unacceptable especially on b2g.

@@ +521,5 @@
> +                                             getter_AddRefs(worker));
> +
> +  // The spec says to check mUpdatePromiseRejected again, but this whole
> +  // function is synchronous. The actual checks occur in the worker parse
> +  // handler or in ResolveRegisterPromises.

Note that the function won't remain synchronous, see above.

Also, this won't be true in the b2g browser.

@@ +533,5 @@
> +
> +  // If parse succeeds, promises will be resolved with this worker, otherwise
> +  // they'll be rejected by error handler.
> +  // We don't pass the instance of ServiceWorker because each Promise will need
> +  // to create one in it's own window.

Nit: its

::: dom/workers/ServiceWorkerManager.h
@@ +24,5 @@
>  BEGIN_WORKERS_NAMESPACE
>  
>  class ServiceWorker;
>  
> +class UpdatePromise MOZ_FINAL

Some documentation on why this class exists would be nice.

@@ +49,5 @@
> +  } mState;
> +
> +  // XXXnsm: Right now we don't need to support AddPromise() after
> +  // already being resolved (i.e. true Promise-like behaviour).
> +  nsTArray<nsTWeakRef<Promise>> mPromises;

nsTWeakRef!  I guess you learn something new about XPCOM every day!

::: dom/workers/test/serviceworkers/mochitest.ini
@@ +1,5 @@
> +[DEFAULT]
> +support-files =
> +  worker.js
> +  worker2.js
> +  worker3.js

Missing files from the patch.
Attachment #8416735 - Flags: review?(ehsan) → review-
Simple self contained patch for scope globbing.
Patch 6 because Patch 5 is coming up soon.
Attachment #8421085 - Flags: review?(ehsan)
Comment on attachment 8421085 [details] [diff] [review]
Patch 6 - Scope ordering and utility functions.

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

The spec literally seems to use "glob-match" to explain how this works <http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#scope-match-algorithm>.  That should probably be clarified, but your algorithms here make sense to me.  Please file a spec issue?  r=me with that and the below fixed.

::: dom/interfaces/base/nsIServiceWorkerManager.idl
@@ +14,5 @@
>    // Returns a Promise
>    nsISupports unregister(in nsIDOMWindow aWindow, in DOMString aScope);
>  
> +  // Testing
> +  DOMString getScopeForUrl(in DOMString path);

Please rev the uuid for good practice.

::: dom/workers/ServiceWorkerContainer.cpp
@@ +142,5 @@
> +
> +  aRv = swm->GetScopeForUrl(aUrl, aScope);
> +  if (aRv.Failed()) {
> +    return;
> +  }

You don't really need this if block.

::: dom/workers/ServiceWorkerManager.cpp
@@ +928,5 @@
> +{
> +  nsCOMPtr<nsIURI> documentURI = aWindow->GetDocumentURI();
> +  if (!documentURI) {
> +    return nullptr;
> +  }

I think I'd move the null check to the nsIURI* overload to make that a bit more robust and also to simplify the other two overloads.

@@ +999,5 @@
> +/* static */ void
> +ServiceWorkerManager::AddScope(nsTArray<nsCString>& aList, const nsACString& aScope)
> +{
> +  for (uint32_t i = 0; i < aList.Length(); ++i) {
> +    nsCString current = aList[i];

const nsCString&.

::: dom/workers/ServiceWorkerManager.h
@@ +191,5 @@
>     */
>    struct ServiceWorkerDomainInfo
>    {
> +    // Ordered list of scopes for glob matching.
> +    nsTArray<nsCString> mOrderedScopes;

I _think_ that using an array for now is sufficient since we don't expect the scope list to become very big, but if that assumption turns to be incorrect, then the memmoves while inserting stuff in the middle of the array may prove costly, in which case we should consider changing this data structure.  Please at least document the reason why you chose an array.
Attachment #8421085 - Flags: review?(ehsan) → review+
Attached patch Patch 5: Install() algorithm. (obsolete) — — Splinter Review 
Patch 5 because Patch 4 is the error handling one, but it isn't ready for review yet.
Attachment #8423202 - Flags: review?(ehsan)
Makes ServiceWorkerInfo refcounted since a copy no longer makes sense with the array of documents.
Attachment #8423256 - Flags: review?(ehsan)
Comment on attachment 8423202 [details] [diff] [review]
Patch 5: Install() algorithm.

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

I think either Ben or Kyle should take over the rest of the review.

::: dom/workers/ServiceWorkerManager.cpp
@@ +683,5 @@
> +{
> +  nsMainThreadPtrHandle<ServiceWorkerRegistration> mRegistration;
> +
> +public:
> +  FinishInstallRunnable(

Nit: explicit.

@@ +707,5 @@
> +{
> +  nsMainThreadPtrHandle<ServiceWorkerRegistration> mRegistration;
> +
> +public:
> +  CancelServiceWorkerInstallationRunnable(

Nit: explicit.

@@ +734,5 @@
> +
> +public:
> +  NS_DECL_ISUPPORTS_INHERITED
> +
> +  FinishInstallHandler(

Nit: explicit.

@@ +811,5 @@
> +
> +    event->SetTrusted(true);
> +
> +    nsresult rv = target->DispatchDOMEvent(nullptr, event, nullptr, nullptr);
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

I don't think we can assert success here. ;-)

@@ +851,5 @@
> +  if (!r->Dispatch(cx)) {
> +    // XXXnsm, not sure how to handle the error here. It's out of scope of the
> +    // spec and could leave the scope's SW situation in limbo. For now just
> +    // reseting mInstallingWorker.
> +    aRegistration->mInstallingWorker.Invalidate();

I'm not really sure what the right thing to do here is, or whether and how the spec should address this.  Please check with Ben and/or Kyle?

@@ +875,5 @@
> +
> +class ActivationRunnable : public nsRunnable
> +{
> +public:
> +  ActivationRunnable(ServiceWorkerRegistration* aRegistration)

Nit: explicit.
Attachment #8423202 - Flags: review?(ehsan) → feedback+
Attached patch Part 8 - Activate algorithm. (obsolete) — — Splinter Review 
Ben for RuntimeService::CreateServiceWorkerFromLoadInfo()

Ehsan for everything else.
Attachment #8423268 - Flags: review?(ehsan)
Attachment #8423268 - Flags: review?(bent.mozilla)
Attached patch Part 8 - Activate algorithm. (obsolete) — — Splinter Review 
Fixed minor compiler errors.
Attachment #8423282 - Flags: review?(ehsan)
Attachment #8423282 - Flags: review?(bent.mozilla)
Attachment #8423268 - Attachment is obsolete: true
Attachment #8423268 - Flags: review?(ehsan)
Attachment #8423268 - Flags: review?(bent.mozilla)
Ugh, qref isn't cooperating with me. Sorry!
Attachment #8423283 - Flags: review?(ehsan)
Attachment #8423283 - Flags: review?(bent.mozilla)
Attachment #8423282 - Attachment is obsolete: true
Attachment #8423282 - Flags: review?(ehsan)
Attachment #8423282 - Flags: review?(bent.mozilla)
Attachment #8416101 - Attachment is obsolete: true
Attachment #8416101 - Flags: review?(khuey)
Comment on attachment 8423256 [details] [diff] [review]
Part 7 - Documents register themselves with corresponding ServiceWorkerInfo.

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +879,4 @@
>      // FIXME(nsm): Actually update the state of active ServiceWorker instances.
>    }
>  
> +  // XXXnsm, How can I force a move constructor use?

mozilla::Move().

::: dom/workers/ServiceWorkerManager.h
@@ +94,5 @@
>    explicit ServiceWorkerInfo(const nsACString& aScriptSpec)
>      : mScriptSpec(aScriptSpec)
>    { }
> +
> +  ~ServiceWorkerInfo()

Nit: please make the dtor private.

@@ +109,5 @@
> +
> +  void
> +  MaybeStopControlling(nsIDocument* aDoc)
> +  {
> +    mControlledDocuments.RemoveElement(aDoc);

Please assert that the document is in mControlledDocuments.

@@ +170,5 @@
> +    return newest.forget();
> +  }
> +
> +  NS_IMETHODIMP
> +  MaybeStartControlling(nsIDocument* aDoc) {

Nit: please mark these MOZ_OVERRIDE.

::: layout/base/nsDocumentViewer.cpp
@@ +1353,5 @@
> +    // If service workers are available, stop controlling this document.
> +    nsCOMPtr<nsIServiceWorkerManager> swm = do_GetService(SERVICEWORKERMANAGER_CONTRACTID);
> +    if (swm) {
> +      swm->MaybeStopControlling(mDocument);
> +    }

I think nsGlobalWindow::FreeInnerObjects is a better place for this, but please double check with bz.
Attachment #8423256 - Flags: review?(ehsan) → review+
Comment on attachment 8423268 [details] [diff] [review]
Part 8 - Activate algorithm.

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +631,5 @@
>  {
>    AssertIsOnMainThread();
>    // FIXME(nsm): Bug 983497 Fire error on relevant ServiceWorkerContainers.
>  
> +  fprintf(stderr, "\n\n\nNSM PARSEERROR %s\n\n\n", NS_ConvertUTF16toUTF8(aFilename).get());

oops!

@@ +699,5 @@
> +{
> +  nsMainThreadPtrHandle<ServiceWorkerRegistration> mRegistration;
> +
> +public:
> +  FinishActivationRunnable(const nsMainThreadPtrHandle<ServiceWorkerRegistration>& aRegistration)

Nit: explicit.

@@ +898,5 @@
> +  bool
> +  WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate)
> +  {
> +    MOZ_ASSERT(aWorkerPrivate);
> +    if (IsCanceled()) {

Why didn't you have this check in the other patch here?

@@ +1053,2 @@
>  
>    nsresult rv = NS_DispatchToMainThread(r);

Talked to nsm on IRC, I'm reviewing a stale patch!

@@ +1096,5 @@
>    return rv;
>  }
>  
> +NS_IMETHODIMP
> +ServiceWorkerManager::CreateServiceWorkerForRegistration(const nsACString& aScriptSpec,

Please get review from bent/khuey on this too.

::: dom/workers/ServiceWorkerManager.h
@@ +113,5 @@
>      mControlledDocuments.RemoveElement(aDoc);
>    }
> +
> +  void
> +  IsControllingDocuments()

Nit: const.
Attachment #8423268 - Attachment is obsolete: false
Comment on attachment 8423283 [details] [diff] [review]
Part 8 - Activate algorithm.

(Review comments submitted on the stale patch.)
Attachment #8423283 - Flags: review?(ehsan) → review+
Attached patch Patch 5: Install() algorithm. (obsolete) — — Splinter Review 
You should take a look because of all the runnables.
Attachment #8423401 - Flags: review?(khuey)
> 
> @@ +109,5 @@
> > +
> > +  void
> > +  MaybeStopControlling(nsIDocument* aDoc)
> > +  {
> > +    mControlledDocuments.RemoveElement(aDoc);
> 
> Please assert that the document is in mControlledDocuments.

We can't make that assertion. If a document within the scope is Shift+Reloaded, then it is not controlled, in which case removal should only fail silently.

> 
> @@ +170,5 @@
> > +    return newest.forget();
> > +  }
> > +
> > +  NS_IMETHODIMP
> > +  MaybeStartControlling(nsIDocument* aDoc) {
> 
> Nit: please mark these MOZ_OVERRIDE.

Compiler error? This isn't inheriting from anything.

> 
> ::: layout/base/nsDocumentViewer.cpp
> @@ +1353,5 @@
> > +    // If service workers are available, stop controlling this document.
> > +    nsCOMPtr<nsIServiceWorkerManager> swm = do_GetService(SERVICEWORKERMANAGER_CONTRACTID);
> > +    if (swm) {
> > +      swm->MaybeStopControlling(mDocument);
> > +    }
> 
> I think nsGlobalWindow::FreeInnerObjects is a better place for this, but
> please double check with bz.

bz? I want to remove the document from a data structure when it goes away. :ehsan says having it here may interfere with the bfcache. What is the best place to put this code?
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #32)
> > @@ +170,5 @@
> > > +    return newest.forget();
> > > +  }
> > > +
> > > +  NS_IMETHODIMP
> > > +  MaybeStartControlling(nsIDocument* aDoc) {
> > 
> > Nit: please mark these MOZ_OVERRIDE.
> 
> Compiler error? This isn't inheriting from anything.

Not sure what I was smoking.  You're right!
Comment on attachment 8423385 [details] [diff] [review]
Part 3 - Update() algorithm.

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +529,5 @@
> +    return;
> +  }
> +
> +  nsresult promiseRejectError = NS_OK;
> +  if (aStatus == NS_ERROR_REDIRECT_LOOP) {

This doesn't do what you expect it to, this error is set when a channel redirects more than its redirectionLimit as set on nsIHttpChannel.  See <http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsIHttpChannel.idl#126>.
Attachment #8423385 - Flags: review?(ehsan) → review-
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #34)
> Comment on attachment 8423385 [details] [diff] [review]
> Part 3 - Update() algorithm.
> 
> Review of attachment 8423385 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/workers/ServiceWorkerManager.cpp
> @@ +529,5 @@
> > +    return;
> > +  }
> > +
> > +  nsresult promiseRejectError = NS_OK;
> > +  if (aStatus == NS_ERROR_REDIRECT_LOOP) {
> 
> This doesn't do what you expect it to, this error is set when a channel
> redirects more than its redirectionLimit as set on nsIHttpChannel.  See
> <http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/
> nsIHttpChannel.idl#126>.

Should've put in a comment there saying that patch 2 calls setRedirectLimit(0)

https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=984048&attachment=8423313
Comment on attachment 8423385 [details] [diff] [review]
Part 3 - Update() algorithm.

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

Oh OK!  Please add that comment then!
Attachment #8423385 - Flags: review- → review+
The test was also timing out in B2G emulator builds.
Comment on attachment 8423283 [details] [diff] [review]
Part 8 - Activate algorithm.

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

I only looked at the worker pieces and skipped most of ServiceWorkerManager.cpp, I assume ehsan looked that part over already.

::: dom/workers/RuntimeService.cpp
@@ +2195,5 @@
>      WorkerDomainInfo* domainInfo;
>      SharedWorkerInfo* sharedWorkerInfo;
>  
> +    nsCString scriptSpec;
> +    nsresult rv = aLoadInfo.mResolvedScriptURI->GetSpec(scriptSpec);

Please call this before grabbing the lock.

@@ +2219,5 @@
>  
>      created = true;
>    }
>  
> +  nsCOMPtr<nsPIDOMWindow> window = aLoadInfo.mWindow;

Why is an extra comptr needed? Can't you just pass aLoadInfo.mWindow to the constructor?

::: dom/workers/RuntimeService.h
@@ +157,5 @@
>                        ServiceWorker** aServiceWorker);
>  
> +  nsresult
> +  CreateServiceWorkerFromLoadInfo(JSContext* aCx,
> +                                  WorkerPrivate::LoadInfo aLoadInfo,

Please make this a const reference.

@@ +303,5 @@
>                               SharedWorker** aSharedWorker);
> +
> +  nsresult
> +  CreateSharedWorkerFromLoadInfo(JSContext* aCx,
> +                                 WorkerPrivate::LoadInfo aLoadInfo,

Here too.

::: dom/workers/ServiceWorkerManager.cpp
@@ +1127,5 @@
> +
> +  AutoSafeJSContext cx;
> +
> +  nsRefPtr<ServiceWorker> serviceWorker;
> +  RuntimeService* rs = RuntimeService::GetService();

This should be GetOrCreateService

@@ +1132,5 @@
> +  if (!rs) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  rv = rs->CreateServiceWorkerFromLoadInfo(cx, info, NS_ConvertUTF8toUTF16(aScriptSpec), aScope,

Nit: This looks longer than 80 chars
Attachment #8423283 - Flags: review?(bent.mozilla) → review+
So the problem was sync pref setters from SpecialPowers don't work in e10s/b2g. Switched to using pushPrefEnv. Patch 1 again.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9d19364fe875
Comment on attachment 8423313 [details] [diff] [review]
Part 2 - Fetch ServiceWorker script without spinning a worker.

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

I think this code should live in the ScriptLoader.  It looks like there's a lot of duplication.

It's also extremely difficult to review these patches without knowing what all the commented out things will do ...

::: dom/workers/ServiceWorkerUtil.cpp
@@ +19,5 @@
> +
> +NS_IMETHODIMP
> +ServiceWorkerFetcher::asyncFetch(ServiceWorkerRegistration* aRegistration,
> +                                 nsPIDOMWindow* aWindow)
> +{

Assert that we're on the main thread.

@@ +27,5 @@
> +#ifdef DEBUG
> +  // Strictly a one time use class!
> +  MOZ_ASSERT(!mUsed);
> +  mUsed = true;
> +#endif

This seems like an odd thing to be so concerned about that you add a DebugOnly member to the class.

@@ +44,5 @@
> +  nsCOMPtr<nsIIOService> ios(do_GetIOService());
> +
> +  nsresult rv;
> +  nsCOMPtr<nsIURI> uri;
> +  rv =

declare nsresult rv here

@@ +50,5 @@
> +                                              NS_ConvertUTF8toUTF16(aRegistration->mScriptSpec),
> +                                              doc, nullptr);
> +  if (NS_FAILED(rv)) {
> +    return NS_ERROR_DOM_SYNTAX_ERR;
> +  }

NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_SYNTAX_ERR);

@@ +52,5 @@
> +  if (NS_FAILED(rv)) {
> +    return NS_ERROR_DOM_SYNTAX_ERR;
> +  }
> +
> +  {

Why the nested scope?

@@ +64,5 @@
> +                                   secMan);
> +
> +    if (NS_FAILED(rv) || NS_CP_REJECTED(shouldLoad)) {
> +      if (NS_FAILED(rv) || shouldLoad != nsIContentPolicy::REJECT_TYPE) {
> +        return rv = NS_ERROR_CONTENT_BLOCKED;

what is this trying to do?

@@ +66,5 @@
> +    if (NS_FAILED(rv) || NS_CP_REJECTED(shouldLoad)) {
> +      if (NS_FAILED(rv) || shouldLoad != nsIContentPolicy::REJECT_TYPE) {
> +        return rv = NS_ERROR_CONTENT_BLOCKED;
> +      }
> +      return rv = NS_ERROR_CONTENT_BLOCKED_SHOW_ALT;

and here?

@@ +74,5 @@
> +  nsCString scheme;
> +  rv = uri->GetScheme(scheme);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }

NS_ENSURE_SUCCESS(rv, rv)

@@ +83,5 @@
> +  // creator.)
> +  rv = doc->NodePrincipal()->CheckMayLoad(uri, false, true);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return NS_ERROR_DOM_SECURITY_ERR;
> +  }

and here

@@ +85,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return NS_ERROR_DOM_SECURITY_ERR;
> +  }
> +
> +  // FIXME(nsm): Verify CSP.

that would be nice.

@@ +99,5 @@
> +
> +  nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(channel);
> +  if (!httpChannel) {
> +    return NS_ERROR_FAILURE;
> +  }

Why are we only allowing http when you said above that we're allowing data URLs/etc?

@@ +102,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  // Prevent redirects.
> +  httpChannel->SetRedirectionLimit(0);

Is that in the spec?  Seems quite odd.

@@ +151,5 @@
> +      //                                                  aStringLen, aString);
> +    }
> +  }
> +
> +  return NS_OK;

If the SWM needs to store the data then it should adopt the buffer here to ensure we don't make unnecessary copies.  See nsScriptLoader::OnStreamComplete.

::: dom/workers/ServiceWorkerUtil.h
@@ +53,5 @@
> +#endif
> +  { }
> +
> +  virtual ~ServiceWorkerFetcher()
> +  { }

Refcounted objects should have protected or private dtors.

@@ +62,5 @@
> +    mAborted = true;
> +  }
> +
> +  NS_IMETHOD
> +  asyncFetch(ServiceWorkerRegistration* aRegistration, nsPIDOMWindow* aWindow);

AsyncFetch.

And why is this NS_IMETHOD?
Attachment #8423313 - Flags: review?(khuey) → review-
Comment on attachment 8423401 [details] [diff] [review]
Patch 5: Install() algorithm.

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

Given the number of comments with parts left to implement it's not clear to me how I could r+ this ...

::: dom/workers/ServiceWorkerManager.cpp
@@ +674,5 @@
>  
>    RejectUpdatePromiseObservers(registration, errorObject);
>  }
>  
> +class FinishInstallRunnable : public nsRunnable

MOZ_FINAL

@@ +687,5 @@
> +    MOZ_ASSERT(!NS_IsMainThread());
> +  }
> +
> +  NS_IMETHODIMP
> +  Run()

NS_IMETHOD
Run() MOZ_OVERRIDE

@@ +691,5 @@
> +  Run()
> +  {
> +    AssertIsOnMainThread();
> +
> +    // FinishInstall takes ownership of the passed info.

This comment does not match your refcounting.

@@ +698,5 @@
> +    return NS_OK;
> +  }
> +};
> +
> +class CancelServiceWorkerInstallationRunnable : public nsRunnable

MOZ_FINAL

@@ +709,5 @@
> +    : mRegistration(aRegistration)
> +  {
> +  }
> +
> +  NS_IMETHODIMP

NS_IMETHOD

@@ +723,5 @@
> +
> +/*
> + * Used to handle InstallEvent::waitUntil() and proceed with installation.
> + */
> +class FinishInstallHandler : public PromiseNativeHandler

MOZ_FINAL

@@ +728,5 @@
> +{
> +  nsMainThreadPtrHandle<ServiceWorkerRegistration> mRegistration;
> +
> +public:
> +  NS_DECL_ISUPPORTS_INHERITED

What's the point of this?

@@ +739,5 @@
> +  }
> +
> +  virtual
> +  ~FinishInstallHandler()
> +  { }

Refcounted objects should have private dtors.

@@ +796,5 @@
> +  DispatchInstallEvent(JSContext* aCx, WorkerPrivate* aWorkerPrivate)
> +  {
> +    aWorkerPrivate->AssertIsOnWorkerThread();
> +    MOZ_ASSERT(aWorkerPrivate->IsServiceWorker());
> +    nsRefPtr<EventTarget> target = do_QueryObject(aWorkerPrivate->GlobalScope());

Why QI?  This is a straightforward downcast.  The compiler will do it implicitly.

@@ +798,5 @@
> +    aWorkerPrivate->AssertIsOnWorkerThread();
> +    MOZ_ASSERT(aWorkerPrivate->IsServiceWorker());
> +    nsRefPtr<EventTarget> target = do_QueryObject(aWorkerPrivate->GlobalScope());
> +
> +    // FIXME(nsm): Set activeWorker to the correct thing.

That would be nice, yes.

@@ +856,5 @@
> +  AutoSafeJSContext cx;
> +  if (!r->Dispatch(cx)) {
> +    // XXXnsm, not sure how to handle the error here. It's out of scope of the
> +    // spec and could leave the scope's SW situation in limbo. For now just
> +    // reseting mInstallingWorker.

Dispatch should not fail unless the worker is shutting down.

@@ +864,5 @@
> +
> +  // From this point on, although we've lost references to the ServiceWorker,
> +  // which means the underlying WorkerPrivate has no references, the worker
> +  // will stay alive due to the modified busy count until the install event has
> +  // been dispatched.

How did we lose it?  Where did it go?

@@ +873,5 @@
> +  // gated on those actions. I (nsm) am not sure if it is worth requiring
> +  // a special spec mention saying the install event should keep the worker
> +  // alive indefinitely purely on the basis of calling waitUntil(), since
> +  // a wait is likely to be required only when performing networking or storage
> +  // transactions in the first place.

I'll try to find some time to read the spec and figure out what we should do there.

@@ +911,5 @@
> +
> +  nsresult rv = NS_DispatchToMainThread(r);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    // FIXME(nsm): Handle error.
> +  }

In practice this never fails.
Attachment #8423401 - Flags: review?(khuey)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #44)
> Comment on attachment 8423313 [details] [diff] [review]
> Part 2 - Fetch ServiceWorker script without spinning a worker.
> 
> Review of attachment 8423313 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this code should live in the ScriptLoader.  It looks like there's a
> lot of duplication.
> 
> It's also extremely difficult to review these patches without knowing what
> all the commented out things will do ...
> 

Sorry about the quite terrible patch. So some of the stuff that the ServiceWorker script caching needs is going to have to happen on the chrome process since cache2 is only available there, as are the crypto primitives to hash the script content.

Do you have any thoughts on how to integrate ScriptLoader with that? Would it be better to have ScriptLoader send some info to the channel about it loading a SW (not sure how to do that), and have a lower level caching in the http channel or in a custom CacheStorage backend?
Flags: needinfo?(khuey)
Ignore fragment in scope URL as in current spec.
Attachment #8440017 - Flags: review?(ehsan)
Changed to use ScriptLoader for now. This means we don't satisfy any of the non-heuristic caching requirements for now, but it makes the code cleaner and keeps this patch very contained. Ideally that means khuey will r+ it and allow landing the rest of the patches.

ehsan, sorry I couldn't get an interdiff this time. I hope bugzilla will do it for you. Not much has changed in the update algorithm itself, just changed some code paths that call FinishFetch(), and now everything that is used is in the tree.
Attachment #8440085 - Flags: review?(khuey)
Attachment #8440085 - Flags: review?(ehsan)
khuey for WorkerPrivate and dom/promise changes.
ehsan for HandleError and other ServiceWorkerManager changes + tests.
Attachment #8440134 - Flags: review?(khuey)
Attachment #8440134 - Flags: review?(ehsan)
Attachment #8440017 - Flags: review?(ehsan) → review+
Comment on attachment 8440085 [details] [diff] [review]
Part 3 - Update algorithm.

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +155,5 @@
> +// Allows newer calls to Update() to 'abort' older calls.
> +// Each call to Update() creates the instance which handles creating the
> +// worker and queues up a runnable to resolve the update promise once the
> +// worker has successfully been parsed.
> +class ServiceWorkerUpdateInstance MOZ_FINAL : public nsISupports

You don't need to make this an nsISupports, you can just use the inline refcounting macros.

@@ +160,5 @@
> +{
> +  // Owner of this instance.
> +  ServiceWorkerRegistration* mRegistration;
> +  nsCString mScriptSpec;
> +  nsPIDOMWindow* mWindow;

What guarantees the window pointer to remain valid as long as this object is alive?  Why not make this a strong ref?

@@ +169,5 @@
> +
> +  ServiceWorkerUpdateInstance(ServiceWorkerRegistration *aRegistration,
> +                              nsPIDOMWindow* aWindow)
> +    : mRegistration(aRegistration),
> +     // Capture the current script spec in case register() get's called.

Nit: indentation, and gets.
Attachment #8440085 - Flags: review?(ehsan)
Comment on attachment 8440134 [details] [diff] [review]
Patch 4 - Handle parse and uncaught errors.

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +645,5 @@
> +
> +  nsCOMPtr<nsIURI> uri;
> +  nsresult rv = NS_NewURI(getter_AddRefs(uri), aScope, nullptr, nullptr);
> +  // Since the scope is 'absolute', this better work.
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));

This looks very hairy.  The comment above doesn't make me feel any better.  Why won't you do proper error checking here?

@@ +680,5 @@
> +
> +  // If the worker was the one undergoing registration, we reject the promises,
> +  // otherwise we fire events on the ServiceWorker instances.
> +
> +  // If their is an update in progress and the worker that errored is the same one

Nit: there.

::: dom/workers/test/serviceworkers/test_installation_simple.html
@@ +99,5 @@
> +    var p = navigator.serviceWorker.register("parse_error_worker.js");
> +    return p.then(function(w) {
> +      ok(false, "Registration should fail with parse error");
> +    }, function(e) {
> +    info(e + " " + e.name + " " + e.message);

Nit: indentation.
Attachment #8440134 - Flags: review?(ehsan) → review+
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #50)
> Comment on attachment 8440085 [details] [diff] [review]
> Part 3 - Update algorithm.
> 
> Review of attachment 8440085 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/workers/ServiceWorkerManager.cpp
> @@ +155,5 @@
> > +// Allows newer calls to Update() to 'abort' older calls.
> > +// Each call to Update() creates the instance which handles creating the
> > +// worker and queues up a runnable to resolve the update promise once the
> > +// worker has successfully been parsed.
> > +class ServiceWorkerUpdateInstance MOZ_FINAL : public nsISupports
> 
> You don't need to make this an nsISupports, you can just use the inline
> refcounting macros.

nsMainThreadPtrHolder<> requires it to be a nsISupports so that a release can be done on the main thread.
(In reply to comment #53)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #50)
> > Comment on attachment 8440085 [details] [diff] [review]
> > Part 3 - Update algorithm.
> > 
> > Review of attachment 8440085 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/workers/ServiceWorkerManager.cpp
> > @@ +155,5 @@
> > > +// Allows newer calls to Update() to 'abort' older calls.
> > > +// Each call to Update() creates the instance which handles creating the
> > > +// worker and queues up a runnable to resolve the update promise once the
> > > +// worker has successfully been parsed.
> > > +class ServiceWorkerUpdateInstance MOZ_FINAL : public nsISupports
> > 
> > You don't need to make this an nsISupports, you can just use the inline
> > refcounting macros.
> 
> nsMainThreadPtrHolder<> requires it to be a nsISupports so that a release can
> be done on the main thread.

I thought it just requires your type to have an AddRef and Release method. :/
Asking for review on updated patch which removes the bits that were creating a worker to pass to the currently installing worker. Creating Service/SharedWorkers on a worker thread triggers a lot of assertions right now and in any case will require some changes. I've added the bug number to that comment. Any FIXME comments that don't have bug numbers I intend to fix in this bug itself.
Attachment #8441800 - Flags: review?(khuey)
Comment on attachment 8423256 [details] [diff] [review]
Part 7 - Documents register themselves with corresponding ServiceWorkerInfo.

Marking as obsolete since this incorrectly implements some logic.

By having registrations track documents, we lose the ability to quickly associate documents -> registration and instead rely on a scope match every time. But this is incorrect since the spec requires that a document be associated with a particular registration for its lifetime. This causes inconsistencies like:

1) index.html register("a.js", { scope: "*" })
2) open foo/bar.html (now controlled by a.js)
3) foo/bar.html register("b.js", { scope: "foo/*" })
4) close foo/bar.html. Now we lookup by scope and see that b.js is the controller, and try to remove from its current worker's nsTArray, where obviously it isn't present.
Attachment #8423256 - Attachment is obsolete: true
Comment on attachment 8440085 [details] [diff] [review]
Part 3 - Update algorithm.

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +145,5 @@
> +      return true;
> +    }
> +
> +    nsRefPtr<FinishFetchOnMainThreadRunnable> r =
> +      new FinishFetchOnMainThreadRunnable(mUpdateInstance);

Just use NS_NewRunnableMethod here.

@@ +163,5 @@
> +  nsCString mScriptSpec;
> +  nsPIDOMWindow* mWindow;
> +
> +  bool mAborted;
> +public:

nit \n before public

@@ +197,5 @@
> +                                                    mScriptSpec,
> +                                                    mRegistration->mScope,
> +                                                    getter_AddRefs(serviceWorker));
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      swm->RejectUpdatePromiseObservers(mRegistration, rv);

Seems like RAII might be useful here.

@@ +211,5 @@
> +
> +    AutoSafeJSContext cx;
> +    if (!r->Dispatch(cx)) {
> +      swm->RejectUpdatePromiseObservers(mRegistration, rv);
> +    }

and here.
Attachment #8440085 - Flags: review?(khuey) → review+
Comment on attachment 8440134 [details] [diff] [review]
Patch 4 - Handle parse and uncaught errors.

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

nsScriptError objects are not be accessible from content, so this won't work.
Attachment #8440134 - Flags: review?(khuey) → review-
Comment on attachment 8441800 [details] [diff] [review]
Patch 5: Install() algorithm.

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

Please fix the commit message.

::: dom/workers/ServiceWorkerManager.cpp
@@ +925,5 @@
> +  aRegistration->mInstallingWorker.Invalidate();
> +
> +  // FIXME(nsm): Actually update state of active ServiceWorker instances to
> +  // installed.
> +  // FIXME(nsllm): Fire statechange on the instances.

nsm? :)
Attachment #8441800 - Flags: review?(khuey) → review+
Kyle for worker error handling
Jason for the changes to JSAPI.
The CreateError() function is used to allow creating an Error object from error information to reject a Promise with.
Attachment #8447226 - Flags: review?(khuey)
Attachment #8447226 - Flags: review?(jorendorff)
Comment on attachment 8447226 [details] [diff] [review]
Patch 4 - Handle parse and uncaught errors.

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

::: js/src/jsapi.h
@@ +4612,5 @@
>  
> +extern JS_PUBLIC_API(bool)
> +CreateError(JSContext *cx, HandleString stack, HandleString fileName,
> +            uint32_t lineNumber, uint32_t columnNumber, JSErrorReport *report,
> +            HandleString message, MutableHandleValue rval);

Instead of adding a separate API, please just add a JSExnType argument to the existing API and rename it to JS::CreateError. You'll have to update both existing call sites (there are only two).

r=me with that. Thanks.
Attachment #8447226 - Flags: review?(jorendorff) → review+
Comment on attachment 8447226 [details] [diff] [review]
Patch 4 - Handle parse and uncaught errors.

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

::: dom/promise/Promise.h
@@ +16,5 @@
>  #include "mozilla/dom/ToJSValue.h"
>  #include "nsWrapperCache.h"
>  #include "nsAutoPtr.h"
>  #include "js/TypeDecls.h"
> +#include "mozilla/dom/ErrorEventBinding.h"

Forward declare this and drop the header include.

::: dom/workers/ServiceWorkerManager.cpp
@@ +131,5 @@
> +      MOZ_ASSERT(go);
> +
> +      AutoSafeJSContext cx;
> +      JS::Rooted<JSObject*> jsGlobal(cx, go->GetGlobalJSObject());
> +      JSAutoCompartment ac(cx, jsGlobal);

You should probably just use AutoJSAPI here.  It even has an Init method that takes an nsIGlobalObject IIRC.

@@ +137,5 @@
> +      JS::Rooted<JSString*> stack(cx, JS_GetEmptyString(JS_GetRuntime(cx)));
> +
> +      JS::Rooted<JS::Value> fnval(cx);
> +      xpc::StringToJsval(cx, aErrorDesc.mFilename, &fnval);
> +      JS::Rooted<JSString*> fn(cx, fnval.toString());

Use ToJSValue.

@@ +141,5 @@
> +      JS::Rooted<JSString*> fn(cx, fnval.toString());
> +
> +      JS::Rooted<JS::Value> msgval(cx);
> +      xpc::StringToJsval(cx, aErrorDesc.mMessage, &msgval);
> +      JS::Rooted<JSString*> msg(cx, msgval.toString());

Here too.
Attachment #8447226 - Flags: review?(khuey) → review+
The registration maintains a count of documents that it controls and the hashtable in ServiceWorkerDomainInfo tracks them via RAII.
Attachment #8448267 - Flags: review?(ehsan)
Trying again, patch 4 & 5. The problem was that CreateSharedWorkerFromLoadInfo() would get the window from the loadinfo, but getting it after the worker script has already loaded results in a nullptr since the worker resets it (since it is no longer needed). Now the method stores a pointer to the window before creating the WorkerPrivate.

https://hg.mozilla.org/integration/mozilla-inbound/rev/30fa36882d57
https://hg.mozilla.org/integration/mozilla-inbound/rev/c18211a2afa1
Let's try once again. Patch 4 & 5 with followup from comment 72 folded. GC Hazard fixed by rooting the ErrorEventInit. That try run is at https://tbpl.mozilla.org/?tree=Try&rev=ba58f609f691

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/083d3ccb9b43
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9d000928d525
Flags: needinfo?(nsm.nikhil)
Comment on attachment 8448267 [details] [diff] [review]
Part 7 - Documents register themselves with corresponding ServiceWorkerInfo.

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

How are you planning to handle documents living in the bf-cache?  Can we bf-cache documents if they're tied to a SW?  (I suspect the answer is yes)

::: content/base/src/nsDocument.cpp
@@ +2607,5 @@
>      NS_ENSURE_SUCCESS(rv, rv);
>    }
>  
> +  // If we are shift-reloaded, don't associate with a ServiceWorker.
> +  nsLoadFlags loadFlags;

Please initialize loadFlags to 0 so that you don't have to do error checking below.

@@ +2610,5 @@
> +  // If we are shift-reloaded, don't associate with a ServiceWorker.
> +  nsLoadFlags loadFlags;
> +  mChannel->GetLoadFlags(&loadFlags);
> +  if (loadFlags & nsIRequest::LOAD_BYPASS_CACHE) {
> +    fprintf(stderr, "NSM Page was shift reloaded, skipping control");

NS_WARNING please.  We shouldn't print this to stderr in non-debug builds!

@@ +2616,5 @@
> +  }
> +
> +  nsCOMPtr<nsIServiceWorkerManager> swm = do_GetService(SERVICEWORKERMANAGER_CONTRACTID);
> +  if (swm) {
> +    swm->MaybeStartControlling(this);

I think this is the right place for this, but Boris should check it over.

::: dom/base/nsGlobalWindow.cpp
@@ +1598,5 @@
>  
> +    // If service workers are available, stop controlling this document.
> +    nsCOMPtr<nsIServiceWorkerManager> swm = do_GetService(SERVICEWORKERMANAGER_CONTRACTID);
> +    if (swm) {
> +      swm->MaybeStopControlling(mDoc);

This too.

::: dom/workers/ServiceWorkerManager.cpp
@@ +1145,5 @@
> +
> +  nsRefPtr<ServiceWorkerRegistration> registration =
> +    GetServiceWorkerRegistration(aDoc);
> +  if (registration) {
> +    domainInfo->mControlledDocuments.Put(aDoc, new AutoServiceWorkerRegistrationCounter(registration));

You should probably assert that the entry doesn't already exist in the hashtable.

@@ +1161,5 @@
> +  if (!domainInfo) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  if (domainInfo->mControlledDocuments.Contains(aDoc)) {

What could cause Contains to return false here?

::: dom/workers/ServiceWorkerManager.h
@@ +152,5 @@
> +    return newest.forget();
> +  }
> +
> +  bool
> +  IsControllingDocuments()

Nit: const.

@@ +163,5 @@
> +{
> +  ServiceWorkerRegistration* mRegistration;
> +
> +public:
> +  AutoServiceWorkerRegistrationCounter(ServiceWorkerRegistration* aRegistration)

Nit: explicit.

@@ +225,5 @@
>  
>      // Scope to registration.
>      nsRefPtrHashtable<nsCStringHashKey, ServiceWorkerRegistration> mServiceWorkerRegistrations;
>  
> +    nsClassHashtable<nsISupportsHashKey, AutoServiceWorkerRegistrationCounter> mControlledDocuments;

I'd tie the hashtable entry to the registration.
Attachment #8448267 - Flags: review?(ehsan) → review-
Made changes requested in comment 76.
I don't understand the bf-cache interactions this will require, so I'll get back to it after the long weekend.
Boris, meanwhile could you answer if the places I've instrumented nsDocument and nsGlobalWindow are the right places?
Comment on attachment 8450479 [details] [diff] [review]
Part 7 - Documents register themselves with corresponding ServiceWorkerInfo.

Hmm.  Why do we want to skip service workers for shift-reload?  And if we do, is LOAD_BYPASS_CACHE the right thing to be checking?

StartDocumentLoad can be (and is) called for documents without a browsing context.  Do we want to MaybeStartControlling for those?  And if we do, what will MaybeStopControlling?

Do we really want to start controlling in document code but stop in window code?  Why not do both in document code?
Attachment #8450479 - Flags: feedback?(bzbarsky) → feedback+
(In reply to Boris Zbarsky [:bz] from comment #79)
> Comment on attachment 8450479 [details] [diff] [review]
> Part 7 - Documents register themselves with corresponding ServiceWorkerInfo.
> 
> Hmm.  Why do we want to skip service workers for shift-reload?  And if we
> do, is LOAD_BYPASS_CACHE the right thing to be checking?
That's required by the spec to allow bypassing the serviceworker and have a full load from the network/http cache.

> 
> StartDocumentLoad can be (and is) called for documents without a browsing
> context.  Do we want to MaybeStartControlling for those?  And if we do, what
> will MaybeStopControlling?

Will nsDocumentViewer still be involved. What is the best place to put the call in otherwise?
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #76)
> Comment on attachment 8448267 [details] [diff] [review]
> Part 7 - Documents register themselves with corresponding ServiceWorkerInfo.
> 
> Review of attachment 8448267 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> How are you planning to handle documents living in the bf-cache?  Can we
> bf-cache documents if they're tied to a SW?  (I suspect the answer is yes)

From what I understand of the bf-cache, this should just work right? Since the documents are only frozen/thawed, and not created or destroyed.
Flags: needinfo?(ehsan)
> That's required by the spec 

How can the spec possibly require something from hidden UI like shift-reload?

Can you please point me to the relevant spec requirement?

> Will nsDocumentViewer still be involved.

It can be.  For example SVG resource documents or svg-as-image.  Or it might not be: for example XHR.  

> What is the best place to put the call in otherwise?

I'm not sure when exactly we want to make the call, so far.  What does the spec say?
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #81)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #76)
> > Comment on attachment 8448267 [details] [diff] [review]
> > Part 7 - Documents register themselves with corresponding ServiceWorkerInfo.
> > 
> > Review of attachment 8448267 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > How are you planning to handle documents living in the bf-cache?  Can we
> > bf-cache documents if they're tied to a SW?  (I suspect the answer is yes)
> 
> From what I understand of the bf-cache, this should just work right? Since
> the documents are only frozen/thawed, and not created or destroyed.

What do you mean by "work"?  The behavior that we want is not entirely clear to me.
Flags: needinfo?(ehsan)
(In reply to Boris Zbarsky [:bz] from comment #82)
> > That's required by the spec 
> 
> How can the spec possibly require something from hidden UI like shift-reload?
> 
> Can you please point me to the relevant spec requirement?
http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#on-fetch-request-algorithm, step 7.
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #83)
> (In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #81)
> > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> > #76)
> > > Comment on attachment 8448267 [details] [diff] [review]
> > > Part 7 - Documents register themselves with corresponding ServiceWorkerInfo.
> > > 
> > > Review of attachment 8448267 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > How are you planning to handle documents living in the bf-cache?  Can we
> > > bf-cache documents if they're tied to a SW?  (I suspect the answer is yes)
> > 
> > From what I understand of the bf-cache, this should just work right? Since
> > the documents are only frozen/thawed, and not created or destroyed.
> 
> What do you mean by "work"?  The behavior that we want is not entirely clear
> to me.

From what I understand of bfcache and its relation to processing stuff on the page (i.e. scripts aren't run again when pages are loaded from bfcache), I don't think the SW will play any role, and so shouldn't require any special code to load/unload/renavigate/refetch resources when the document is restored from bfcache. I could be entirely wrong.
Hrm.  That's pretty bad, because different browsers have different heuristics for when to do a forced refresh...
How about nsDocument::StartDocumentLoad() to start controlling and nsDocument::Destroy() to stop?
We should keep this check in for now (mainly so devs can have an escape hatch when playing with this in Nightly) and see where the spec goes. I really want to get a ton of patches out of my queue and available for devs to play with.
Flags: needinfo?(bzbarsky)
What actual behavior are we aiming for?  Destroy() will get called for things like documents in docshells (though only when dropping from bfcache; is the timing there correct?) and for svg images (I think), but not for XHR, say.
Flags: needinfo?(bzbarsky)
Comment on attachment 8454648 [details] [diff] [review]
Part 7 - Documents register themselves with corresponding ServiceWorkerInfo.

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

::: dom/base/nsGlobalWindow.cpp
@@ +103,5 @@
>  #include "nsISelection.h"
>  #include "nsIPrompt.h"
>  #include "nsIPromptService.h"
>  #include "nsIPromptFactory.h"
> +#include "nsIServiceWorkerManager.h"

I'll get rid of this when landing.
(In reply to Boris Zbarsky [:bz] from comment #88)
> What actual behavior are we aiming for?  Destroy() will get called for
> things like documents in docshells (though only when dropping from bfcache;
> is the timing there correct?) and for svg images (I think), but not for XHR,
> say.

If documents from XHR don't really do network resource fetching and script execution and so on, then it is perfectly fine for ServiceWorkers to completely ignore them. What would happen if the contents of these documents were actually inserted into a iframe or similar? Would these code paths of StartDocumentLoad() and Destroy() be executed for documents. ServiceWorkers should be involved whenever the document may make network requests that should be intercepted by the ServiceWorker. So if in the case of an XHR document, it can't do fetches, then its fine for ServiceWorkers not to be involved.
> If documents from XHR don't really do network resource fetching and script execution

They don't.  SVG resource documents kinda do, for network resource fetching.  Sorta.  But those should presumably use the display document's service worker anyway.

> then it is perfectly fine for ServiceWorkers to completely ignore them.

I assume that means not registering at all, yes?  That means detecting what sort of document you have in StartDocumentLoad....

Alternately, perhaps, you could do it in SetScriptGlobalObject?  I _think_ that should only happen for documents in Windows.

> What would happen if the contents of these documents were actually inserted into a iframe
> or similar? 

Can't do that with document, thankfully.  At least so far.
(In reply to Boris Zbarsky [:bz] from comment #93)
> > If documents from XHR don't really do network resource fetching and script execution
> 
> They don't.  SVG resource documents kinda do, for network resource fetching.
> Sorta.  But those should presumably use the display document's service
> worker anyway.

Yes.

> 
> > then it is perfectly fine for ServiceWorkers to completely ignore them.
> 
> I assume that means not registering at all, yes?  That means detecting what
> sort of document you have in StartDocumentLoad....
> 
> Alternately, perhaps, you could do it in SetScriptGlobalObject?  I _think_
> that should only happen for documents in Windows.

Yes, I mean not registering at all. SetScriptGlobalObject() sounds like a good entry point.
But is that call done before we start to process resource loads on the document? If we aggressively start fetching resources the document refers to (links to CSS in the <head> or something) before we set a script global for the document, then it wouldn't work.
Landed part 6. There are occasional try failures, but I believe they are related to Bug 1031928 and Bug 1032610 and not this patches fault.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0bc1595ee11b
> But is that call done before we start to process resource loads on the document

I believe so, yes....
nsDocumentViewer::Close() calls SetScriptGlobalObject(nullptr), leading to hitting the Contains() assertion in MaybeStartControlling.
Added a check to ensure a non-null global object is passed. Would it be nicer to have a boolean that is flipped the first time MaybeStartControlling is called in nsDocument instead? Like nsDocument::mMaybeStartControllingCalled?
Attachment #8454827 - Flags: review?(ehsan)
Attachment #8454703 - Attachment is obsolete: true
Attachment #8454703 - Flags: review?(ehsan)
Comment on attachment 8454827 [details] [diff] [review]
Part 7 - Documents register themselves with corresponding ServiceWorkerInfo

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

Boris is probably a better reviewer for this at this point.  Sorry, Boris!
Attachment #8454827 - Flags: review?(ehsan) → review?(bzbarsky)
Comment on attachment 8454827 [details] [diff] [review]
Part 7 - Documents register themselves with corresponding ServiceWorkerInfo

>+++ b/content/base/src/nsDocument.cpp
>@@ -4457,16 +4458,35 @@ nsDocument::SetScriptGlobalObject(nsIScr

OK, so doing MaybeStartControlling in this function means we'll call it when the document becomes the active document in the docshell.

But we call MaybeStopControlling when the document is torn down.  I _think_ that means that the MOZ_ASSERT(!domainInfo->mControlledDocuments.Contains(aDoc)) in ServiceWorkerManager::MaybeStartControlling can be triggered.  It goes like so:

1)  Load a document.  This calls SetScriptGlobalObject, hance MaybeStartControlling.
2)  Load another document, have the first one go into bfcache.  First document gets a
    SetScriptGlobalObject(nullptr) but _not_ a Destroy call.
3)  Hit the back button to bring first document out of bfcache.  This will call
    SetScriptGlobalObject(nonnull) again, and trigger a second MaybeStartControlling.

One simple option might be to track a boolean for whether we've added outselves already...

>+    if (loadFlags & nsIRequest::LOAD_BYPASS_CACHE) {

I'm not happy with the part of the spec you linked me to, because it never defines "is triggered by a force-refresh".  In Gecko, for example, a force-refresh on a document will load subresources that start loading before onload with the LOAD_BYPASS_CACHE flag, but not do that for the ones after onload, iirc.  I would be somewhat surprised if there is a spec for this so far, and even more surprised if browsers interoperate on it, but serviceworker makes these details _very_ observable.  Please raise a spec issue on this.

>+      swm->MaybeStartControlling(this);

If you plan to ignore the retval anyway, why not just make the method notxpcom (and probably nostdcall) and make it return void?

>+  nsCOMPtr<nsIServiceWorkerManager> swm = do_GetService(SERVICEWORKERMANAGER_CONTRACTID);

Could we throw this into mozilla::services?  Followup ok for this part.

>+++ b/dom/interfaces/base/nsIServiceWorkerManager.idl
> [uuid(6117cdf1-cb10-42a3-9901-4f1bab7ffa4d)]

Need to rev this.

>+  [noscript] void MaybeStartControlling(in nsIDocument aDoc);
>+  [noscript] void MaybeStopControlling(in nsIDocument aDoc);

Needs way more documentation, please, about things like idempotence vs not and which documents this should be called for and the like.

>+++ b/dom/workers/ServiceWorkerManager.cpp
>+    ServiceWorkerManager::ServiceWorkerDomainInfo* domainInfo = swm->GetDomainInfo(mScriptURI);

GetDomainInfo returns an already_AddRefed, no?  How does this work?

> ServiceWorkerManager::GetServiceWorkerRegistration(nsPIDOMWindow* aWindow)
>+  nsCOMPtr<nsIDocument> documentURI = aWindow->GetExtantDoc();

Change the variable name too?

>+ServiceWorkerManager::MaybeStartControlling(nsIDocument* aDoc)
>+    // Use the already_AddRefed<> form of Put.

Document why?

The rest looks fine.  r=me with the above addressed.
Attachment #8454827 - Flags: review?(bzbarsky) → review+
Attached patch ServiceWorkerManager behind pref (obsolete) — — Splinter Review 
Boris,

I'd like to land this along with Patch 7 since otherwise we'll have documents being controlled and uncontrolled unnecessarily.
Would it be better to just #ifndef RELEASE_BUILD on the ServiceWorker code in nsDocument?

What I'm trying to do is to have minimal performance impact on nsDocument loading while ServiceWorkers are disabled.
Attachment #8459375 - Flags: review?(bzbarsky)
Comment on attachment 8459375 [details] [diff] [review]
ServiceWorkerManager behind pref

This will make getService on this service claim NS_ERROR_OUT_OF_MEMORY, but I can live with that.

r=me if the document code is the only place we getService this thing.
Attachment #8459375 - Flags: review?(bzbarsky) → review+
Though... What happens if the pref is toggled while a document exists?  Will we end up in some bizarre frankenstate?
(In reply to Boris Zbarsky [:bz] from comment #106)
> Though... What happens if the pref is toggled while a document exists?  Will
> we end up in some bizarre frankenstate?

The document will simply call MaybeStopControlling(), which doesn't do anything if the document wasn't controlled in the first place. Worst case, the pref will only be preffed by those wanting to use ServiceWorkers, who should expect instability at this point :)
(In reply to Boris Zbarsky [:bz] from comment #105)
> Comment on attachment 8459375 [details] [diff] [review]
> ServiceWorkerManager behind pref
> 
> This will make getService on this service claim NS_ERROR_OUT_OF_MEMORY, but
> I can live with that.
> 
> r=me if the document code is the only place we getService this thing.

Yes, that's the only place for now.
Patch 7 was also pushed with the "ServiceManager behind pref" patch folded.
Try run:
https://tbpl.mozilla.org/?tree=Try&rev=129fc89fd97e
Attachment #8454827 - Attachment is obsolete: false
Attachment #8454827 - Flags: checkin+
Attachment #8421085 - Attachment is obsolete: false
Attachment #8421085 - Flags: checkin+
Comment on attachment 8454827 [details] [diff] [review]
Part 7 - Documents register themselves with corresponding ServiceWorkerInfo

Not sure why bzexport keeps obsoleting unrelated attachments
Attachment #8454827 - Attachment is obsolete: false
The try link in comment 112 still has errors, but those are from test_installation_simple.html and related to a possible worker bug, which I'm waiting for bent to confirm.

The error that is fixed is https://tbpl.mozilla.org/?tree=Try&rev=43feddb52fca OS X 10.8 debug the crash in FinishInstall()

And since I no longer remember why khuey was ni? on this, clearing it.
Flags: needinfo?(khuey)
Attachment #8460701 - Flags: review?(ehsan) → review+
Blocks: 1043004
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: leave-open
Resolution: --- → FIXED
QA Whiteboard: [qa-]
Target Milestone: --- → mozilla34
No longer depends on: 1011268
Service Workers API fully documented at https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorker_API, and could do with a review.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: