Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Description • 11 years ago

ServiceWorker scripts and scripts imported using importScripts() have "always store in offline cache, on reload always fetch from network" semantics [1].

ScriptLoader should be modified to create the channel with appropriate flags. Afaik, Necko does not support a suggestion to always store something in offline cache. I'll update this once I ask the Necko team.

[1]: https://github.com/slightlyoff/ServiceWorker/blob/master/advanced_topics.md

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 1 • 11 years ago

Shouldn't this be a necko bug?

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 2 • 11 years ago

Stores script in appcache, but then can't load it from the network the second time around (always hits appcache). Talking with Necko guys to fix this.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 3 • 11 years ago

Cache ServiceWorkers to disk and load them exclusively from disk when requested.
This should carry over to importScripts() calls automatically, but haven't tested it.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 4 • 10 years ago

Utility class that fetches the worker script.
Using ScriptLoader directly would require spinning up a Worker, which we don't want to do.
This patch does not implement 'byte-wise check and then cache'.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 5 • 10 years ago

Kyle, this is yours to review because it is inspired by ScriptLoader.
It doesn't really do anything useful, I'd like to land it so the other registration patches can land.
I'll come back to this bug (or file a new one) later.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 6 • 10 years ago

Moved attachment to Bug 984048.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 7 • 10 years ago

https://hg.mozilla.org/users/nsm.nikhil_gmail.com/service-workers2/rev/f399569a6fdd

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 8 • 10 years ago

Some PBackground stubs to save work if someone picks this up.

	Andrea Marchesini [:baku]
	Comment 9 • 10 years ago

Just a feedback because the patch is not ready to land yet.

	Andrea Marchesini [:baku]
	Comment 10 • 10 years ago

I'm going to submit a mochitest in a separated patch.

	Ben Kelly [:bkelly, not reviewing]
	Comment 11 • 10 years ago

Comment on attachment 8511065 [details] [diff] [review]
patch

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

This is a good start, but it seems there are still some bits missing.  Also, the CacheStorage API you are using is going to change very shortly.  See the comments below.

Also, this is going to need to go into Maple prior to landing on mozilla-central.  I would prefer to use feedback for now instead of review to reflect that.  Once Cache is stable enough to land in m-c, then we can r+ this.  Does that sound reasonable?

::: dom/workers/ScriptLoader.cpp
@@ +877,5 @@
> +    return rv;
> +  }
> +
> +  ErrorResult error;
> +  mPromise = mCacheStorage->Get(NS_ConvertUTF8toUTF16(spec), error);

So you use the full URL for the Cache key here to provide separation between different ServiceWorker scripts within the same origin?  It seems this is not covered in the spec.  Can you open an issue on the SW github to clarify?

Also, this will not currently work as written.  CacheStorage::Get() does not create a Cache if it is not already there.  You have to do CacheStorage::Create() if Get() resolves to undefined.

Of course, the spec was recently changed to remove Get() and Create() in favor of CacheStorage::Open().  Open() does the get-or-create semantics we want here.  I'm going to implement this in maple soon.

Finally, I don't see any code that actually uses the Cache object that is requested from CacheStorage here.  You need to use Cache::Put() and Cache::Match() to store and retrieve scripts from the cache.

@@ +963,5 @@
> +      break;
> +    }
> +
> +    mBuffer.Append(buffer, ret);
> +  }

Can you just use NS_ConsumeStream() here?  See nsStreamUtils.h.

	Ben Kelly [:bkelly, not reviewing]
	Comment 12 • 10 years ago

I spoke with Jake on IRC and it sounds like we want a separate Cache not just per SW script URL, but per-version of the script.  So we need to append a version string or something to the Key passed to CacheStorage as well.

	Ben Kelly [:bkelly, not reviewing]
	Comment 13 • 10 years ago

See:  https://github.com/slightlyoff/ServiceWorker/issues/473#issuecomment-60401602

	Ben Kelly [:bkelly, not reviewing]
	Comment 14 • 10 years ago

> ::: dom/workers/ScriptLoader.cpp
> @@ +877,5 @@
> > +    return rv;
> > +  }
> > +
> > +  ErrorResult error;
> > +  mPromise = mCacheStorage->Get(NS_ConvertUTF8toUTF16(spec), error);

Also, I just pushed the spec changes to maple, so you can use CacheStorage::Open() here for get-or-create semantics.

	Ben Kelly [:bkelly, not reviewing]
	Comment 16 • 10 years ago

Comment on attachment 8525371 [details] [diff] [review]
cache.patch

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

::: dom/workers/ScriptLoader.cpp
@@ +823,5 @@
> +    mCacheStorage = new CacheStorage(cache::CHROME_ONLY_NAMESPACE,
> +                                     nullptr,
> +                                     mSandboxGlobalObject,
> +                                     NS_ConvertUTF16toUTF8(origin),
> +                                     baseDomain);

Sorry, this recently changed.  You must now pass the quotaGroup string and two booleans.  See how nsGlobalWindow and WorkerScope do this.

@@ +956,5 @@
> +CacheScriptLoader::ManageResponseObject(JSContext* aCx,
> +                                        JS::Handle<JSObject*> aObj)
> +{
> +  Response* response = nullptr;
> +  nsresult rv = UNWRAP_OBJECT(Response, aObj, response);

The Cache::Match() will resolve undefined if the Request is not in the Cache already.  In this case you need to perform a Fetch and then Cache::Put() the result.  I don't see this being done anywhere.

	Ben Kelly [:bkelly, not reviewing]
	Comment 17 • 10 years ago

Comment on attachment 8525371 [details] [diff] [review]
cache.patch

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

::: dom/workers/ScriptLoader.cpp
@@ +1008,5 @@
> +  while (true) {
> +    char buffer[4096];
> +    uint32_t ret;
> +
> +    mRv = mInputStream->Read(buffer, sizeof(buffer), &ret);

I think you could slightly optimize this by using ReadSegments().  You would need to check NS_InputStreamIsBuffered() and if false wrap it in an nsBufferedInputStream.  The Cache is not guaranteed to give you a buffered stream, but if it does then it would be nice to avoid an extra buffer copy.

More context: Right now Cache does not return buffered streams.  Once I land my patch to compress/uncompress streams, though, then it will returned buffered streams.

	Ben Kelly [:bkelly, not reviewing]
	Comment 18 • 10 years ago

Comment on attachment 8525371 [details] [diff] [review]
cache.patch

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

::: dom/workers/ScriptLoader.cpp
@@ +1003,5 @@
> +    return NS_OK;
> +  }
> +
> +  MOZ_ASSERT(mInputStream);
> +

Also, I suggest also asserting that mInputStream->IsNonBlocking() returns false here.  Your read loop below will not work for async streams.  This is fine since Cache will provide a sync stream, but lets assert for safety.

	Ben Kelly [:bkelly, not reviewing]
	Comment 19 • 10 years ago

Note, while talking to Jonas about various issues with the principal he mentioned we should be setting a load group for each service worker.  I think that probably needs to be done as part of this patch as the load group is passed into ChannelFromScriptURL() in ScriptLoader.

	Ben Kelly [:bkelly, not reviewing]
	Comment 23 • 9 years ago

Comment on attachment 8569932 [details] [diff] [review]
patch 1 - ScriptLoader using cache if needed

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

This is a good start and probably works in most cases.  I think we want to rethink a few things:

1) Per our discussion, I think we should look at this again.  It needs to handle the CancelOnMainThread() while populating the Cache.  In this case, I recommend just calling CacheStorage::Delete() to wipe the cache.

2) Also, we should only cache top level importScript() calls.

3) Finally, I think we should be streaming the network results to disk as they come in instead of duplicating a large buffer in memory.  This means starting the Cache::Put() when we get OnStartRequest() from the channel, providing an nsPipe to the synthetic Response object, and then writing to the pipe when the channel posts OnDataAvailable().

I think collectively this is represents a fair amount of structural changes.  I'm going to pause reviewing until we can address these issues.

::: dom/fetch/Request.cpp
@@ +90,5 @@
>      if (NS_IsMainThread()) {
>        nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(global);
> +      if (window) {
> +        nsCOMPtr<nsIURI> docURI = window->GetDocumentURI();
> +        nsCString spec;

nsAutoCString

@@ +107,5 @@
> +        if (aRv.Failed()) {
> +          return nullptr;
> +        }
> +      } else {
> +        // If we don't have a window, we must assume that this is a full URL.

This method is getting quite long and convoluted.  Can you split out the different cases into separate methods?  RequestFromDocument(), RequestFromChromeURL(), etc?

@@ +109,5 @@
> +        }
> +      } else {
> +        // If we don't have a window, we must assume that this is a full URL.
> +        nsCOMPtr<nsIURI> uri;
> +        aRv = NS_NewURI(getter_AddRefs(uri), input, nullptr, nullptr);

Add a comment that dom::URL::Constructor() is not permitted in chrome code.

@@ +114,5 @@
> +        if (NS_WARN_IF(aRv.Failed())) {
> +          return nullptr;
> +        }
> +
> +        nsCString spec;

nsAutoCString

::: dom/workers/RuntimeService.cpp
@@ +2283,5 @@
>  RuntimeService::CreateSharedWorkerFromLoadInfo(JSContext* aCx,
>                                                 WorkerPrivate::LoadInfo* aLoadInfo,
>                                                 const nsAString& aScriptURL,
>                                                 const nsACString& aName,
> +                                               const nsACString& aServiceWorkerCacheId,

It feels weird to me to have a ServiceWorker specific param in a SharedWorker method call.

::: dom/workers/RuntimeService.h
@@ +157,5 @@
>    CreateServiceWorkerFromLoadInfo(JSContext* aCx,
>                                    WorkerPrivate::LoadInfo* aLoadInfo,
>                                    const nsAString& aScriptURL,
>                                    const nsACString& aScope,
> +                                  const nsACString& aCacheId,

Can you call this "CacheName" here and everywhere else?

The spec calls it a name and in the Cache code the "CacheId" is something else.  It might just get confusing if someone is looking at both sets of code.

Also, I think we should be using nsString here since Cache names are a DOMString in the webidl.

::: dom/workers/ScriptLoader.cpp
@@ +253,5 @@
> +    AssertIsOnMainThread();
> +  }
> +
> +  void
> +  StoreLoader(CacheScriptLoader* aLoader)

nit: Maybe use "AddLoader()" to better communicate its being added to a list?

@@ +271,5 @@
> +
> +  Cache*
> +  GetCache() const
> +  {
> +    return mCache;

AssertIsOnMainThread()

@@ +294,5 @@
> +  FailLoaders(nsresult aRv);
> +
> +  WorkerPrivate* mWorkerPrivate;
> +  nsRefPtr<Cache> mCache;
> +  nsRefPtr<CacheStorage> mCacheStorage;

You shouldn't need to keep this ref'd.

There is a bug in Cache currently, though, that I need to fix.  I need to have the Promise hold the Cache and CacheStorage objects alive.  I'll add a patch to my queue to do this after I finish this review.

@@ +459,5 @@
>  
> +  void
> +  LoadingFinished(uint32_t aIndex, nsresult aRv)
> +  {
> +    MOZ_ASSERT(aIndex < mLoadInfos.Length());

AssertIsOnMainThread()

@@ +580,5 @@
> +
> +      return NS_OK;
> +    }
> +
> +    mCacheCreator = new CacheCreator(mWorkerPrivate);

MOZ_ASSERT(!mCacheCreator) before setting it here.

@@ +599,5 @@
> +
> +  nsresult
> +  LoadScript(uint32_t aIndex)
> +  {
> +    MOZ_ASSERT(aIndex < mLoadInfos.Length());

AssertIsOnMainThread()

@@ +950,5 @@
> +  }
> +
> +  ErrorResult error;
> +  mCacheStorage =
> +    CacheStorage::CreateOnMainThread(cache::CHROME_ONLY_NAMESPACE,

Rather than storing the CacheStorage in a member variable, I think you should just return it as an already_AddRefed.  It seems you use it once, immediately after you return from this method.

@@ +1084,5 @@
> +  MOZ_ASSERT(mLoadInfo.mFullURL.IsEmpty());
> +  CopyUTF8toUTF16(spec, mLoadInfo.mFullURL);
> +
> +  RequestOrUSVString request;
> +  *request.SetAsUSVString().ToAStringPtr() = mLoadInfo.mFullURL;

This is unsafe.  You must use the FakeString's Rebind() like this:

  request.SetAsUSVString().Rebind(mLoadInfo.mFullURL.Data(),
                                  mLoadInfo.mFullURL.Length());

@@ +1139,5 @@
> +    do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID);
> +  MOZ_ASSERT(target);
> +
> +  // Dispatch the reading of the inputstream to another thread.
> +  rv = target->Dispatch(this, NS_DISPATCH_NORMAL);

I think maybe we should use NS_AsyncCopy() with the STS as its target here instead.  This will let you cancel the operation if CancelMainThread() occurs.

@@ +1161,5 @@
> +    return NS_OK;
> +  }
> +
> +  MOZ_ASSERT(mInputStream);
> +  mRv = NS_ConsumeStream(mInputStream, UINT32_MAX, mBuffer);

This is synchronous and cannot be canceled if the worker is shutting down.

@@ +1163,5 @@
> +
> +  MOZ_ASSERT(mInputStream);
> +  mRv = NS_ConsumeStream(mInputStream, UINT32_MAX, mBuffer);
> +
> +  return NS_DispatchToMainThread(this);

MOZ_ALWAYS_TRUE() here?  I don't think dispatching to main thread can fail.

@@ +1175,5 @@
> +  ErrorResult rv;
> +  ResponseInit init;
> +
> +  nsAutoString bodyString;
> +  bodyString.Assign(mLoadInfo.mScriptTextBuf, mLoadInfo.mScriptTextLength);

I don't think we should duplicate this potentially large buffer in memory.

Instead, I think we should start the Cache::Put() operation before the network request completes.  We can assign an nsPipe as the body of the stream.  Then on every OnDataAvailable from the channel we write to the pipe.  This will stream the data to disk.

It also closes the window of time between when we start running the script and when its fully persisted to disk.

@@ +1180,5 @@
> +
> +  Optional<ArrayBufferOrArrayBufferViewOrBlobOrUSVStringOrURLSearchParams> body;
> +
> +  body.Construct();
> +  *body.Value().SetAsUSVString().ToAStringPtr() = bodyString;

Of course, this one is safe because you have an Owning string I believe.  The binding unions suck. :-(

Also, can you skip the bodyString local by just doing:

body.Value().SetAsUSVString().ToAStringPtr()->Assign(mLoadInfo.mScriptTextBuf, mLoadInfo.mScriptTestLength);

@@ +1190,5 @@
> +
> +  RequestOrUSVString request;
> +
> +  MOZ_ASSERT(!mLoadInfo.mFullURL.IsEmpty());
> +  *request.SetAsUSVString().ToAStringPtr() = mLoadInfo.mFullURL;

Not safe.  Use SetAsUSVString().Rebind().

::: dom/workers/WorkerPrivate.h
@@ +244,5 @@
>  private:
>    WorkerPrivate* mParent;
>    nsString mScriptURL;
>    nsCString mSharedWorkerName;
> +  nsCString mServiceWorkerCacheId;

I think this would fit better on the WorkerPrivateParent::LoadInfo struct.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 24 • 9 years ago

Comment on attachment 8569932 [details] [diff] [review]
patch 1 - ScriptLoader using cache if needed

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

I read this patch to understand patch 2. I didn't look at some of the details in all the new classes you introduced. I'd like to look at this again.

::: dom/fetch/Request.cpp
@@ +118,5 @@
> +        nsCString spec;
> +        aRv = uri->GetSpec(spec);
> +        if (NS_WARN_IF(aRv.Failed())) {
> +          return nullptr;
> +        }

It would be nice if everything in this else block except the copy could be in #ifdef DEBUG since this is only called from trusted code where we should expect a full, valid URL. Inside the block, please put the last line as MOZ_ASSERT(input.Equals(spec))

::: dom/workers/ScriptLoader.cpp
@@ +24,5 @@
>  #include "nsNetUtil.h"
>  #include "nsScriptLoader.h"
>  #include "nsString.h"
>  #include "nsTArray.h"
> +#include "nsStreamUtils.h"

Nit: alphabetical.

@@ +39,3 @@
>  #include "mozilla/dom/Exceptions.h"
> +#include "mozilla/dom/Response.h"
> +#include "mozilla/dom/Promise.h"

Nit: P then R.

@@ +241,5 @@
>  
> +class ScriptLoaderRunnable;
> +class CacheScriptLoader;
> +
> +class CacheCreator MOZ_FINAL : public PromiseNativeHandler

Please add a comment for this class and all other utility classes introduced in this patch. How do they map to stuff we designed on the whiteboard?

@@ +342,5 @@
> +  void
> +  ManageResponseObject(JSContext* aCx, JS::Handle<JSObject*> aObj);
> +
> +  WorkerPrivate* mWorkerPrivate;
> +  ScriptLoadInfo& mLoadInfo;

Comment about who owns this and why it is safe to hold a ref. Especially since this is a runnable.

@@ +350,5 @@
> +  nsresult mRv;
> +  nsCString mBuffer;
> +};
> +
> +NS_IMPL_ISUPPORTS(CacheScriptLoader, nsIRunnable)

Just have this inherit nsRunnable and you can get rid of this macro and some other things.

@@ +389,5 @@
> +  }
> +
> +  nsCOMPtr<nsIGlobalObject> mGlobal;
> +  nsRefPtr<Cache> mCache;
> +  ScriptLoadInfo& mLoadInfo;

Owner?

@@ +442,5 @@
> +      if (NS_FAILED(NS_ProxyRelease(mainThread, mCacheCreator))) {
> +        NS_WARNING("Failed to proxy release of cacheCreator, leaking instead!");
> +      }
> +    }
> +  }

Make mCacheCreator a nsMainThreadPtrHandle/Holder and you can get rid of this.

@@ +458,5 @@
>    }
>  
> +  void
> +  LoadingFinished(uint32_t aIndex, nsresult aRv)
> +  {

Nit: assert main thread.

@@ +566,5 @@
>      AssertIsOnMainThread();
>  
> +    nsresult rv;
> +
> +    if (!mWorkerPrivate->IsServiceWorker() ||

Please add a comment here about why you check each of these conditions to do a normal load.

@@ +579,5 @@
> +      }
> +
> +      return NS_OK;
> +    }
> +

You can MOZ_ASSERT(mIsWorkerScript && mLoadInfos.Length() == 1) here and get rid of the for loop below.

@@ +833,5 @@
> +    return NS_OK;
> +  }
> +
> +  void
> +  DataReceived(uint32_t aIndex, const nsACString& aBuffer)

Please rename to DataReceivedFromCache.

@@ +922,5 @@
>  
> +nsresult
> +CacheCreator::CreateCacheStorage()
> +{
> +  AssertIsOnMainThread();

Also assert mWorkerPrivate->IsServiceWorker()?

@@ +971,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  // We use the ServiceWorker scope as key for the cacheStorage.

... scope (which is the SharedWorker name) as key ...

@@ +974,5 @@
> +
> +  // We use the ServiceWorker scope as key for the cacheStorage.
> +  ErrorResult error;
> +  NS_ConvertUTF8toUTF16 key(mWorkerPrivate->SharedWorkerName());
> +  nsRefPtr<Promise> promise = mCacheStorage->Open(NS_ConvertUTF8toUTF16(key),

Nit: Too many converts to utf16

::: dom/workers/WorkerPrivate.cpp
@@ +2379,5 @@
> +                                        const nsAString& aScriptURL,
> +                                        bool aIsChromeWorker,
> +                                        WorkerType aWorkerType,
> +                                        const nsACString& aSharedWorkerName,
> +                                        const nsACString& aServiceWorkerCacheId,

It would be good to have at least a typedef, but even better a small value-like class for the cache name. That way you could type all of these as the class, and then you make sure it is impossible to convert from string -> ScriptCacheName, but possible from ScriptCacheName -> string. Then you could have a static getter ScriptCacheName::GenerateUniqueName() to get a new name in the [[Update]] process and pass it around everywhere. It will read much nicer if nothing else.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 25 • 9 years ago

Comment on attachment 8569932 [details] [diff] [review]
patch 1 - ScriptLoader using cache if needed

sorry restoring bkelly=r-

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 26 • 9 years ago

Comment on attachment 8569933 [details] [diff] [review]
patch 2 - cacheID integration with ServiceWorkerManager

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +578,4 @@
>  
>      // FIXME(nsm): "Extract mime type..."
>      // FIXME(nsm): Byte match to aString.
>      NS_WARNING("Byte wise check is disabled, just using new one");

Replace this with "The byte-by-byte check with the older script is performed by ScriptLoader."

::: dom/workers/ServiceWorkerManager.h
@@ +147,5 @@
>    nsRefPtr<ServiceWorkerInfo> mActiveWorker;
>    nsRefPtr<ServiceWorkerInfo> mWaitingWorker;
>    nsRefPtr<ServiceWorkerInfo> mInstallingWorker;
>  
> +  nsCString mPendingCacheId;

Could you call this mInstallingCacheId just so it aliases nicely to the worker instances?
And then arrange these 3 as they are arranged above.

@@ +149,5 @@
>    nsRefPtr<ServiceWorkerInfo> mInstallingWorker;
>  
> +  nsCString mPendingCacheId;
> +  nsCString mActiveCacheId;
> +  nsCString mWaitingCacheId;

The typedef?

::: dom/workers/ServiceWorkerRegistrarTypes.ipdlh
@@ +15,5 @@
>    nsCString scriptSpec;
>    nsCString currentWorkerURL;
> +
> +  nsCString activeCacheId;
> +  nsCString waitingCacheId;

If it is not too hard could we translate the ScriptCacheName class into ipdl too?

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +61,5 @@
> +}
> +
> +nsresult
> +SwapCache(nsIPrincipal* aPrincipal, nsACString& aDestination,
> +          nsACString& aSource)

Nit: This isn't really swapping since if destination is a non-empty cache, it doesn't set source to that cache. Better to call this MoveCache?
Could you swap source and destination ordering?

	Andrea Marchesini [:baku]
	Comment 27 • 9 years ago

I have to do status  200, statusText "OK", because the stream cannot be created onStartRequest()

	Andrea Marchesini [:baku]
	Comment 30 • 9 years ago

So, actually this patch does nothing useful :)
It compares the cached buffer with the network one and returns the value to the job. At this point the job should use this value to choose if the 'install' event has to be dispatched or not.
This should happen in a new patch, probably in a separate bug.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 32 • 9 years ago

Comment on attachment 8571458 [details] [diff] [review]
patch 4 - comparison

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

I'd like to give this a once over after the other patches are updated.
r- only for the spec issue.

::: dom/workers/ServiceWorkerManager.cpp
@@ +575,5 @@
>        Fail(NS_ERROR_DOM_NETWORK_ERR);
>        return rv;
>      }
>  
> +    nsAutoString cacheName;

Please add a comment about the spec step being implemented here.

@@ +576,5 @@
>        return rv;
>      }
>  
> +    nsAutoString cacheName;
> +    if (!mRegistration->mWaitingCacheName.IsEmpty()) {

MOZ_ASSERT(mWaitingWorker) in this block

@@ +578,5 @@
>  
> +    nsAutoString cacheName;
> +    if (!mRegistration->mWaitingCacheName.IsEmpty()) {
> +      cacheName = mRegistration->mWaitingCacheName;
> +    } else {

assert |!mWaitingWorker| and |mActiveWorker|

Do you think it is worth it to move the cache name into the ServiceWorkerInfo instead? I'll leave that decision to you.

@@ +581,5 @@
> +      cacheName = mRegistration->mWaitingCacheName;
> +    } else {
> +      cacheName = mRegistration->mActiveCacheName;
> +    }
> +

The spec also asks to check that the new script's URI and newest worker's script's URI are equal too.

@@ +612,5 @@
> +
> +    return NS_OK;
> +  }
> +
> +  virtual void

Nit: registerjob is final, so this virtual can be dropped.

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +46,5 @@
> +                                          sandboxGlobalObject,
> +                                          aPrincipal, aRv);
> +}
> +
> +class ComparePromiseHandler MOZ_FINAL : public PromiseNativeHandler

Considering this class handles 2 different promise resolves and 2 runnable actions, please add a comment :)

@@ +96,5 @@
> +  ManageCacheResult(JSContext* aCx, JS::Handle<JS::Value> aValue)
> +  {
> +    AssertIsOnMainThread();
> +
> +    if (!aValue.isObject()) {

NS_WARN_IF

@@ +102,5 @@
> +      return;
> +    }
> +
> +    JS::Rooted<JSObject*> obj(aCx, &aValue.toObject());
> +    if (!obj) {

NS_WARN_IF

@@ +133,5 @@
> +  ManageValueResult(JSContext* aCx, JS::Handle<JS::Value> aValue)
> +  {
> +    AssertIsOnMainThread();
> +
> +    if (!aValue.isObject()) {

NS_WARN_IF

@@ +134,5 @@
> +  {
> +    AssertIsOnMainThread();
> +
> +    if (!aValue.isObject()) {
> +      mCallback->ComparisonResult(NS_OK, false);

Please comment why this is OK and not fail.

@@ +139,5 @@
> +      return;
> +    }
> +
> +    JS::Rooted<JSObject*> obj(aCx, &aValue.toObject());
> +    if (!obj) {

NS_WARN_IF

@@ +152,5 @@
> +      return;
> +    }
> +
> +    response->GetBody(getter_AddRefs(mInputStream));
> +    MOZ_ASSERT(mInputStream);

Can we assert here that response's type is not error and response->Ok() is true?

@@ +177,5 @@
> +        return NS_OK;
> +      }
> +
> +      mCallback->ComparisonResult(NS_OK, mIsEqual);
> +      return NS_OK;

Nit: Move the OK call to } else { and keep only one return.

@@ +185,5 @@
> +
> +    nsAutoCString buffer;
> +    mRv = NS_ConsumeStream(mInputStream, UINT32_MAX, buffer);
> +    if (NS_SUCCEEDED(mRv)) {
> +      mIsEqual = mBuffer.Equals(NS_ConvertUTF8toUTF16(buffer));

It would be fantastically awesome if we could read this and the network stream in chunks and compare them :) Just a comment.

@@ +200,5 @@
> +
> +  enum {
> +    WaitingForCache,
> +    WaitingForValue,
> +    Finished

Unused.

@@ +291,5 @@
> +  MOZ_ASSERT(!aURL.IsEmpty());
> +  MOZ_ASSERT(aCallback);
> +
> +  if (aCacheName.IsEmpty()) {
> +    return NS_OK;

You already asserted above.

	Ben Kelly [:bkelly, not reviewing]
	Comment 33 • 9 years ago

Comment on attachment 8570818 [details] [diff] [review]
patch 1 - ScriptLoader using cache if needed

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

This is definitely on the right track.  Thanks for doing the early Cache::Put!

I'm concerned, though, that we might be holding raw pointers to WorkerPrivate after the sync loop is stopped in some cases.  I think we want to be extra careful about this.  In particular, I think we need to block cancelation until we are sure all copying is complete since that copying keeps the CacheScriptLoader objects alive.

Also, I'm not sure the use of LoadInfo::mServiceWorkerCacheName is quite thread safe.  It appears its read and written on different threads.

::: dom/fetch/Request.cpp
@@ +53,5 @@
> +GetRequestURLFromWindow(const GlobalObject& aGlobal, nsPIDOMWindow* aWindow,
> +                        const nsAString& aInput, nsAString& aRequestURL,
> +                        ErrorResult& aRv)
> +{
> +  MOZ_ASSERT(aWindow);

MOZ_ASSERT(NS_IsMainThread());

@@ +64,5 @@
> +  }
> +
> +  nsRefPtr<mozilla::dom::URL> url =
> +    dom::URL::Constructor(aGlobal, aInput, NS_ConvertUTF8toUTF16(spec), aRv);
> +  if (aRv.Failed()) {

NS_WARN_IF?  Or can this reasonably fail?

@@ +69,5 @@
> +    return;
> +  }
> +
> +  url->Stringify(aRequestURL, aRv);
> +  if (aRv.Failed()) {

NS_WARN_IF?

@@ +77,5 @@
> +
> +void
> +GetRequestURLFromChrome(const nsAString& aInput, nsAString& aRequestURL,
> +                        ErrorResult& aRv)
> +{

MOZ_ASSERT(NS_IsMainThread());

@@ +101,5 @@
> +void
> +GetRequestURLFromWorker(const GlobalObject& aGlobal, const nsAString& aInput,
> +                        nsAString& aRequestURL, ErrorResult& aRv)
> +{
> +   workers::WorkerPrivate* worker = workers::GetCurrentThreadWorkerPrivate();

MOZ_ASSERT(!NS_IsMainThread());

@@ +105,5 @@
> +   workers::WorkerPrivate* worker = workers::GetCurrentThreadWorkerPrivate();
> +   MOZ_ASSERT(worker);
> +   worker->AssertIsOnWorkerThread();
> +
> +   nsString baseURL = NS_ConvertUTF8toUTF16(worker->GetLocationInfo().mHref);

I think this can just be:

  NS_ConvertUTF8toUTF16 baseURL(worker->GetLocationInfo().mHref);

@@ +108,5 @@
> +
> +   nsString baseURL = NS_ConvertUTF8toUTF16(worker->GetLocationInfo().mHref);
> +   nsRefPtr<workers::URL> url =
> +     workers::URL::Constructor(aGlobal, aInput, baseURL, aRv);
> +   if (aRv.Failed()) {

NS_WARN_IF?

@@ +113,5 @@
> +     return;
> +   }
> +
> +   url->Stringify(aRequestURL, aRv);
> +   if (aRv.Failed()) {

NS_WARN_IF?

::: dom/fetch/Response.h
@@ +108,5 @@
>  
> +  static already_AddRefed<Response>
> +  Create(nsIGlobalObject* aGlobalObject,
> +         const Optional<ArrayBufferOrArrayBufferViewOrBlobOrUSVStringOrURLSearchParams>& aBody,
> +         const ResponseInit& aInit, ErrorResult& rv);

Is this used any more?

::: dom/workers/ScriptLoader.cpp
@@ +66,5 @@
>                       nsIIOService* ios,
>                       nsIScriptSecurityManager* secMan,
>                       const nsAString& aScriptURL,
>                       bool aIsWorkerScript,
> +                     bool aIsServiceWorker,

At some point we should make an enum representing worker type instead of using booleans like this.  I won't make you do it here, though.

@@ +122,5 @@
>  
> +  // ServiceWorker has its own cache.
> +  if (aIsServiceWorker) {
> +    flags |= nsIRequest::LOAD_BYPASS_CACHE;
> +  }

Does the spec say to do this?  Just because we are using the Service Worker Cache API, doesn't mean we still shouldn't pull from the http cache if something is present.

@@ +190,5 @@
> +  // This full URL string is populate only if this object is used in a
> +  // ServiceWorker.
> +  nsString mFullURL;
> +
> +  // This promise is set only when the script is related to a serviceWorker but

Is "related" the right word here?  Not sure what you were going for here.

@@ +323,5 @@
> +
> +  void
> +  FailLoaders(nsresult aRv);
> +
> +  WorkerPrivate* mWorkerPrivate;

Why not a strong ref here?  What guarantees that the WorkerPrivate will survive while we do all this async logic?

I guess the sync loop... but it appears CacheCreator can survive past the sync loop being stopped currently.

@@ +460,5 @@
> +      if (NS_FAILED(NS_GetMainThread(getter_AddRefs(mainThread)))) {
> +        NS_WARNING("Failed to proxy release of cacheCreator, leaking instead!");
> +      }
> +
> +      if (NS_FAILED(NS_ProxyRelease(mainThread, mCacheCreator))) {

I think mCacheCreator should be explicitly released on the main thread in ExecuteFinishedScripts() before dispatching the last ScriptExecutorRunnable().

The CacheCreator holds a raw pointer to the WorkerPrivate in mWorkerPrivate.  It better not survive past the point where the sync loop is stopped.  That happens when the last ScriptExecutorRunnable is executed.

The same goes for the CacheScriptLoader objects contained in the CacheCreator.

I think we want plenty of assertions to check that these async objects are cleaned up when that last executor runnable is dispatched.

@@ +590,5 @@
>    }
>  
> +  void
> +  CacheSavingFailed()
> +  {

I think you should cancel any copying out of the Cache at this point.  If you use the nsIInputStreamPump I mention in my other comment, then you can call pump->Cancel().  I believe you can then look for the OnStopRequest() to know the copying is complete.

I think this is important because you want to make sure that these copies complete fully and nothing is still referencing your CacheScriptLoader any more.  The CacheScriptLoader references WorkerPrivate with a raw pointer and must be gone by the time we stop the sync loop.  So all async copying must be confirmed done before allowing that to happen.

@@ +741,5 @@
> +      nsCOMPtr<nsIOutputStream> writer;
> +
> +      rv = NS_NewPipe(getter_AddRefs(reader), getter_AddRefs(writer), 0,
> +                      UINT32_MAX, // unlimited size to avoid writer WOULD_BLOCK case
> +                      false, false); // non-blocking reader/writer

The writer blocking mode doesn't really matter because of the "infinite" pipe size, but I think you want a non-blocking reader.  I would just set these to |true, true| to be safe.

@@ +748,4 @@
>        }
>  
> +      nsRefPtr<InternalResponse> ir =
> +        new InternalResponse(200, NS_LITERAL_CSTRING("OK"));

Maybe a comment that you are synthesizing the result code, but its never exposed to content.

@@ +762,4 @@
>  
> +      ErrorResult error;
> +      loadInfo.mCachePromise =
> +        mCacheCreator->GetCache()->Put(request, *response, error);

Please call Cache::Put() after the channel->AsyncOpen() below.  In the case of errors there won't be any spurious copying to the Cache still going on.  Instead the Response and its body stream will close properly.

This also ensure loadInfo.mCachePromise is not incorrectly set after an error.

If the Cache::Put() fails you can then cancel the channel.

@@ +1046,5 @@
> +CacheCreator::CreateCacheStorage()
> +{
> +  AssertIsOnMainThread();
> +  MOZ_ASSERT(mWorkerPrivate->IsServiceWorker());
> +

MOZ_ASSERT(!mCacheStorage)

@@ +1061,5 @@
> +  NS_ASSERTION(principal, "This should never be null here!");
> +
> +  AutoSafeJSContext cx;
> +  nsCOMPtr<nsIXPConnectJSObjectHolder> sandbox;
> +  nsresult rv = xpc->CreateSandbox(cx, principal, getter_AddRefs(sandbox));

Please have someone knowledgeable about js review this part.  I don't really know jsapi.

@@ +1289,5 @@
> +    return NS_OK;
> +  }
> +
> +  MOZ_ASSERT(mInputStream);
> +  mRv = NS_ConsumeStream(mInputStream, UINT32_MAX, mBuffer);

I don't think we want to use NS_ConsumeStream().  This is going to be a single blocking read that cannot be canceled.  Since we need to ensure the CacheScriptLoader is cleaned up on cancel, this is probably not what we want.

I previously thought you could use NS_AsnycCopy(), but I see now that you're reading the Cache file stream into a single buffer.  So NS_AsyncCopy() is not a good fit.

Instead, I think you want to use nsIInputStreamPump attached to an nsIStreamLoader.  On cancel you can then call pump->Cancel().  All of this can even be re-targeted to the STS as well.

For an example, see:

  https://dxr.mozilla.org/mozilla-central/source/dom/fetch/Fetch.cpp?from=Fetch.cpp#993

@@ +1454,5 @@
> +    // to use it for any async importScripts. For this reason we eliminate the
> +    // cache name here, after the evaluation of the script.
> +    if (mScriptLoader.mIsWorkerScript && mWorkerPrivate->IsServiceWorker()) {
> +      mWorkerPrivate->TruncateServiceWorkerCacheName();
> +    }

This is executed on the worker thread, but you access WorkerPrivate::ServiceWorkerCacheName() on the main thread.  I'm fairly certain you cannot read and write the same nsString object from different threads.

::: dom/workers/ServiceWorkerManager.cpp
@@ +2489,5 @@
>      return rv;
>    }
>  
>    info.mResolvedScriptURI = info.mBaseURI;
> +  info.mServiceWorkerCacheName = aCacheName;

What about the case in CreateServiceWorkerForWindow()?  There we call RuntimeService::CreateServiceWorker() and rely on it to construct the LoadInfo from the window.  Don't we need to get the cache name set in that case as well?

Or if it should not be set in that case, then maybe a comment explaining?

I don't see this handled in patch 2, either.

::: dom/workers/WorkerPrivate.h
@@ +567,5 @@
> +  const nsString&
> +  ServiceWorkerCacheName() const
> +  {
> +    MOZ_ASSERT(IsServiceWorker());
> +    return mLoadInfo.mServiceWorkerCacheName;

MOZ_ASSERT(NS_IsMainThread());

@@ +574,5 @@
> +  void
> +  TruncateServiceWorkerCacheName()
> +  {
> +    MOZ_ASSERT(IsServiceWorker());
> +    return mLoadInfo.mServiceWorkerCacheName.Truncate();

MOZ_ASSERT(NS_IsMainThread());

	Andrea Marchesini [:baku]
	Comment 34 • 9 years ago

> I guess the sync loop... but it appears CacheCreator can survive past the
> sync loop being stopped currently.

No it doesn't. If we release the CacheCreator where you suggested, we are still in the sync loop.


> @@ +590,5 @@
> >    }
> >  
> > +  void
> > +  CacheSavingFailed()
> > +  {
> 
> I think you should cancel any copying out of the Cache at this point.  If
> you use the nsIInputStreamPump I mention in my other comment, then you can
> call pump->Cancel().  I believe you can then look for the OnStopRequest() to
> know the copying is complete.

Why so? We cancel the channel, right?
That should stop the operation as well. If it doesn't, we have another kind of problem.

> I think this is important because you want to make sure that these copies
> complete fully and nothing is still referencing your CacheScriptLoader any
> more.  The CacheScriptLoader references WorkerPrivate with a raw pointer and
> must be gone by the time we stop the sync loop.  So all async copying must
> be confirmed done before allowing that to happen.

We do this because we don't call ExecuteFinishedScripts if:
1. we have a promise - and this is nullify in rejected/resolved callbacks
2. if mLoadingFinished is not true.

> 
> @@ +741,5 @@
> > +      nsCOMPtr<nsIOutputStream> writer;
> > +
> > +      rv = NS_NewPipe(getter_AddRefs(reader), getter_AddRefs(writer), 0,
> > +                      UINT32_MAX, // unlimited size to avoid writer WOULD_BLOCK case
> > +                      false, false); // non-blocking reader/writer
> 
> The writer blocking mode doesn't really matter because of the "infinite"
> pipe size, but I think you want a non-blocking reader.  I would just set
> these to |true, true| to be safe.

In order to work with a Tee, the writer must be blocking. I cannot find where we check this but I'll write a comment about this later.

> I don't think we want to use NS_ConsumeStream().  This is going to be a
> single blocking read that cannot be canceled.  Since we need to ensure the
> CacheScriptLoader is cleaned up on cancel, this is probably not what we want.

This is done in the IO thread. Plus, if the channel is canceled, this should fail somehow.

	Andrea Marchesini [:baku]
	Comment 35 • 9 years ago

Still I didn't change the pump issue. I'm not sure it's needed. give me a feedback based on the comment above. Thanks!

	Ben Kelly [:bkelly, not reviewing]
	Comment 36 • 9 years ago

(In reply to Andrea Marchesini (:baku) from comment #34)
> > I think you should cancel any copying out of the Cache at this point.  If
> > you use the nsIInputStreamPump I mention in my other comment, then you can
> > call pump->Cancel().  I believe you can then look for the OnStopRequest() to
> > know the copying is complete.
> 
> Why so? We cancel the channel, right?
> That should stop the operation as well. If it doesn't, we have another kind
> of problem.

There are two types of copies in this code, right?  Copying from the channel to the cache and copying out of the cache into the script buffer.  Canceling the channel stops the first type of copy, but not the second one.

Using NS_ConsumeStream() on a potentially large script file (asmjs file, for example) could take a relatively long time to return.  Do we safely guarantee in the cancel case that we wait for this?  Do we want to keep the worker alive in the sync loop this whole time?

I guess CacheSavingFailed() is the wrong place to cancel this operation since its not really "saving".  But you do need to cancel it from CancelOnMainThread() somehow.  Right now it appears CancelOnMainThread() drops the mCacheCreator and mCachePromise refs whether the NS_ConsumeStream() is done or not.  If NS_ConsumeStream() is still operating, its going to hold your CacheScriptLoader alive, no?

I'm happy to be wrong about that, but thats what it looks like to me right now.  :-)

> > The writer blocking mode doesn't really matter because of the "infinite"
> > pipe size, but I think you want a non-blocking reader.  I would just set
> > these to |true, true| to be safe.
> 
> In order to work with a Tee, the writer must be blocking. I cannot find
> where we check this but I'll write a comment about this later.

True.  It does require blocking behavior.  An infinite pipe just means you'll never hit it the condition.  Using blocking for the writer is fine.  We just want to make sure the reader part is non-blocking.

> > I don't think we want to use NS_ConsumeStream().  This is going to be a
> > single blocking read that cannot be canceled.  Since we need to ensure the
> > CacheScriptLoader is cleaned up on cancel, this is probably not what we want.
> 
> This is done in the IO thread. Plus, if the channel is canceled, this should
> fail somehow.

It won't fail because NS_ConsumeStream() is reading from the cache file on disk, not the channel.  There is no way to cancel this from the Cache side other than to cancel your copying operation.

	Ben Kelly [:bkelly, not reviewing]
	Comment 37 • 9 years ago

By the way, I asked the blink folks and they are also storing things in the http cache.  So removing the bypass flag seems the correct thing to do.

	Andrea Marchesini [:baku]
	Comment 38 • 9 years ago

1. Pump is used
2. better canceling algorithm

	Ben Kelly [:bkelly, not reviewing]
	Comment 39 • 9 years ago

Comment on attachment 8573104 [details] [diff] [review]
patch 1 - ScriptLoader using cache if needed

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

Definitely making progress!  I like how we are streaming to disk with the pump now.  Thank you for doing that!

As I dug into the ExecuteFinishedScripts(), however, I think there are some non-trivial problems there.  You can see the details in my comments, but I think its possible to execute scripts that are not fully loaded yet with the current logic.  I think this is serious enough that I still need to r-.  I'm sorry!

Also, I think we need some kind of testing interface to inspect the contents of the Cache in a mochitest.  Can you add a chrome only function on the ServiceWorkerGlobal or ServiceWorker that returns a Promise to the underlying Cache object?  We can then verify that files are being stored in our mochitests.

::: dom/workers/RuntimeService.cpp
@@ +2200,5 @@
>  nsresult
>  RuntimeService::CreateServiceWorker(const GlobalObject& aGlobal,
>                                      const nsAString& aScriptURL,
>                                      const nsACString& aScope,
> +                                    const nsAString& aCacheName,

Is it fair to MOZ_ASSERT(!aCacheName.IsEmpty())?  I realize this will trip until P2 sets the name.

@@ +2219,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  loadInfo.mServiceWorkerCacheName = aCacheName;

Can we remove everything after this line and replace it with a call to CreateServiceWorkerFromLoadInfo()?

Even better, can we remove RuntimeService::CreateServiceWorker() and move this all back into ServiceWorkerManager::CreateServiceWorkerForWindow()?  This would make RuntimeService have a single entry point for ServiceWorkers via CreateServiceWorkerFromLoadInfo().

It just seems we have duplicate code now and this weird second path that is easily forgotten.

::: dom/workers/ScriptLoader.cpp
@@ +200,4 @@
>    bool mExecutionScheduled;
>    bool mExecutionResult;
> +
> +  enum CacheStatus {

I mention in other comments that it would be nice to explicitly use states like Canceled and WritingToCache.

@@ +258,5 @@
> +  NS_INLINE_DECL_REFCOUNTING(CacheCreator)
> +
> +  CacheCreator(WorkerPrivate* aWorkerPrivate)
> +    : mWorkerPrivate(aWorkerPrivate)
> +    , mCacheName(mWorkerPrivate->ServiceWorkerCacheName())

If we save the cache name here, do we really need to save mWorkerPrivate in CacheCreator?  Can we also just save if this is a service worker or not?  If we can avoid storing the WorkerPrivate in a raw pointer on an async object, I would just feel better and might avoid future errors.

@@ +328,5 @@
> +
> +  CacheScriptLoader(WorkerPrivate* aWorkerPrivate, ScriptLoadInfo& aLoadInfo,
> +                    uint32_t aIndex, bool aIsWorkerScript,
> +                    ScriptLoaderRunnable* aRunnable)
> +    : mWorkerPrivate(aWorkerPrivate)

Can you make a CacheScriptLoader::Create() factory method and then extract the baseURL from the worker private there?  You could also save if the worker private is a service worker.  This would make it so you don't have to save the WorkerPrivate in a raw pointer and reduce the risk of things being deleted out from under us.

@@ +459,5 @@
> +    MOZ_ASSERT(aIndex < mLoadInfos.Length());
> +    ScriptLoadInfo& loadInfo = mLoadInfos[aIndex];
> +
> +    loadInfo.mLoadResult = aRv;
> +    loadInfo.mLoadingFinished = true;

MOZ_ASSERT(!loadInfo.mLoadingFinished) or we need to explicitly handle duplicate calls for the same index.  For example, the load result is be overwritten here, etc.  The ScripExecutorRunnable is going to try to read this information on another thread, so we have to make sure it doesn't change once the executor is dispatched.

I guess the MaybeExecuteFinishedScripts() name suggests you expect this called multiple times.  In that case we probably only want to overwrite mLoadResult if a failure has not already been saved there, etc.  We should also assert in the duplicate case that an executor was not already dispatched.

@@ +475,5 @@
> +    // We execute the last step if we don't have a pending operation with the
> +    // cache and the loading is completed.
> +    if (!mCanceledMainThread &&
> +        loadInfo.mLoadingFinished &&
> +        !loadInfo.mCachePromise) {

I don't think these checks are going to work here.  Consider two loads with indices 0 and 1:

  1) LoadingFinished(0, NS_OK) gets called, but mCachePromise is set.  This returns here because we're not ready to execute the script.
  2) LoadingFinished(1, NS_OK) gets called, but without mCachePromise set.  This calls into ExecuteFinishedScripts().
  3) When ExecuteFinishedScripts() is called for index 1, it starts executing the script at index 0 even though its still being written to Cache::Put().  This happens because ExecuteFinishedScripts() looks at the entire array.

This same thing can happen for the the mLoadingFinished flag as well.  Thats probably worse.

I think we need to check for mCachePromise and mLoadingFinished within the ExecuteFinishedScripts() loop where loadInfo.mChannel is currently checked.  Ideally all of these values could be rolled into a single ScriptLoadInfo::Finished() method or something.

@@ +555,5 @@
> +      }
> +
> +      if (loadInfo.mCachePromise) {
> +        MOZ_ASSERT(mWorkerPrivate->IsServiceWorker());
> +        loadInfo.mCachePromise->MaybeReject(NS_BINDING_ABORTED);

It seems this might trigger a duplicate call to LoadingFinished().  DeleteCache() above calls FailLoaders() which then calls Fail() on each loader.  Each Fail() call then calls LoadingFinished().

Rejecting this promise then unconditionally calls LoadingFinished() again.

If you make the promise reject call Fail(), you could then check there an mFailed boolean or NS_FAILED(mStatus) or something to avoid duplicates.

@@ +925,5 @@
> +                                     aBuffer.Length(),
> +                                     NS_LITERAL_STRING("UTF-8"), parentDoc,
> +                                     loadInfo.mScriptTextBuf,
> +                                     loadInfo.mScriptTextLength);
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

This won't build on non-debug because rv is only used in the MOZ_ASSERT.

Also, please aggressively free the UTF8 buffer after the conversion to UTF16.  I saw this sort of thing cause very large memory spikes on b2g previously.

@@ +993,5 @@
>          MOZ_ASSERT(false, "This should never fail!");
>        }
>      }
> +
> +    if (lastIndex == mLoadInfos.Length() - 1) {

Add a comment that this tests to see if the final script has completed loading.

Also, I think this whole block should move before the ScriptExecutorRunnable is dispatched.  In theory the script could execute and the sync loop exit before we clear mCacheCreator here.

@@ +1014,5 @@
> +CachePromiseHandler::ResolvedCallback(JSContext* aCx,
> +                                      JS::Handle<JS::Value> aValue)
> +{
> +  mLoadInfo.mCachePromise = nullptr;
> +  mRunnable->MaybeExecuteFinishedScripts(mIndex);

So thinking about this more, we are introducing a perf penalty here.  The file is going to have to be flushed to disk before we execute the script.  I think we should add a TODO or XXX comment to possibly optimize this in the future.

Probably the only reasonable way to do this is for Cache to implement bug 1110137.  That would make Cache resolve the Promise once the meta data is stored in the database, but not wait for the body to fully stream to disk.

We can't really execute the scripts, but maintain the sync loop until the cache promise resolves, because what if the cache promise rejects?  We've already executed the script.

In the meantime loading SW scripts will be a tad slower than normal.

@@ +1022,5 @@
> +CachePromiseHandler::RejectedCallback(JSContext* aCx,
> +                                      JS::Handle<JS::Value> aValue)
> +{
> +  mLoadInfo.mCachePromise = nullptr;
> +  mRunnable->DeleteCache();

Add a comment that this will fail the loaders and cause them to calling LoadingFinished() with an error code?

@@ +1083,5 @@
> +  // We use the ServiceWorker scope as key for the cacheStorage.
> +  ErrorResult error;
> +  MOZ_ASSERT(!mWorkerPrivate->ServiceWorkerCacheName().IsEmpty());
> +  nsRefPtr<Promise> promise =
> +    mCacheStorage->Open(mWorkerPrivate->ServiceWorkerCacheName(), error);

Why do you call ServiceWorkerCacheName() here when you have stored the name in mCacheName.  It seems you can get rid of one or the other?

@@ +1163,5 @@
> +{
> +  AssertIsOnMainThread();
> +  MOZ_ASSERT(NS_FAILED(aRv));
> +  MOZ_ASSERT(mWorkerPrivate->IsServiceWorker());
> +

Maybe check for duplicate Fail() calls here?  I guess this only matters if you direct all failures through this method as suggest in other comments.

@@ +1165,5 @@
> +  MOZ_ASSERT(NS_FAILED(aRv));
> +  MOZ_ASSERT(mWorkerPrivate->IsServiceWorker());
> +
> +  if (mPump) {
> +    MOZ_ASSERT(mLoadInfo.mCacheStatus == ScriptLoadInfo::Cached);

Should we chance mCacheStatus to ScriptLoadInfo::Canceled or something?  It seems confusing to leave it as Cached when its not really in the cache.

@@ +1186,5 @@
> +  if (mIsWorkerScript) {
> +    WorkerPrivate* parentWorker = mWorkerPrivate->GetParent();
> +    if (parentWorker) {
> +      baseURI = parentWorker->GetBaseURI();
> +      NS_ASSERTION(baseURI, "Should have been set already!");

Can we use MOZ_ASSERT here?

@@ +1194,5 @@
> +      baseURI = mWorkerPrivate->GetBaseURI();
> +    }
> +  } else {
> +    baseURI = mWorkerPrivate->GetBaseURI();
> +    NS_ASSERTION(baseURI, "Should have been set already!");

MOZ_ASSERT?

@@ +1236,5 @@
> +CacheScriptLoader::RejectedCallback(JSContext* aCx,
> +                                    JS::Handle<JS::Value> aValue)
> +{
> +  AssertIsOnMainThread();
> +  mRunnable->LoadingFinished(mIndex, NS_ERROR_FAILURE);

Call Fail() for consistent error path? This would also make it easier to avoid calling LoadingFinished() multiple times in the case of a cancellation.

@@ +1244,5 @@
> +CacheScriptLoader::ResolvedCallback(JSContext* aCx,
> +                                    JS::Handle<JS::Value> aValue)
> +{
> +  AssertIsOnMainThread();
> +

It seems we should check to see if we've had our Fail() method call.  If so, we probably don't want to start a LoadScript() call below.

@@ +1245,5 @@
> +                                    JS::Handle<JS::Value> aValue)
> +{
> +  AssertIsOnMainThread();
> +
> +  if (!aValue.isObject()) {

The resolved value should be exactly |undefined| if the Match() fails to find a cached value.  Should this check be |aValue.isUndefined()|?

@@ +1253,5 @@
> +  }
> +
> +  JS::Rooted<JSObject*> obj(aCx, &aValue.toObject());
> +  if (!obj) {
> +    mRunnable->LoadingFinished(mIndex, NS_ERROR_FAILURE);

Fail()

@@ +1260,5 @@
> +
> +  Response* response = nullptr;
> +  nsresult rv = UNWRAP_OBJECT(Response, obj, response);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    mRunnable->LoadingFinished(mIndex, rv);

Fail()

@@ +1271,5 @@
> +
> +  MOZ_ASSERT(!mPump);
> +  rv = NS_NewInputStreamPump(getter_AddRefs(mPump), inputStream);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    mRunnable->LoadingFinished(mIndex, rv);

Fail()

@@ +1275,5 @@
> +    mRunnable->LoadingFinished(mIndex, rv);
> +    return;
> +  }
> +
> +  rv = mPump->AsyncRead(this, nullptr);

As an optimization, you could also perform all copying off the main thread by doing this after AsyncRead():

  nsCOMPtr<nsIThreadRetargetableRequest> rr = do_QueryInterface(mPump);
  if (rr) {
    nsCOMPtr<nsIEventTarget> sts = do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID);
    rv = rr->RetargetDeliveryTo(sts);
    if (NS_WARN_IF(NS_FAILED(rv))) {
      NS_WARNING("Retargeting failed");
    }
  }

This is what Fetch.cpp does.

@@ +1278,5 @@
> +
> +  rv = mPump->AsyncRead(this, nullptr);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    mPump = nullptr;
> +    mRunnable->LoadingFinished(mIndex, rv);

Fail()

@@ +1282,5 @@
> +    mRunnable->LoadingFinished(mIndex, rv);
> +    return;
> +  }
> +
> +  mLoadInfo.mCacheStatus = ScriptLoadInfo::Cached;

Can we modify this to ScriptLoadInfo::WritingToCache and then change to ScriptLoadInfo::Cached once the operation has completed?  I know it doesn't matter to the code, but it might help people reading it later.

@@ +1293,5 @@
> +{
> +  AssertIsOnMainThread();
> +
> +  nsAutoCString data;
> +  mRv = NS_ReadInputStreamToString(aInputStream, data, aCount);

NS_ReadInputStreamToString() is convenient, but I think this does more allocations and copies than necessary.  You're almost certainly going to trigger a heap allocation in data and then copy the data in there.  Then you will trigger a growing allocation and another copy in mBuffer.Append() below.

Instead of doing this I think it would be better to create an nsStreamLoader and have CacheScriptLoader implement nsIStreamLoaderObserver.  You can then adopt the data directly in OnStreamComplete().  You can then have your buffer Adopt() the data.

I think this is the safest thing to do and avoids re-inventing nsStreamLoader.

@@ +1311,5 @@
> +}
> +
> +NS_IMETHODIMP
> +CacheScriptLoader::OnStopRequest(nsIRequest* aRequest, nsISupports* aContext,
> +                                 nsresult aStatusCode)

If you switch to nsStreamLoader and nsIStreamLoaderObserver, then this method basically becomes OnStreamComplete().

@@ +1321,5 @@
> +  if (NS_SUCCEEDED(mRv)) {
> +    mRunnable->DataReceivedFromCache(mIndex, mBuffer);
> +  }
> +
> +  mRunnable->LoadingFinished(mIndex, mRv);

If NS_FAILED(mRv) here, should we call Fail()?  maybe just short circuit this at the top of the method here?

::: dom/workers/ServiceWorkerManager.cpp
@@ +1758,5 @@
>  NS_IMETHODIMP
>  ServiceWorkerManager::CreateServiceWorkerForWindow(nsPIDOMWindow* aWindow,
>                                                     const nsACString& aScriptSpec,
>                                                     const nsACString& aScope,
> +                                                   const nsAString& aCacheName,

MOZ_ASSERT(!aCacheName.IsEmpty())?

@@ +2476,5 @@
>  NS_IMETHODIMP
>  ServiceWorkerManager::CreateServiceWorker(nsIPrincipal* aPrincipal,
>                                            const nsACString& aScriptSpec,
>                                            const nsACString& aScope,
> +                                          const nsAString& aCacheName,

MOZ_ASSERT(!aCacheName.IsEmpty())?

::: dom/workers/WorkerPrivate.h
@@ +576,5 @@
> +  TruncateServiceWorkerCacheName()
> +  {
> +    MOZ_ASSERT(IsServiceWorker());
> +    AssertIsOnMainThread();
> +    return mLoadInfo.mServiceWorkerCacheName.Truncate();

Should the name always be set at this point?  If so, can we assert it?

::: modules/libjar/nsJARChannel.cpp
@@ +20,2 @@
>  #include "nsIScriptSecurityManager.h"
> +#include "nsIStreamListener.h"

Should this be included in the patch?  I don't know what nsJARChannel has to do with this bug.

	Ben Kelly [:bkelly, not reviewing]
	Comment 40 • 9 years ago

Comment on attachment 8570821 [details] [diff] [review]
patch 3 - tests

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

As I mentioned in P1 comments, I think we need to verify that files are being stored somehow.  This means either trying to load offline in mochitests or by inspecting the cache.  (Ideally both.)  I think we can add a chrome-only function on the ServiceWorker or ServiceWorkerGlobal to get access to the Cache object.  Not sure how to force an offline test.

Also, I need some comments in these tests to understand whats going on.  :-)  It appears, though, that we are verifying the scripts are loaded and executed, right?  I just don't get how handleRequest() is invoked.

::: dom/workers/test/serviceworkers/scriptcache.sjs
@@ +1,2 @@
> +var counter = 0;
> +function handleRequest(request, response) {

How does this function get executed?

	Ben Kelly [:bkelly, not reviewing]
	Comment 41 • 9 years ago

Comment on attachment 8570820 [details] [diff] [review]
patch 2 - cacheName integration

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

Looks good to me with a few comments fixed.  I think Nikhil should review this, though.  I really don't know the intricacies of the ServiceWorker activation process at all.

::: dom/workers/ServiceWorkerManager.cpp
@@ +99,5 @@
>  
>    aData.scope() = aRegistration->mScope;
>    aData.scriptSpec() = aRegistration->mScriptSpec;
> +  aData.activeCacheName() = aRegistration->mActiveCacheName;
> +  aData.waitingCacheName() = aRegistration->mWaitingCacheName;

These can be empty here.  Right?  Thats fine, just making sure we can't assert anything here.

@@ +659,5 @@
>      nsresult rv =
>        swm->CreateServiceWorker(mRegistration->mPrincipal,
>                                 mRegistration->mInstallingWorker->ScriptSpec(),
>                                 mRegistration->mScope,
> +                               mRegistration->mPendingCacheName,

MOZ_ASSERT(mRegistration->mPendingCacheName)?  Its not obvious to me that this async called method necessarily happens after the pending cache name is set above.

@@ +759,5 @@
>    void
>    FailCommon(nsresult aRv)
>    {
> +    nsresult rv = serviceWorkerScriptCache::PurgeCache(mRegistration->mPrincipal,
> +                                                        mRegistration->mPendingCacheName);

nit: indentation

@@ +764,5 @@
> +    if (NS_FAILED(rv)) {
> +      NS_WARNING("Failed to purge the cache after a failure.");
> +    }
> +
> +    mRegistration->mPendingCacheName.Truncate();

Have PurgeCache do this automatically?

@@ +826,5 @@
> +    if (NS_FAILED(rv)) {
> +      NS_WARNING("Failed to swap the pending cache with the waiting one.");
> +    }
> +
> +    MOZ_ASSERT(mRegistration->mPendingCacheName.IsEmpty());

This assertion will fail if SwapCache() returns a failure.  The name swap doesn't actually take place if the underlying PurgeCache() fails.

The SwapCache() should probably proceed with the name swapping in the case anyways.

When does the modified registration persist to disk?  Do you need to issue an IPC?

@@ +1178,5 @@
> +  if (NS_FAILED(rv)) {
> +    NS_WARNING("Failed to swap the waiting cache with the active one.");
> +  }
> +
> +  MOZ_ASSERT(mWaitingCacheName.IsEmpty());

Same issue with the assertion when SwapCache fails here.

I see where the registration is persisted after this swap, though.

@@ +1199,5 @@
>    nsRefPtr<ServiceWorker> serviceWorker;
> +  rv = swm->CreateServiceWorker(mPrincipal,
> +                                mActiveWorker->ScriptSpec(),
> +                                mScope,
> +                                mActiveCacheName,

MOZ_ASSERT(mActiveCacheName)

@@ +2383,5 @@
>      nsRefPtr<ServiceWorker> sw;
>      rv = CreateServiceWorker(registration->mPrincipal,
>                               registration->mActiveWorker->ScriptSpec(),
>                               registration->mScope,
> +                             registration->mActiveCacheName,

MOZ_ASSERT(registration->mActiveCacheName)

::: dom/workers/ServiceWorkerManager.h
@@ +149,5 @@
>    nsRefPtr<ServiceWorkerInfo> mInstallingWorker;
>  
> +  nsString mPendingCacheName;
> +  nsString mActiveCacheName;
> +  nsString mWaitingCacheName;

Please document here exactly which steps in the activation process these names correspond to.  I know we talked about it on the whiteboard, but I can barely remember 5 minutes ago. :-)

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +68,5 @@
> +  MOZ_ASSERT(aPrincipal);
> +
> +  nsresult rv = PurgeCache(aPrincipal, aDestination);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;

Mentioned in other comments, but your assertions don't seem to expect this return.  They expect the names to be swapped even in the case of a failure.

	Andrea Marchesini [:baku]
	Comment 42 • 9 years ago

> ::: dom/workers/ScriptLoader.cpp
> @@ +200,4 @@
> >    bool mExecutionScheduled;
> >    bool mExecutionResult;
> > +
> > +  enum CacheStatus {
> 
> I mention in other comments that it would be nice to explicitly use states
> like Canceled and WritingToCache.

Well, but I don't need those states... right?

> If we save the cache name here, do we really need to save mWorkerPrivate in
> CacheCreator?  Can we also just save if this is a service worker or not?  If
> we can avoid storing the WorkerPrivate in a raw pointer on an async object,
> I would just feel better and might avoid future errors.

I also need the principal for the sandbox. I don't think it's a problem to store mWorkerPrivate. We are sure that the CacheCreator is destroyed before the sync loop is terminated.


> I think we need to check for mCachePromise and mLoadingFinished within the
> ExecuteFinishedScripts() loop where loadInfo.mChannel is currently checked. 
> Ideally all of these values could be rolled into a single
> ScriptLoadInfo::Finished() method or something.

Right. I added a check in ExecuteFinishedScripts() so that we don't process the loadInfo with a CachePromise.

> Also, I think this whole block should move before the ScriptExecutorRunnable
> is dispatched.  In theory the script could execute and the sync loop exit
> before we clear mCacheCreator here.

No because the cacheName must be available for nested importScripts. 
I'll think about this issue.

> @@ +1163,5 @@
> > +{
> > +  AssertIsOnMainThread();
> > +  MOZ_ASSERT(NS_FAILED(aRv));
> > +  MOZ_ASSERT(mWorkerPrivate->IsServiceWorker());
> > +
> 
> Maybe check for duplicate Fail() calls here?  I guess this only matters if
> you direct all failures through this method as suggest in other comments.

I don't get this point.

> 
> MOZ_ASSERT(!aCacheName.IsEmpty())?

In the next patches. In this patch it's always empty.

	Ben Kelly [:bkelly, not reviewing]
	Comment 43 • 9 years ago

(In reply to Andrea Marchesini (:baku) from comment #42)
> > I mention in other comments that it would be nice to explicitly use states
> > like Canceled and WritingToCache.
> 
> Well, but I don't need those states... right?

Ok, then maybe I just don't like the current names. :-)  I find it confusing that an entry is in the "Cached" state when its still being written to the Cache.  Its not actually Cached yet.

Just to be clear, this is a nit.

> > If we save the cache name here, do we really need to save mWorkerPrivate in
> > CacheCreator?  Can we also just save if this is a service worker or not?  If
> > we can avoid storing the WorkerPrivate in a raw pointer on an async object,
> > I would just feel better and might avoid future errors.
> 
> I also need the principal for the sandbox. I don't think it's a problem to
> store mWorkerPrivate. We are sure that the CacheCreator is destroyed before
> the sync loop is terminated.

You could save the principal in a member as well.  I mean, CacheCreator::Load() is called immediately after the constructor.  Or you could pass the WorkerPrivate into Load() as an argument.

I know we try to guarantee that CacheCreator and CacheScriptLoader are destroyed before the sync loop exits, but its entirely non-obvious to the reader that actually happens.  We have no way of asserting that right now either.  We drop the explicit refs, yes, but refs from promises and things could still keep the object alive.  Future changes could easily end up violating the destroyed-before-sync-loop-exits constraint and cause a UAF.

It just seems safer to me not to run this risk if we don't need actual dynamic values off of the WorkerPrivate.  If we're just accessing stuff that won't change, then lets just get it up front and avoid the risk.

> > Also, I think this whole block should move before the ScriptExecutorRunnable
> > is dispatched.  In theory the script could execute and the sync loop exit
> > before we clear mCacheCreator here.
> 
> No because the cacheName must be available for nested importScripts. 
> I'll think about this issue.

Ah, ok.  Then I guess this block does need to be triggered after the execution on the worker thread.  It seems either a runnable back to main thread or a lock are our only options.  Right?

> > @@ +1163,5 @@
> > Maybe check for duplicate Fail() calls here?  I guess this only matters if
> > you direct all failures through this method as suggest in other comments.
> 
> I don't get this point.

Can Fail() happen multiple times?  Is it expected?  Trying to fully grasp the life cycle.  It would be easier to understand the lifecycle if all failures for the CacheScriptLoader went through this method.

	Ben Kelly [:bkelly, not reviewing]
	Comment 44 • 9 years ago

Note, we also need some rebasing here.  It seems ServiceWorkerManager and RuntimeService have changed a bit.

	Ben Kelly [:bkelly, not reviewing]
	Comment 45 • 9 years ago

Comment on attachment 8570820 [details] [diff] [review]
patch 2 - cacheName integration

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

::: dom/workers/ServiceWorkerRegistrar.cpp
@@ +316,5 @@
> +    GET_LINE(cacheName);
> +    CopyUTF8toUTF16(cacheName, entry->activeCacheName());
> +
> +    GET_LINE(cacheName);
> +    CopyUTF8toUTF16(cacheName, entry->waitingCacheName());

Do we have any concept of version on the registrations file?  What happens if someone has an existing registration file and then this bug lands?  Will it blow up?

Seems like we should wipe registrations for now and then do graceful upgrades once we pref on by default.

	Andrea Marchesini [:baku]
	Comment 46 • 9 years ago

> Do we have any concept of version on the registrations file?  What happens
> if someone has an existing registration file and then this bug lands?  Will
> it blow up?

Yes we do. But I don't know if we want to implement a "migration" from version 1 to version 2.
I mean, ServiceWorker is not enabled yet.

	Ben Kelly [:bkelly, not reviewing]
	Comment 47 • 9 years ago

(In reply to Andrea Marchesini (:baku) from comment #46)
> > Do we have any concept of version on the registrations file?  What happens
> > if someone has an existing registration file and then this bug lands?  Will
> > it blow up?
> 
> Yes we do. But I don't know if we want to implement a "migration" from
> version 1 to version 2.
> I mean, ServiceWorker is not enabled yet.

What we are doing in Cache is detecting old versions now, but not doing upgrades yet.  Instead we treat it as database corruption and wipe the current stored values.  So data is lost, but we don't get stuck with a corrupted/non-working profile.

	Andrea Marchesini [:baku]
	Comment 49 • 9 years ago

no review request yet. I want to see the review for patch 1 first.

	Andrea Marchesini [:baku]
	Comment 50 • 9 years ago

> ::: dom/workers/test/serviceworkers/scriptcache.sjs

> How does this function get executed?

mochitest magic :) sjs is 'server side' javascript code. When you create sjs file, the handleRequest is used to manage the requests.

	Andrea Marchesini [:baku]
	Comment 51 • 9 years ago

Please, reconsider the review for the test. SJS is documented here:

https://developer.mozilla.org/en-US/docs/Httpd.js/HTTP_server_for_unit_tests

What you propose will be extremity useful for the devtools, but just for testing how the scriptLoader works, having a sjs is enough, imo.

	Ben Kelly [:bkelly, not reviewing]
	Comment 52 • 9 years ago

This cannot land until Cache::Put() is functional.  This needs bug 1110814.

	Ben Kelly [:bkelly, not reviewing]
	Comment 53 • 9 years ago

Comment on attachment 8575249 [details] [diff] [review]
patch 1 - ScriptLoader using cache if needed

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

Thanks for following through on all of this Andrea!  r=me with comments addressed.  In particular, I'd still like the mServiceWorkerCacheName to only be accessed on one thread.

I do want to ask Kyle to look at the ScriptLoader.cpp changes as well.  I think this is tricky code and we don't hit all the corner cases in our tests yet.  Having a worker peer look at is probably advisable.

::: dom/workers/ScriptLoader.cpp
@@ +182,5 @@
>    }
>  
>    nsString mURL;
> +
> +  // This full URL string is populate only if this object is used in a

nit: populated

@@ +188,5 @@
> +  nsString mFullURL;
> +
> +  // This promise is set only when the script is for a serviceWorker but
> +  // it's not in the cache yet. The promise is resolved when the fully body is
> +  // stored into the cache.

Also mention that mCachePromise will be set to nullptr after resolution?  I believe we use this as a check for completion elsewhere.

@@ +207,5 @@
> +    // load it from the cache.
> +    Uncached,
> +
> +    // Intermediate state.
> +    WritingToCache,

Thank you!

@@ +280,5 @@
> +  void
> +  AddLoader(CacheScriptLoader* aLoader)
> +  {
> +    AssertIsOnMainThread();
> +    mLoaders.AppendElement(aLoader);

MOZ_ASSERT(!mCacheStorage)

I don't think we want to ever call AddLoader() after we begin actually loading.

@@ +296,5 @@
> +  Cache*
> +  GetCache() const
> +  {
> +    AssertIsOnMainThread();
> +    return mCache;

This is called like GetCache()->Put() using the raw pointer return value.  Can you assert mCache here?

@@ +321,5 @@
> +
> +  void
> +  FailLoaders(nsresult aRv);
> +
> +  WorkerPrivate* mWorkerPrivate;

Please at least comment that this is only valid until the sync loop is stopped.

@@ +368,5 @@
> +  {
> +    AssertIsOnMainThread();
> +  }
> +
> +  WorkerPrivate* mWorkerPrivate;

Again, please add a comment about life cycle of this weak ref.

@@ +488,5 @@
> +    // We execute the last step if we don't have a pending operation with the
> +    // cache and the loading is completed.
> +    if (!mCanceledMainThread &&
> +        loadInfo.mLoadingFinished &&
> +        !loadInfo.mCachePromise) {

Now that we check these mLoadingFinished and !loadInfo.mCachePromise in ExecuteFinishedScripts(), I think you can lose the checks here.

If you want to keep the checks here, then please add a Finished() method that wraps them up to avoid code duplication.

@@ +563,5 @@
>            NS_FAILED(loadInfo.mChannel->Cancel(NS_BINDING_ABORTED))) {
>          NS_WARNING("Failed to cancel channel!");
>          loadInfo.mChannel = nullptr;
>          loadInfo.mLoadResult = NS_BINDING_ABORTED;
> +        loadInfo.mLoadingFinished = true;

Call LoadingFinished(index, NS_BINDING_ABORTED) here instead of duplicating the code?

@@ +569,5 @@
> +
> +      if (loadInfo.mCachePromise) {
> +        MOZ_ASSERT(mWorkerPrivate->IsServiceWorker());
> +        loadInfo.mCachePromise->MaybeReject(NS_BINDING_ABORTED);
> +        loadInfo.mCachePromise = nullptr;

Should this be done prior to canceling the channel?  It seems the channel cancellation may trigger LoadingFinished() and it would be nice to have mCachePromise already cleared in that case.

@@ +725,5 @@
> +      rv = NS_NewPipe(getter_AddRefs(reader), getter_AddRefs(writer), 0,
> +                      UINT32_MAX, // unlimited size to avoid writer WOULD_BLOCK case
> +                      true, false); // non-blocking reader/writer
> +      if (NS_WARN_IF(NS_FAILED(rv))) {
> +        return rv;

loadInfo.mCacheStatus = ScriptLoadInfo::Cancel on all error conditions here?

@@ +737,5 @@
> +      nsCOMPtr<nsIStreamListenerTee> tee =
> +        do_CreateInstance(NS_STREAMLISTENERTEE_CONTRACTID);
> +      rv = tee->Init(loader, writer, nullptr);
> +      if (NS_WARN_IF(NS_FAILED(rv))) {
> +        return rv;

loadInfo.mCacheStatus = ScriptLoadInfo::Cancel

@@ +743,4 @@
>  
> +      rv = channel->AsyncOpen(tee, indexSupports);
> +      if (NS_WARN_IF(NS_FAILED(rv))) {
> +        return rv;

loadInfo.mCacheStatus = ScriptLoadInfo::Cancel

@@ +759,5 @@
> +      loadInfo.mCachePromise =
> +        mCacheCreator->GetCache()->Put(request, *response, error);
> +      if (NS_WARN_IF(error.Failed())) {
> +        channel->Cancel(error.ErrorCode());
> +        return error.ErrorCode();

loadInfo.mCacheStatus = ScriptLoadInfo::Cancel

@@ +938,5 @@
> +
> +    DebugOnly<nsresult> rv =
> +      nsScriptLoader::ConvertToUTF16(nullptr, aString, aStringLen,
> +                                     NS_LITERAL_STRING("UTF-8"), parentDoc,
> +                                     loadInfo.mScriptTextBuf,

MOZ_ASSERT(!loadInfo.mScriptTextBuf) before calling ConvertToUTF16().

I think ConvertToUTF16() will just overwrite and leak any existing buffer.

@@ +989,5 @@
>      if (firstIndex != UINT32_MAX) {
>        for (uint32_t index = firstIndex; index < mLoadInfos.Length(); index++) {
>          ScriptLoadInfo& loadInfo = mLoadInfos[index];
>  
>          // If we still have a channel then the load is not complete.

Please update this comment.  The completeness is not solely determined by the channel any more.

@@ +1062,5 @@
> +
> +  nsIPrincipal* principal = mWorkerPrivate->GetPrincipal();
> +  if (!principal) {
> +    WorkerPrivate* parentWorker = mWorkerPrivate->GetParent();
> +    NS_ASSERTION(parentWorker, "Must have a principal!");

MOZ_ASSERT(parentWorker)?  It seems we want a hard fail instead of the soft assertion from NS_ASSERTION.

@@ +1066,5 @@
> +    NS_ASSERTION(parentWorker, "Must have a principal!");
> +    principal = parentWorker->GetPrincipal();
> +  }
> +
> +  NS_ASSERTION(principal, "This should never be null here!");

MOZ_ASSERT

@@ +1076,5 @@
> +    return rv;
> +  }
> +
> +  mSandboxGlobalObject = xpc::NativeGlobal(sandbox->GetJSObject());
> +  if (!mSandboxGlobalObject) {

NS_WARN_IF

@@ +1096,5 @@
> +nsresult
> +CacheCreator::Load()
> +{
> +  AssertIsOnMainThread();
> +  MOZ_ASSERT(mWorkerPrivate->IsServiceWorker());

MOZ_ASSERT(!mLoaders.IsEmpty())

I assume we never want to run this if we don't have at least one loader, right?

@@ +1139,5 @@
> +CacheCreator::ResolvedCallback(JSContext* aCx, JS::Handle<JS::Value> aValue)
> +{
> +  AssertIsOnMainThread();
> +
> +  if (!aValue.isObject()) {

You can just MOZ_ASSERT(aValue.isObject()) instead of checking.  Per the spec CacheStorage::Open() should never resolve without a cache object.  Any errors will go to the rejected path.

@@ +1157,5 @@
> +    FailLoaders(rv);
> +    return;
> +  }
> +
> +  mCache = cache;

MOZ_ASSERT(mCache) after this?

@@ +1170,5 @@
> +{
> +  AssertIsOnMainThread();
> +
> +  ErrorResult rv;
> +  nsRefPtr<Promise> promise = mCacheStorage->Delete(mCacheName, rv);

Add a comment that its safe to do this while Cache::Match() and Cache::Put() calls are running.

@@ +1302,5 @@
> +  }
> +
> +  nsCOMPtr<nsIInputStream> inputStream;
> +  response->GetBody(getter_AddRefs(inputStream));
> +  MOZ_ASSERT(inputStream);

In theory its possible to have a Response object without an inputStream.  This is represented by inputStream == nullptr.  In those cases I think you just want to set the script buffer to be zero length and be done.

This will be exceptionally rare, but it is possible.  Particularly if a synthesized Response somehow gets put into the Cache.

@@ +1331,5 @@
> +  if (rr) {
> +    nsCOMPtr<nsIEventTarget> sts =
> +      do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID);
> +    rv = rr->RetargetDeliveryTo(sts);
> +    if (NS_WARN_IF(NS_FAILED(rv))) {

I don't think we should treat this as a hard fail.  The operation should still complete even if our optimization to do copying on STS failed.  In other code we just use an NS_WARNING instead.

::: dom/workers/WorkerPrivate.h
@@ +488,5 @@
> +  void
> +  TruncateServiceWorkerCacheName()
> +  {
> +    MOZ_ASSERT(IsServiceWorker());
> +    return mLoadInfo.mServiceWorkerCacheName.Truncate();

Clearing the string on the separate thread still makes me uncomfortable.  We can't assert that its not being used on the main thread any more.

Could we instead do something like this:

struct LoadInfo
{
  Atomic<bool> mLoadingWorkerScript;
};

void
SetLoadingWorkerScript(bool aLoadingWorkerScript)
{
  // any thread
  mLoadingWorkerScript = aLoadingWorkerScript;
}

const nsString&
ServiceWorkerCacheName() const
{
  MOZ_ASSERT(IsServiceWorker());
  AssertIsOnMainThread();
  return mLoadingWorkerScript ? mLoadInfo.mServiceWorkerCacheName : EmptyString();
}

The ScriptLoader then call SetLoadingWorkerScript(false) instead of TruncateServiceWorkerCacheName().

::: modules/libjar/nsJARChannel.cpp
@@ +15,5 @@
>  #include "nsIViewSourceChannel.h"
>  #include "nsContentUtils.h"
>  #include "nsProxyRelease.h"
>  
> +#include "nsIInputStreamPump.h"

Is this needed?  Seems like an unrelated hunk.

	Ben Kelly [:bkelly, not reviewing]
	Comment 54 • 9 years ago

Comment on attachment 8570821 [details] [diff] [review]
patch 3 - tests

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

Ok, I understand now.  Thanks for explaining.

Can you write a follow-up bug to test actual offline support?

r=me with that follow-up bug and the one comment addressed.

Thanks!

::: dom/workers/test/serviceworkers/scriptcache_worker.js
@@ +11,5 @@
> +    if (!res.length) {
> +      dump("ERROR: no clients are currently controlled.\n");
> +    }
> +
> +    res[0].postMessage(counter == 2 ? "OK" : "KO");

Can you do another importScripts() call here to prove that async loading still works on the ServiceWorker?

	Ben Kelly [:bkelly, not reviewing]
	Comment 55 • 9 years ago

I think we definitely want a full try on this one before landing.

	Andrea Marchesini [:baku]
	Comment 56 • 9 years ago

I retrieve data from the network and from the cache at the same time to improve the performance. Just a feedback waiting for the reviews of the other patches.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 57 • 9 years ago

Comment on attachment 8575472 [details] [diff] [review]
patch 4 - comparison

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

I can't figure out where you are comparing the URLs of the current scriptspec and the Newest() to be equal. Also see my earlier review. General process looks good.

::: dom/workers/ServiceWorkerManager.cpp
@@ +543,4 @@
>      }
>  
> +    if (aInCacheAndEqual) {
> +      Done(NS_OK);

This should be Succeed().

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +255,5 @@
> +{
> +public:
> +  NS_INLINE_DECL_REFCOUNTING(CompareManager)
> +
> +  CompareManager(CompareCallback* aCallback)

you'll want 'explicit ' on all the ctors in this file, otherwise builds will break (well, at least my build will break :))

	Andrea Marchesini [:baku]
	Comment 58 • 9 years ago

Comments applied.

	Andrea Marchesini [:baku]
	Comment 59 • 9 years ago

nsm, can you take a look at this patch again? It's rebased on top of m-i.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 63 • 9 years ago

Comment on attachment 8576737 [details] [diff] [review]
patch 2 - cacheName integration

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

I'd like to take a look at changes in ServiceWorkerManager.cpp again.

::: dom/workers/ServiceWorkerManager.cpp
@@ +575,5 @@
> +
> +    nsAutoString cacheName;
> +    rv = serviceWorkerScriptCache::GenerateCacheName(cacheName);
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      Fail(NS_ERROR_DOM_NETWORK_ERR);

Just a note that this will change to NS_ERROR_DOM_TYPE_ERR in Bug 1131271.

@@ +577,5 @@
> +    rv = serviceWorkerScriptCache::GenerateCacheName(cacheName);
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      Fail(NS_ERROR_DOM_NETWORK_ERR);
> +      return rv;
> +    }

Move this block above the modification to mSetOfScopesBeingUpdated so we don't have to worry about clearing it on failure.

@@ +640,5 @@
>      swm->InvalidateServiceWorkerRegistrationWorker(mRegistration,
>                                                     WhichServiceWorker::INSTALLING_WORKER);
> +
> +    nsAutoString cacheName;
> +    nsresult rv = serviceWorkerScriptCache::GenerateCacheName(cacheName);

This worker is the worker that was just updated that is going to be sent the install event. We should be re-using the cacheName from the update step.

@@ +756,5 @@
> +    if (mRegistration->mInstallingWorker) {
> +      nsresult rv = serviceWorkerScriptCache::PurgeCache(mRegistration->mPrincipal,
> +                                                         mRegistration->mInstallingWorker->CacheName());
> +      if (NS_FAILED(rv)) {
> +        NS_WARNING("Failed to purge the cache after a failure.");

Nit: Failed to purge the installing worker cache.

@@ +1625,5 @@
> +                                                registration->mWaitingWorker->CacheName());
> +      if (NS_FAILED(rv)) {
> +        NS_WARNING("Failed to purge the waiting cache.");
> +      }
> +    }

Is it possible to move this entire change into RegistrationInfo::Clear()? This will only run on unregister(), but a first time register() if it fails, then Clear() is also called from ContinueAfterInstallEvent, in which you may also want to purge the installing worker info's cache.

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +26,5 @@
> +
> +  nsIXPConnect* xpc = nsContentUtils::XPConnect();
> +  MOZ_ASSERT(xpc, "This should never be null!");
> +
> +  AutoSafeJSContext cx;

Can you use AutoJSAPI here so we can hook into the new error reporting stuff? The no-arg Init() will give you a cx and CreateSandbox will take care of entering the right compartment.

::: dom/workers/ServiceWorkerScriptCache.h
@@ +18,5 @@
> +nsresult
> +PurgeCache(nsIPrincipal* aPrincipal, const nsAString& aCacheName);
> +
> +nsresult
> +GenerateCacheName(nsAString& aname);

Nit: aName.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 64 • 9 years ago

Comment on attachment 8576742 [details] [diff] [review]
patch 4 - comparison

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

This is good but I'd like to see the changes due to Bug 1130688

::: dom/workers/ServiceWorkerManager.cpp
@@ +539,3 @@
>    {
>      if (NS_WARN_IF(NS_FAILED(aStatus))) {
>        Fail(NS_ERROR_DOM_NETWORK_ERR);

Nit: Now changed to TYPE_ERR

@@ +550,1 @@
>      NS_WARNING("Byte wise check is disabled, just using new one");

Nit: Remove this.

@@ +560,2 @@
>      if (NS_WARN_IF(NS_FAILED(rv))) {
>        Fail(NS_ERROR_DOM_NETWORK_ERR);

Nit: TYPE_ERR

@@ +691,5 @@
> +
> +    // 9.2.20 If newestWorker is not null, and newestWorker's script url is
> +    // equal to registration's registering script url and response is a
> +    // byte-for-byte match with the script resource of newestWorker...
> +    if (workerInfo && workerInfo->ScriptSpec() == mRegistration->mScriptSpec) {

I'd prefer Equals(mRegistration->mScriptSpec)

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +84,5 @@
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return rv;
> +    }
> +
> +    // FIXME(nsm): Set redirect limit.

This landed, please copy the relevant change.

@@ +135,5 @@
> +};
> +
> +NS_IMPL_ISUPPORTS(CompareNetwork, nsIStreamLoaderObserver)
> +
> +// This class gets a cached URL form the CacheStorage and then it calls

Nit: Response from

@@ +382,5 @@
> +                                 const uint8_t* aString)
> +{
> +  AssertIsOnMainThread();
> +
> +  // If no channel, Abort() has been called.

Not sure if you need this considering when you abort the channel it will call OnStartListener/OnStopRequest with NS_BINDING_ABORTED. But better safe than sorry I guess.

@@ +417,5 @@
> +    mManager->NetworkFinished(NS_ERROR_FAILURE);
> +    return NS_OK;
> +  }
> +
> +  // FIXME(nsm): "Extract mime type..."

Bug 1130688 introduced some additional checking of the SW URL's location here. It seems best to notify the ComparisonResult callback of the final URL I guess and have SWM perform the check as it does right now.

@@ +544,5 @@
> +    mManager->CacheFinished(rv, false);
> +    return;
> +  }
> +
> +  MOZ_ASSERT(response->Ok());

Can we really assert this?

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 65 • 9 years ago

Taking for now until Andrea is back.
Kyle, if you were waiting on review because Baku is on vacation, could you do it now? This is one of the major blockers for ServiceWorkers.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 67 • 9 years ago

Comment on attachment 8576731 [details] [diff] [review]
patch 1 - ScriptLoader using cache if needed

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

Given the amount of sketchiness we're doing with references into mLoadInfos, I think that should now be marked const (arguably it should have been before, but c'est la vie).  That involves tweaking the ctor a bit (should be doable with move references) and possibly a couple other things.

Is the canceling logic in this patch exercised by tests?  I'm very skeptical that it's correct.  In particular, when we explicitly reject the promise the rejection handlers will not run until after we've returned to the event loop.  By that point we've already told the worker it can tear down the sync loop and mWorkerPrivate could be invalid.  Am I missing something that prevents this?

::: dom/workers/ScriptLoader.cpp
@@ +102,5 @@
>    // If this script loader is being used to make a new worker then we need
>    // to do a same-origin check. Otherwise we need to clear the load with the
>    // security manager.
>    if (aIsWorkerScript) {
> +    nsAutoCString scheme;

Why?  If we're calling a getter it probably has its own copy of the string to hand out, so all Auto does is use more stack space.

@@ +189,5 @@
> +
> +  // This promise is set only when the script is for a serviceWorker but
> +  // it's not in the cache yet. The promise is resolved when the fully body is
> +  // stored into the cache.  mCachePromise will be set to nullptr after
> +  // resolution.

ServiceWorker

full body

@@ +203,5 @@
>    bool mExecutionResult;
> +
> +  enum CacheStatus {
> +    // By default a normal script is just loaded from the network. But for
> +    // ServiceWorker, we have to check if the cache contains the script and

ServiceWorkers

@@ +211,5 @@
> +    // Intermediate state.
> +    WritingToCache,
> +
> +    // Intermediate state.
> +    ReadingFromCache,

These two state comments are not particularly helpful.

@@ +215,5 @@
> +    ReadingFromCache,
> +
> +    // This status is used when this script has been loaded from the
> +    // ServiceWorker cache.
> +    Cached,

Just "This script has been ..."

@@ +228,5 @@
> +  CacheStatus mCacheStatus;
> +
> +  bool Finished() const
> +  {
> +    return  mLoadingFinished && !mCachePromise && !mChannel;

nit: extra whitespace between return and mLoadingFinished

@@ +273,5 @@
> +
> +class CacheCreator MOZ_FINAL : public PromiseNativeHandler
> +{
> +public:
> +  NS_INLINE_DECL_REFCOUNTING(CacheCreator)

This already has a refcount inherited from PromiseNativeHandler.

@@ +300,5 @@
> +  nsresult
> +  Load();
> +
> +  Cache*
> +  GetCache() const

Cache(), for a never-null getter

@@ +308,5 @@
> +    return mCache;
> +  }
> +
> +  nsIGlobalObject*
> +  GetGlobal() const

And Global()

@@ +321,5 @@
> +
> +private:
> +  ~CacheCreator()
> +  {
> +    AssertIsOnMainThread();

FWIW, the refcounting macros will already enforce this (when combined with the AssertIsOnMainThread in the ctor).

@@ +345,5 @@
> +class CacheScriptLoader MOZ_FINAL : public PromiseNativeHandler
> +                                  , public nsIStreamLoaderObserver
> +{
> +public:
> +  NS_DECL_ISUPPORTS

ISUPPORTS_INHERITED

@@ +395,5 @@
> +
> +class CachePromiseHandler MOZ_FINAL : public PromiseNativeHandler
> +{
> +public:
> +  NS_INLINE_DECL_REFCOUNTING(CachePromiseHandler)

ibid

@@ +734,5 @@
> +      nsCOMPtr<nsIOutputStream> writer;
> +
> +      rv = NS_NewPipe(getter_AddRefs(reader), getter_AddRefs(writer), 0,
> +                      UINT32_MAX, // unlimited size to avoid writer WOULD_BLOCK case
> +                      true, false); // non-blocking reader/writer

Your comments don't match the booleans.

@@ +736,5 @@
> +      rv = NS_NewPipe(getter_AddRefs(reader), getter_AddRefs(writer), 0,
> +                      UINT32_MAX, // unlimited size to avoid writer WOULD_BLOCK case
> +                      true, false); // non-blocking reader/writer
> +      if (NS_WARN_IF(NS_FAILED(rv))) {
> +        loadInfo.mCacheStatus = ScriptLoadInfo::Cancel;

You should set mCacheStatus at the beginning of this block, so you don't need to set it in all the early returns.

@@ +852,5 @@
>      nsCOMPtr<nsIURI> finalURI;
>      rv = NS_GetFinalChannelURI(channel, getter_AddRefs(finalURI));
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    nsAutoCString filename;

Again, why?

@@ +917,5 @@
>            return NS_ERROR_DOM_BAD_URI;
>          }
>        }
>        else  {
> +        nsAutoCString scheme;

ibid

@@ +957,5 @@
> +      nsScriptLoader::ConvertToUTF16(nullptr, aString, aStringLen,
> +                                     NS_LITERAL_STRING("UTF-8"), parentDoc,
> +                                     loadInfo.mScriptTextBuf,
> +                                     loadInfo.mScriptTextLength);
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

Asserting this is almost certainly wrong.  Why do we not have to handle this error condition?

@@ +1074,5 @@
> +  nsIXPConnect* xpc = nsContentUtils::XPConnect();
> +  MOZ_ASSERT(xpc, "This should never be null!");
> +
> +  nsIPrincipal* principal = mWorkerPrivate->GetPrincipal();
> +  if (!principal) {

Please add a comment stating that it may be too early for this worker to have a principal, but if so our parent has one already.

@@ +1076,5 @@
> +
> +  nsIPrincipal* principal = mWorkerPrivate->GetPrincipal();
> +  if (!principal) {
> +    WorkerPrivate* parentWorker = mWorkerPrivate->GetParent();
> +    MOZ_ASSERT(parentWorker, "Must have a principal!");

That's not what this asserts.

@@ +1116,5 @@
> +
> +  nsresult rv = CreateCacheStorage();
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }

NS_ENSURE_SUCCESS(rv, rv)? ;)

@@ +1157,5 @@
> +  MOZ_ASSERT(aValue.isObject());
> +
> +  JS::Rooted<JSObject*> obj(aCx, &aValue.toObject());
> +  if (!obj) {
> +    FailLoaders(NS_ERROR_FAILURE);

Can this actually fail?

@@ +1162,5 @@
> +    return;
> +  }
> +
> +  Cache* cache = nullptr;
> +  nsresult rv = UNWRAP_OBJECT(Cache, obj, cache);

I wish this weren't necessary.

@@ +1163,5 @@
> +  }
> +
> +  Cache* cache = nullptr;
> +  nsresult rv = UNWRAP_OBJECT(Cache, obj, cache);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {

Should this just be asserted?  Is there any case in which we'll fail to get back a Cache?

@@ +1192,5 @@
> +  }
> +
> +  // We don't care to know the result of the promise object.
> +
> +  FailLoaders(NS_ERROR_FAILURE);

nit: extra \n above

@@ +1208,5 @@
> +    mPump->Cancel(aRv);
> +    mPump = nullptr;
> +  }
> +
> +  if (mFailed) { return; }

This is not Mozilla style.  Why is this after the mPump check?

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 68 • 9 years ago

Fixes all review comments except load infos.

I've removed the rawptrs to the workerprivate and instead pass things to functions from the ScriptLoaderRunnable.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 70 • 9 years ago

Comment on attachment 8579679 [details] [diff] [review]
Patch 1.1 - Set baseURI when script is obtained from cache

sorry, bzexport is not cooperating.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 73 • 9 years ago

When a SW script is updated (it gets a new cache), if there was already an existing running worker with the same scope and script, that would be reused and the update wouldn't happen.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 74 • 9 years ago

We don't have to bypass in ScriptLoader since this bypass will 'refresh' the cache to the new version.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 76 • 9 years ago

Shouldn't trip all the assertions by trying to get a cache name on shared workers.

	Ben Kelly [:bkelly, not reviewing]
	Comment 77 • 9 years ago

Comment on attachment 8580364 [details] [diff] [review]
Patch 7 - Call Done() after Succeed() when cache and network match

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +589,5 @@
>      }
>  
>      if (aInCacheAndEqual) {
>        Succeed();
> +      Done(NS_OK);

nit: It might be nice to make Succeed() default to calling Done().  The one case (AFAICT) that calls Succeed(), but still has work to do could override the behavior explicitly.

	Ben Kelly [:bkelly, not reviewing]
	Comment 78 • 9 years ago

Comment on attachment 8580369 [details] [diff] [review]
Patch 10 - Bypass HTTP cache when downloading ServiceWorker script to compare against.

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

I'd like to clarify the spec issue here before proceeding.

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +92,5 @@
> +      return rv;
> +    }
> +
> +    flags |= nsIRequest::LOAD_BYPASS_CACHE;
> +    rv = mChannel->SetLoadFlags(flags);

The spec says:

  Let response be the result of running fetch using r, forcing a network fetch if cached entry is greater than 1 day old.

Is there any way to do the check to see if the entry is greater than 1 day old?  Or does necko not support it?

I think we should try to do this if we can.  I agree, though, the next best thing is always bypassing.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 79 • 9 years ago

Comment on attachment 8580214 [details] [diff] [review]
Patch 1 - interdiff

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

::: dom/workers/ScriptLoader.cpp
@@ +667,5 @@
>                                IsMainWorkerScript(), this);
>        mCacheCreator->AddLoader(loader);
>      }
>  
> +    // The worker may have a null principal on first load, but in that case it's

its

@@ +796,1 @@
>                                                   ir);

can you fit this all on one line now?

@@ +996,3 @@
>  
>      DataReceived();
>      LoadingFinished(aIndex, NS_OK);

Probably easier to write this all as:

if (NS_SUCCEEDED(rv)) {
  DataReceived();
}

LoadingFinished(aIndex, rv);

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 80 • 9 years ago

Comment on attachment 8579679 [details] [diff] [review]
Patch 1.1 - Set baseURI when script is obtained from cache

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

::: dom/workers/ScriptLoader.cpp
@@ +993,5 @@
> +        return;
> +      }
> +
> +      mWorkerPrivate->SetBaseURI(finalURI);
> +    }

This needs to be rebased over rewriting the other one.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 81 • 9 years ago

When we generate the SharedWorker key, are we using that to name a file or something?  If so, we're sanitizing illegal characters, right?

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 82 • 9 years ago

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #81)
> When we generate the SharedWorker key, are we using that to name a file or
> something?  If so, we're sanitizing illegal characters, right?

No it isn't used anywhere persistent.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 83 • 9 years ago

rebased

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 84 • 9 years ago

updated as requested. r=khuey

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 85 • 9 years ago

Comment on attachment 8582085 [details] [diff] [review]
Patch 1.1 - Set baseURI when script is obtained from cache

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

::: dom/workers/ScriptLoader.cpp
@@ +988,5 @@
>        nsScriptLoader::ConvertToUTF16(nullptr, aString, aStringLen,
>                                       NS_LITERAL_STRING("UTF-8"), parentDoc,
>                                       loadInfo.mScriptTextBuf,
>                                       loadInfo.mScriptTextLength);
> +    if (NS_SUCCEEDED(rv)) {

if (NS_SUCCEEDED(rv) && IsMainWorkerScript()) {
  new uri blah blah
  if (NS_SUCCEEDED(rv)) {
    set base uri
  }
}

if (NS_SUCCEEDED(rv)) {
  DataReceived();
}

LoadingFinished ...

Do you even need the first succeeded check?  Does it hurt to set the base URI on the WorkerPrivate even if the load fails?

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 86 • 9 years ago

We don't need the check, but SetBaseURI does a fair amount of string manipulations, which is memory churn that can be avoided if we don't need it.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 89 • 9 years ago

Just updated as per my own feedback above.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 90 • 9 years ago

Moves the lines that were in SWM to ServiceWorkerScriptCache

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 91 • 9 years ago

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

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 92 • 9 years ago

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

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 93 • 9 years ago

If the failure was due to inability to create a channel (csp, other restrictions) we still want to mark the load as finished

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 94 • 9 years ago

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

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 95 • 9 years ago

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

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 97 • 9 years ago

The old version didn't go down so well. Turns out we need to ensure

1) Don't call LoadFinished with the same index multiple times (unless we are willing to relax the assertion to a if(...) { return }

2) If all imports are canceled due to an error, the imports that did create a channel, and are currently being fetched, will prevent CancelMainThread() from calling ExecuteFinishedScripts() (the firstIndex/lastIndex thing). When they do finish, the older patch, having the mCanceledByMainThread check prevented ExecuteFinishedScripts() from being called at all. I'm still waiting on this try

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

but a little more confident in this after running some local tests.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 98 • 9 years ago

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

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 99 • 9 years ago

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

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 100 • 9 years ago

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

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 103 • 9 years ago

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

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 106 • 9 years ago

please ignore the static void ContinueLoad(), i've removed it locally.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 107 • 9 years ago

also the printfs and Math.sin

	Andrea Marchesini [:baku]
	Comment 108 • 9 years ago

Comment on attachment 8587114 [details] [diff] [review]
patch 16 - Keep ServiceWorker alive during installation steps.

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

::: dom/workers/ScriptLoader.cpp
@@ +355,5 @@
>    void
>    DeleteCache();
>  
> +static void
> +ContinueLoad(nsITimer* aTimer, void* aClosure);

indentation... you don't actually use this.

@@ +569,5 @@
>  
>      if (aStatus >= Terminating && !mCanceled) {
>        mCanceled = true;
>  
> +      fprintf(stderr, "\n\nSTATUS is %d\n\n", aStatus);

ignoring... :)

@@ +1202,5 @@
>  
>  void
>  CacheCreator::RejectedCallback(JSContext* aCx, JS::Handle<JS::Value> aValue)
>  {
> +  fprintf(stderr, "CacheCreator::RejectedCallback\n");

here too.

::: dom/workers/test/serviceworkers/test_scopes.html
@@ +96,5 @@
>      });
>    }
>  
>    function runTest() {
> +  Math.sin(0);

indentation... and... why?

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 109 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd1337898e70
https://hg.mozilla.org/integration/mozilla-inbound/rev/b65a9ff67828
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8f5db0240d3
https://hg.mozilla.org/integration/mozilla-inbound/rev/90878abc7979
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa6257d4f011
https://hg.mozilla.org/integration/mozilla-inbound/rev/0006ba1831c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/e26d4bbe9a31
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3cd8548247f
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecfa5f174895
https://hg.mozilla.org/integration/mozilla-inbound/rev/b31404955302
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ab9d1e187ae
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ccfe26b7b71
https://hg.mozilla.org/integration/mozilla-inbound/rev/9193b6ecccb3
https://hg.mozilla.org/integration/mozilla-inbound/rev/9318a59b5ee5
https://hg.mozilla.org/integration/mozilla-inbound/rev/67d4baffc85c
https://hg.mozilla.org/integration/mozilla-inbound/rev/6427e5bf5dff
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3ebcd2907ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3af13399c80
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c04ce06f939
https://hg.mozilla.org/integration/mozilla-inbound/rev/0613b7a78686

	Ryan VanderMeulen [:RyanVM]
	Comment 110 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/cd1337898e70
https://hg.mozilla.org/mozilla-central/rev/b65a9ff67828
https://hg.mozilla.org/mozilla-central/rev/d8f5db0240d3
https://hg.mozilla.org/mozilla-central/rev/90878abc7979
https://hg.mozilla.org/mozilla-central/rev/aa6257d4f011
https://hg.mozilla.org/mozilla-central/rev/0006ba1831c6
https://hg.mozilla.org/mozilla-central/rev/e26d4bbe9a31
https://hg.mozilla.org/mozilla-central/rev/b3cd8548247f
https://hg.mozilla.org/mozilla-central/rev/ecfa5f174895
https://hg.mozilla.org/mozilla-central/rev/b31404955302
https://hg.mozilla.org/mozilla-central/rev/9ab9d1e187ae
https://hg.mozilla.org/mozilla-central/rev/0ccfe26b7b71
https://hg.mozilla.org/mozilla-central/rev/9193b6ecccb3
https://hg.mozilla.org/mozilla-central/rev/9318a59b5ee5
https://hg.mozilla.org/mozilla-central/rev/67d4baffc85c
https://hg.mozilla.org/mozilla-central/rev/6427e5bf5dff
https://hg.mozilla.org/mozilla-central/rev/f3ebcd2907ed
https://hg.mozilla.org/mozilla-central/rev/b3af13399c80
https://hg.mozilla.org/mozilla-central/rev/6c04ce06f939
https://hg.mozilla.org/mozilla-central/rev/0613b7a78686