Save to disk so workers are available across restarts. This bug is only concerned with saving the meta-data, the data is saved in the cache by Bug 931249.

Writes to and reads from serviceworkerstore.json. Inspired by NotificationStorage and NotificationDB.

In the spirit of Bug 899574 for Notifications. Some of the API is obviously erroneous, but the parts that are required for now work. Needs cleanup.

Comment on attachment 8400320 [details] [diff] [review] Write ServiceWorker registration metadata to profile. Feedback for the IPC roundabout JS component system please.

Ok, this patch is big and what it does is the permanent registration of ServiceWorker. In works in this way: 1. There is IPDL protocol for PBackground. 2. ServiceWorkerManager uses PBackground and when PBackground is ready it creates a ServiceWorkerManagerChild. 3. This child will receive data when ServiceWorkerManagerParent receives it from ServiceWorkerManagerService. 4. ServiceWorkerManagerParent keeps ServiceWorkerManagerService alive and it receives/sends data to it. 5. ServiceWorkerManagerService loads data from a JSON file and saves data to the same JSON file. Any new registration received from the parent is sent to the other parents, and from there to the children and, from there to ServiceWorkerManager. This code is not really tested because I want to have a feedback of this part first. Thanks!

This patch should be enough to complete this issue but I want to work a bit more on it. It does this: 1. ServiceWorkerRegister runs at startup time and it loads the content of a file with the existing registrations 2. When a new app is executed it sends the registration data for that particular origin. 3. Register/Unregister/Upload are sent via IPDL to the ServiceWorkerRegister and it saves data in a file. 4. reading/writing run on a separated thread. Open questions: 1. this patch sends the data to the app when it load the URI, but it doesn't do anything for ff desktop. 2. The execution of the SaveData() should be executed with a timer but PBackground thread doesn't have them (right?)

Comment on attachment 8447682 [details] [diff] [review] WIP Review of attachment 8447682 [details] [diff] [review]: ----------------------------------------------------------------- On further thinking, I think mInstallingWorker and registration.scriptURL do not need to be written to file. Waiting for clarification on https://github.com/slightlyoff/ServiceWorker/issues/375. You'll also want to modify SWM::Register() so that it waits until registrations are loaded to prevent collisions. Unconnected to this bug, but is their a possibility some of this information could be sent to a child process spawned for an app when the process is being created, so if we need the first page load to be controlled, it could be?

> You'll also want to modify SWM::Register() so that it waits until > registrations are loaded to prevent collisions. This cannot happen because when the app is loaded the data is already set into SWM. > Unconnected to this bug, but is their a possibility some of this information > could be sent to a child process spawned for an app when the process is > being created, so if we need the first page load to be controlled, it could > be? This should be already supported. When the process is spawned we call loadURL with the registrations for that particular origin. Does it answer your question? I have a new patch updated. Tomorrow I'll work on gtests for reading/writing the data file.

This patch is not a WIP but it's not 100% finished because I want to see some utest for reading/writing. Unfortunately today I failed to integrate gtest with xpcom components and I don't know if this is actually possible to do. bent?

gtest included.

Patch rebased.

Comment on attachment 8485875 [details] [diff] [review] service.patch Review of attachment 8485875 [details] [diff] [review]: ----------------------------------------------------------------- This looks pretty good, I'd like to see the changes. ::: caps/moz.build @@ +17,5 @@ > EXPORTS += [ > 'nsJSPrincipals.h', > 'nsNullPrincipal.h', > + 'nsPrincipal.h', > + 'nsScriptSecurityManager.h', Rather than add these here just add 'caps' to the LOCAL_INCLUDES in your new moz.build. ::: dom/ipc/TabChild.cpp @@ +1612,5 @@ > return !HasAppOwnerApp(); > } > > bool > +TabChild::RecvLoadURL(const nsCString& uri, Nit: "aURI" since you're changing this line anyway. ::: dom/ipc/TabParent.cpp @@ +509,5 @@ > return; > } > > + // This object contains the configuration for this new app. > + BrowserConfiguration configuration; We expect to add lots more here in the future so it might be nice to have a separate "InitConfiguration" method that loads this thing up. ::: dom/workers/ServiceWorkerManager.cpp @@ +59,5 @@ > + uint32_t appId = principal->GetAppId(); > + > + if (appStatus == nsIPrincipal::APP_STATUS_NOT_INSTALLED || > + appId == nsIScriptSecurityManager::NO_APP_ID || > + appId == nsIScriptSecurityManager::UNKNOWN_APP_ID) { Please double check with sicking or paul that this is correct. @@ +71,5 @@ > + return NS_OK; > + } > + > + // If we are in "app code", use manifest URL as unique origin since > + // multiple apps can share the same origin but not same broadcast messages. Nit: Copy/paste error with "broadcast messages" @@ +84,5 @@ > +} > + > +nsresult > +PopulateRegistrationData(nsPIDOMWindow* aWindow, > + const ServiceWorkerRegistrationInfo* aRegistration, Nit: Make this const& @@ +445,5 @@ > + nsRefPtr<ServiceWorkerRegistrator> swr = > + ServiceWorkerRegistrator::GetOrCreate(); > + if (!swr) { > + NS_WARNING("Failed to get the Service Worker Registrator."); > + return; MOZ_ASSERT this @@ +1761,5 @@ > + > + for (int32_t i = 0, len = aRegistrations.Length(); i < len; ++i) { > + nsRefPtr<ServiceWorkerDomainInfo> domainInfo; > + MOZ_ASSERT(!mDomainMap.Get(aRegistrations[i].mOrigin, > + getter_AddRefs(domainInfo))); Just use the single arg overload for Get() here. @@ +1793,5 @@ > + // Flush the pending requests. > + for (uint32_t i = 0, len = mPendingOperations.Length(); i < len; ++i) { > + switch (mPendingOperations[i].mType) { > + case PendingOperation::OperationRegistration: > + { Nit: { on previous line @@ +1809,5 @@ > + if (NS_FAILED(rv)) { > + NS_WARNING("Failed to dispatch a runnable."); > + return; > + } > + mActor->SendUnregisterServiceWorker(mPendingOperations[i].mScope); Hrm, you already called this ServiceWorkerManager::Unregister. If you remove this then you can de-dupe your dispatch code above. @@ +1827,5 @@ > +ServiceWorkerManager::StoreRegistration( > + nsPIDOMWindow* aWindow, > + ServiceWorkerRegistrationInfo* aRegistration) > +{ > + MOZ_ASSERT(mActor); Assert args too ::: dom/workers/ServiceWorkerManager.h @@ +101,5 @@ > { > return mScriptSpec; > } > > + void SetScriptSpec(const nsCString& aSpec) Maybe assert that aSpec is non-empty? ::: ipc/glue/BackgroundParentImpl.cpp @@ +110,5 @@ > } > > +bool > +BackgroundParentImpl::RecvRegisterServiceWorker( > + const ServiceWorkerRegistrationData& aRegistrator) These need security checks of some kind. For now you could just check BackgroundParent::IsOtherProcessActor() here and in unregister, MOZ_ASSERT and then return false. @@ +116,5 @@ > + AssertIsInMainProcess(); > + AssertIsOnBackgroundThread(); > + > + nsRefPtr<ServiceWorkerRegistrator> service = > + ServiceWorkerRegistrator::GetOrCreate(); This should be Get(), you should have always created this at startup. @@ +130,5 @@ > + AssertIsInMainProcess(); > + AssertIsOnBackgroundThread(); > + > + nsRefPtr<ServiceWorkerRegistrator> service = > + ServiceWorkerRegistrator::GetOrCreate(); Same. ::: ipc/glue/BackgroundParentImpl.h @@ +35,5 @@ > + virtual bool > + RecvRegisterServiceWorker(const ServiceWorkerRegistrationData&) MOZ_OVERRIDE; > + > + virtual bool > + RecvUnregisterServiceWorker(const nsString&) MOZ_OVERRIDE; These need param names. ::: ipc/glue/PBackground.ipdl @@ +16,5 @@ > parent: > // Only called at startup during mochitests to check the basic infrastructure. > PBackgroundTest(nsCString testArg); > + > + RegisterServiceWorker(ServiceWorkerRegistrationData registrator); This needs a PrincipalInfo. The UnregisterServiceWorker call too. Nit: s/registrator/data/ ::: toolkit/components/serviceworkerregistrator/ServiceWorkerRegistrator.cpp @@ +32,5 @@ > + { 0x9a, 0xeb, 0x56, 0x50, 0xd6, 0x4c, 0x82, 0xdf } } > + > +namespace { > + > +StaticRefPtr<ServiceWorkerRegistrator> gServiceWorkerRegistrator; I'd make this a raw pointer. @@ +34,5 @@ > +namespace { > + > +StaticRefPtr<ServiceWorkerRegistrator> gServiceWorkerRegistrator; > + > +class LoadDataRunnable MOZ_FINAL : public nsRunnable I don't think you need this, just use NS_NewRunnableMethod instead. Same for the other runnable. @@ +49,5 @@ > + return NS_OK; > + } > + > +private: > + ServiceWorkerRegistrator* mService; Holding a raw pointer is not ok here, but if you use NS_NewRunnableMethod then it will magically do the right thing. Same for the other runnable. @@ +73,5 @@ > + > +void > +GetToken(nsACString::const_iterator& aIter, > + nsACString::const_iterator& aStart, > + nsACString::const_iterator& aEnd, MOZ_ASSERT(aStart < aEnd) aEnd can be const. @@ +77,5 @@ > + nsACString::const_iterator& aEnd, > + nsACString& aToken) > +{ > + if (FindCharInReadable('\n', aIter, aEnd)) { > + aToken.Assign(Substring(aStart, aIter)); MOZ_ASSERT(*aIter == '\n')? @@ +131,5 @@ > + > + // This operation can be done only if there are not writers and the data has > + // been fully loaded from the disk. > + MutexAutoLock lock(mMutex); > + if (!mDataLoaded) { This needs to be |while (!mDataLoaded)| @@ +132,5 @@ > + // This operation can be done only if there are not writers and the data has > + // been fully loaded from the disk. > + MutexAutoLock lock(mMutex); > + if (!mDataLoaded) { > + mCondVar.Wait(); Let's make sure that we have telemetry to record the wait duration here. If it's hit frequently or if the wait is really bad we need to know about it and maybe rearchitect this. @@ +149,5 @@ > + > + // This operation can be done only if there are not writers and the data has > + // been fully loaded from the disk. > + MutexAutoLock lock(mMutex); > + if (!mDataLoaded) { Need |while| and telemetry too. Maybe move this to a helper function that handles it. @@ +166,5 @@ > +void > +ServiceWorkerRegistrator::RegisterServiceWorker( > + const ServiceWorkerRegistrationData& aData) > +{ > + MOZ_ASSERT(!NS_IsMainThread()); Let's pull in the AssertIsOnBackgroundThread function. @@ +173,5 @@ > + MOZ_ASSERT(mDataLoaded); > + > + for (uint32_t i = 0, len = mData.Length(); i < len; ++i) { > + if (mData[i].mScope == aData.mScope) { > + mData.RemoveElementAt(i); I'd just replace the element rather than removing and then appending. @@ +179,5 @@ > + } > + } > + > + mData.AppendElement(aData); > + ScheduleSaveData(); I don't think this should be protected by the lock should it? @@ +185,5 @@ > + > +void > +ServiceWorkerRegistrator::UnregisterServiceWorker(const nsACString& aScope) > +{ > + MOZ_ASSERT(!NS_IsMainThread()); AssertIsOnBackgroundThread @@ +195,5 @@ > + for (uint32_t i = 0; i < mData.Length(); ++i) { > + if (mData[i].mScope == aScope) { > + mData.RemoveElementAt(i); > + deleted = true; > + --i; You can break out here, so you don't need the 'deleted' variable, and you can call ScheduleSaveData() directly here. @@ +214,5 @@ > + nsresult rv = ReadData(); > + > + // Also if the reading failed we have to notify what is waiting for data. > + if (NS_WARN_IF(NS_FAILED(rv))) { > + NS_WARNING("Failed to load data for the ServiceWorker Registations."); One warning is probably enough :) @@ +224,5 @@ > + mCondVar.Notify(); > +} > + > +nsresult > +ServiceWorkerRegistrator::ReadData() Assert thread here. @@ +227,5 @@ > +nsresult > +ServiceWorkerRegistrator::ReadData() > +{ > + nsCOMPtr<nsIFile> file; > + nsresult rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, The directory service isn't threadsafe, you need to grab and cache this on the main thread somehow before you get to this point. Need to fix this everywhere. @@ +233,5 @@ > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return rv; > + } > + > + file->Append(NS_LITERAL_STRING(SERVICEWORKERREGISTRATOR_FILE)); This can fail... @@ +258,5 @@ > + return rv; > + } > + > + nsCString buffer; > + rv = NS_ReadInputStreamToString(stream, buffer, (uint32_t)size); Need to first verify size <= UINT32_MAX. @@ +268,5 @@ > + buffer.BeginReading(start); > + buffer.EndReading(end); > + nsACString::const_iterator iter(start); > + > + if (!FindCharInReadable('\n', iter, end)) { Rather than do all this processing yourself why not just QI |stream| to nsILineInputStream? @@ +277,5 @@ > + start = ++iter; > + > + // The file is corrupted ? > + if (!version.Equals(SERVICEWORKERREGISTRATOR_VERSION)) { > + return NS_ERROR_FAILURE; Should probably log a console error here (nsContentUtils::LogSimpleConsoleError), right? I doubt it'll be file corrupted but trying to open a newer/older version of the file if we're jumping between versions or something. @@ +283,5 @@ > + > + while (start != end) { > + ServiceWorkerRegistrationData* entry = mData.AppendElement(); > + > + GetToken(iter, start, end, entry->mOrigin); Shouldn't you check that |iter < end| after each of these? @@ +300,5 @@ > + return NS_OK; > +} > + > +void > +ServiceWorkerRegistrator::DeleteData() Assert non-main thread. @@ +312,5 @@ > + > + file->Append(NS_LITERAL_STRING(SERVICEWORKERREGISTRATOR_FILE)); > + > + bool exists; > + rv = file->Exists(&exists); You don't seem to care if it exists or not (which is fine), so don't call this. @@ +326,5 @@ > + > +void > +ServiceWorkerRegistrator::ScheduleSaveData() > +{ > + MOZ_ASSERT(!NS_IsMainThread()); This should be AssertIsOnBackgroundThread right? @@ +333,5 @@ > + return; > + } > + > + // FIXME: would be nice that this can run with a timeout after X secs and not > + // immediately. Is this a followup or something? @@ +342,5 @@ > + > + mSavingRunnable = new SaveDataRunnable(this); > + nsresult rv = target->Dispatch(mSavingRunnable, NS_DISPATCH_NORMAL); > + if (NS_FAILED(rv)) { > + NS_WARNING("Failed to dispatch the SaveDataRunnable."); Null out mSavingRunnable here. @@ +354,5 @@ > + > + mSavingRunnable = nullptr; > + > + nsresult rv = WriteData(); > + Nit: unnecessary newline @@ +362,5 @@ > + } > +} > + > +nsresult > +ServiceWorkerRegistrator::WriteData() Assert non-main thread here. @@ +371,5 @@ > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return rv; > + } > + > + file->Append(NS_LITERAL_STRING(SERVICEWORKERREGISTRATOR_FILE)); We still need to make sure we do the atomic write thing (write to tmp file, then rename). @@ +374,5 @@ > + > + file->Append(NS_LITERAL_STRING(SERVICEWORKERREGISTRATOR_FILE)); > + > + // From now we need a lock. > + MutexAutoLock lock(mMutex); If you hold a lock here then the next RegisterServiceWorker/UnregisterServiceWorker is going to block while this IO is in progress. Why not just take a snapshot of the data while holding the lock and then write from your snapshot? @@ +376,5 @@ > + > + // From now we need a lock. > + MutexAutoLock lock(mMutex); > + > + nsCString buffer; Rather than build up a buffer in memory why not just write each entry separately? There's no real downside as far as I can tell, while here you're holding double the amount of memory you really need. @@ +377,5 @@ > + // From now we need a lock. > + MutexAutoLock lock(mMutex); > + > + nsCString buffer; > + buffer.Append(SERVICEWORKERREGISTRATOR_VERSION); AppendLiteral @@ +378,5 @@ > + MutexAutoLock lock(mMutex); > + > + nsCString buffer; > + buffer.Append(SERVICEWORKERREGISTRATOR_VERSION); > + buffer.Append("\n"); Use the char version, '\n', for all the others below too. @@ +393,5 @@ > + > + buffer.Append(mData[i].mCurrentWorkerURL); > + buffer.Append("\n"); > + > + buffer.Append(SERVICEWORKERREGISTRATOR_TERMINATOR); AppendLiteral @@ +413,5 @@ > + if (count != buffer.Length()) { > + return NS_ERROR_UNEXPECTED; > + } > + > + return NS_OK; This won't actually ensure that the data has been written to disk... To do that you'd have to flush. Is that important? @@ +417,5 @@ > + return NS_OK; > +} > + > +void > +ServiceWorkerRegistrator::ProfileStarted() This is where you should cache profile dir. @@ +437,5 @@ > +ServiceWorkerRegistrator::ProfileStopped() > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + nsresult rv = WriteData(); Hrm, you might have already queued your mSavingRunnable, right? And you don't really want to call WriteData on the main thread here during shutdown since other components need to do their shutdown stuff too. Seems like you should schedule mSavingRunnable immediately if it isn't scheduled already and then spin the event loop while waiting for mSavingRunnable to become null. ::: toolkit/components/serviceworkerregistrator/ServiceWorkerRegistrator.h @@ +15,5 @@ > +#include "nsCOMPtr.h" > +#include "nsString.h" > + > +#define SERVICEWORKERREGISTRATOR_FILE "serviceworker.txt" > +#define SERVICEWORKERREGISTRATOR_VERSION "v1" I would just make this numeric. The 'v' is unnecessary. @@ +20,5 @@ > +#define SERVICEWORKERREGISTRATOR_TERMINATOR "#" > + > +class nsRunnable; > + > +class ServiceWorkerRegistrationData Let's let IPDL generate this file. Move to /dom/ipc/DOMTypes.ipdlh @@ +47,5 @@ > + NS_DECL_THREADSAFE_ISUPPORTS > + NS_DECL_NSIOBSERVER > + NS_DECL_NSISERVICEWORKERREGISTRATOR > + > + static already_AddRefed<ServiceWorkerRegistrator> GetOrCreate(); This should be Get() (or GetInstance?). You'll never create as the result of a public function. @@ +51,5 @@ > + static already_AddRefed<ServiceWorkerRegistrator> GetOrCreate(); > + > + void GetRegistrations(nsTArray<ServiceWorkerRegistrationData>& aValues); > + > + void GetRegistrationsPerOrigin( Nit: s/Per/For/ @@ +60,5 @@ > + void UnregisterServiceWorker(const nsACString& aScope); > + > + // These are for internal use only. > + void LoadData(); > + void SaveData(); These should be hidden. @@ +62,5 @@ > + // These are for internal use only. > + void LoadData(); > + void SaveData(); > + > +protected: Nit: Comment that this is subclassed by gtest @@ +83,5 @@ > + nsRefPtr<nsRunnable> mSavingRunnable; > + > +protected: > + // FIXME: this should be an hashtable. > + nsTArray<ServiceWorkerRegistrationData> mData; Please document which members need to be protected by the mutex. @@ +91,5 @@ > +// This part is useful for the serialization of data for IPDL > +namespace IPC { > + > +template <> > +struct ParamTraits<ServiceWorkerRegistrationData> This won't be needed any more. ::: toolkit/components/serviceworkerregistrator/moz.build @@ +8,5 @@ > + 'nsIServiceWorkerRegistrator.idl', > +] > + > +EXPORTS += [ > + 'ServiceWorkerRegistrator.h' I don't think you should export this, just let other modules add to LOCAL_INCLUDES. ::: toolkit/components/serviceworkerregistrator/nsIServiceWorkerRegistrator.idl @@ +4,5 @@ > + > +#include "nsISupports.idl" > + > +[scriptable, uuid(8ba1d1a6-6b25-44d4-aed1-6a79496299e6)] > +interface nsIServiceWorkerRegistrator : nsISupports As discussed, let's not make this an XPCOM component, but merely add startup/shutdown observer notifications via nsLayoutStatics::Init. Nit: s/Registrator/Registrar/ everywhere.

> > +nsresult > > +PopulateRegistrationData(nsPIDOMWindow* aWindow, > > + const ServiceWorkerRegistrationInfo* aRegistration, > > Nit: Make this const& I don't get this. > > + for (int32_t i = 0, len = aRegistrations.Length(); i < len; ++i) { > > + nsRefPtr<ServiceWorkerDomainInfo> domainInfo; > > + MOZ_ASSERT(!mDomainMap.Get(aRegistrations[i].mOrigin, > > + getter_AddRefs(domainInfo))); > > Just use the single arg overload for Get() here. I cannot. This overload doesn't exist for this kind of hashtable. > @@ +1809,5 @@ > > + if (NS_FAILED(rv)) { > > + NS_WARNING("Failed to dispatch a runnable."); > > + return; > > + } > > + mActor->SendUnregisterServiceWorker(mPendingOperations[i].mScope); > > Hrm, you already called this ServiceWorkerManager::Unregister. If you remove > this then you can de-dupe your dispatch code above. I don't understand what you are suggesting. > These need security checks of some kind. For now you could just check > BackgroundParent::IsOtherProcessActor() here and in unregister, MOZ_ASSERT > and then return false. IsOtherProcessActor needs an actor... how can I use it? > This needs a PrincipalInfo. The UnregisterServiceWorker call too. Which kind of check should I do with the principalInfo? > > +StaticRefPtr<ServiceWorkerRegistrator> gServiceWorkerRegistrator; > > I'd make this a raw pointer. Why? I have to return a reference and the first one that receives it will free it... > @@ +73,5 @@ > > + > > +void > > +GetToken(nsACString::const_iterator& aIter, > > + nsACString::const_iterator& aStart, > > + nsACString::const_iterator& aEnd, > > MOZ_ASSERT(aStart < aEnd) There is not such kind of operator in the const_iterator. > > +nsresult > > +ServiceWorkerRegistrator::ReadData() > > Assert thread here. ReadData, WriteData and DeleteData can run in any thread because of the GTest. This is the reason why I created 'LoadData', with the assertions. > Should probably log a console error here > (nsContentUtils::LogSimpleConsoleError), right? I doubt it'll be file > corrupted but trying to open a newer/older version of the file if we're > jumping between versions or something. This means to jump on the main-thread. Correct? What about if I write a comment and when we have a version 2 of this file we can do it in a follow up? > @@ +333,5 @@ > > + return; > > + } > > + > > + // FIXME: would be nice that this can run with a timeout after X secs and not > > + // immediately. > > Is this a followup or something? Sort of... Do we have support of nsITimer in PBackground thread? > > + file->Append(NS_LITERAL_STRING(SERVICEWORKERREGISTRATOR_FILE)); > > We still need to make sure we do the atomic write thing (write to tmp file, > then rename). Separated patch. > This won't actually ensure that the data has been written to disk... To do > that you'd have to flush. Is that important? Correct... but do we care? Flush is an heavy operation.

The move operation is not completely atomic because if ff crashes between the delete and the move, we loose data. I think this is fine but a feedback is welcome.

Comment on attachment 8491384 [details] [diff] [review] service.patch Review of attachment 8491384 [details] [diff] [review]: ----------------------------------------------------------------- The big problem here is raciness with writing data, especially at shutdown. But you still don't have any security checks for child processes... ::: dom/workers/ServiceWorkerManager.cpp @@ +36,5 @@ > #include "WorkerRunnable.h" > #include "WorkerScope.h" > > using namespace mozilla; > +using namespace mozilla::ipc; Nit: Alphabetize i guess. @@ +48,5 @@ > + : mType(OperationNone) > + { } > + > + enum { > + OperationNone, This seems unnecessary. Just take a valid type arg in the constructor? @@ +456,3 @@ > { > + // Register this component to PBackground. > + ipc::BackgroundChild::GetOrCreateForCurrentThread(this); Maybe MOZ_ALWAYS_TRUE() this. @@ +1777,5 @@ > + const nsTArray<ServiceWorkerRegistrationData>& aRegistrations) > +{ > + AssertIsOnMainThread(); > + > + for (int32_t i = 0, len = aRegistrations.Length(); i < len; ++i) { s/int32_t/uint32_t/ @@ +1832,5 @@ > + break; > + } > + > + default: > + MOZ_ASSUME_UNREACHABLE("This should not really happen."); make this MOZ_CRASH ::: dom/workers/ServiceWorkerRegistrar.cpp @@ +56,5 @@ > + } > +} > + > +void > +ServiceWorkerRegistrar::Shutdown() I don't think this is needed. Just use |ClearOnShutdown(&gServiceWorkerRegistrar)|. And no need to worry about the observer service, it will release your service when it's done anyway. @@ +77,5 @@ > +/* static */ already_AddRefed<ServiceWorkerRegistrar> > +ServiceWorkerRegistrar::Get() > +{ > + if (XRE_GetProcessType() != GeckoProcessType_Default) { > + return nullptr; Shouldn't this be MOZ_CRASH? @@ +87,5 @@ > +} > + > +ServiceWorkerRegistrar::ServiceWorkerRegistrar() > + : mMutex("ServiceWorkerRegistrar.mMutex") > + , mCondVar(mMutex, "ServiceWorkerRegistrar.mCondVar") Mutex+CondVar == Monitor! Let's use that instead? @@ +110,5 @@ > + // been fully loaded from the disk. > + MutexAutoLock lock(mMutex); > + WaitForDataLoaded(); > + > + mozilla::Telemetry::AccumulateTimeDelta( You shouldn't call out into other code while your mutex is held. Just scope the mutex to handle waiting and modifying |aData|, then drop the mutex and call |AccumulateTimeDelta|. @@ +111,5 @@ > + MutexAutoLock lock(mMutex); > + WaitForDataLoaded(); > + > + mozilla::Telemetry::AccumulateTimeDelta( > + mozilla::Telemetry::NETWORK_CACHE_V2_MISS_TIME_MS, This is the wrong data id. @@ +128,5 @@ > + > + // This operation can be done only if there are not writers and the data has > + // been fully loaded from the disk. > + MutexAutoLock lock(mMutex); > + WaitForDataLoaded(); This should be timed too. @@ +132,5 @@ > + WaitForDataLoaded(); > + > + aValues.Clear(); > + > + for (int32_t i = 0, len = mData.Length(); i < len; ++i) { s/int32_t/uint32_t/ @@ +205,5 @@ > + DeleteData(); > + } > + > + MutexAutoLock lock(mMutex); > + mDataLoaded = true; Maybe assert that |mDataLoaded| is still false before you set it here. @@ +247,5 @@ > + return NS_ERROR_FAILURE; > + } > + > + nsAutoCString version; > + bool isEOF = true; This is named backwards, it's supposed to represent "has more lines", not EOF. @@ +263,5 @@ > + > + while (isEOF) { > + ServiceWorkerRegistrationData* entry = mData.AppendElement(); > + > +#define GET_LINE(x) \ Please #undef this after you're done with it. @@ +266,5 @@ > + > +#define GET_LINE(x) \ > + rv = lineInputStream->ReadLine(x, &isEOF); \ > + if (NS_WARN_IF(NS_FAILED(rv))) { \ > + mData.Clear(); \ Rather than adding |mData.Clear()| everywhere here and below that something fails you should just make |LoadData()| clear |mData| if |ReadData()| returns a failure code. @@ +274,5 @@ > + mData.Clear(); \ > + return NS_ERROR_FAILURE; \ > + } > + > + Nit: Only one newline is necessary @@ +297,5 @@ > + return NS_OK; > +} > + > +void > +ServiceWorkerRegistrar::DeleteData() Shouldn't this clear mData? @@ +346,5 @@ > +ServiceWorkerRegistrar::SaveData() > +{ > + MOZ_ASSERT(!NS_IsMainThread()); > + > + mSavingRunnable = nullptr; This is pretty racy. The main thread may be spinning at shutdown waiting for this to complete, and here you unset it before you've even written anything to disk. You should definitely only let mSavingRunnable be modified on the main thread. And then you should wait unti you've finished writing before you post a runnable back to the main thread to unset mSavingRunnable. @@ +379,5 @@ > + data = mData; > + } > + > + nsCOMPtr<nsIOutputStream> stream; > + rv = NS_NewLocalFileOutputStream(getter_AddRefs(stream), file); This should be NS_NewSafeLocalFileOutputStream... Then QI to nsISafeOutputStream and call Finish() at the end of the method. @@ +384,5 @@ > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return rv; > + } > + > + nsCString buffer; nsAutoCString @@ +399,5 @@ > + return NS_ERROR_UNEXPECTED; > + } > + > + for (int32_t i = 0, len = data.Length(); i < len; ++i) { > + nsCString buffer; nsAutoCString, and it's named the same as your other |buffer| string... maybe just reuse that one and call Truncate() here? @@ +435,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + nsresult rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, > + getter_AddRefs(mProfileDir)); MOZ_ASSERT(!mProfileDir) before this. @@ +469,5 @@ > + if (!mSavingRunnable) { > + ScheduleSaveData(); > + } > + > + nsCOMPtr<nsIThread> thread(::do_GetCurrentThread()); Is the :: necessary? @@ +486,5 @@ > + > + if (!strcmp(aTopic, "profile-after-change")) { > + nsCOMPtr<nsIObserverService> observerService = > + mozilla::services::GetObserverService(); > + observerService->AddObserver(this, "profile-before-change", false); Wait, you already added this observer in Initialize... Won't ProfileStopped() be called twice? You should either keep both registrations in Init() and remove this one or remove the other in Init(). @@ +511,5 @@ > + return NS_ERROR_UNEXPECTED; > +} > + > +void > +ServiceWorkerRegistrar::WaitForDataLoaded() Add a mMutex.AssertCurrentThreadOwns() here. ::: dom/workers/ServiceWorkerRegistrar.h @@ +50,5 @@ > + // subclassing it. > + void LoadData(); > + void SaveData(); > + > + // These 3 methods must be protected by the mMutex. You need to say which *members* are protected, not which methods. Data is the important stuff. ::: dom/workers/ServiceWorkerRegistrarTypes.ipdlh @@ +8,5 @@ > +namespace dom { > + > +struct ServiceWorkerRegistrationData > +{ > + nsCString origin; Shouldn't this actually be PrincipalInfo? ::: dom/workers/moz.build @@ +104,5 @@ > > XPCSHELL_TESTS_MANIFESTS += ['test/xpcshell/xpcshell.ini'] > + > +if CONFIG['ENABLE_TESTS']: > + DIRS += ['test/gtest'] This should be TEST_DIRS, no need for the CONFIG check. ::: ipc/glue/BackgroundParentImpl.cpp @@ +5,5 @@ > #include "BackgroundParentImpl.h" > > #include "FileDescriptorSetParent.h" > #include "mozilla/Assertions.h" > +#include "mozilla/dom/ServiceWorkerRegistrar.h" Nit: 'S' comes after 'P', so move this down a line. @@ +207,5 @@ > } > > +bool > +BackgroundParentImpl::RecvRegisterServiceWorker( > + const PrincipalInfo& aPrincipalInfo, This will have to bounce to the main thread for the moment if the message is coming from a child process... Use |BackgroundParent::GetContentParent()| and if it is non-null that means the message is coming from a child process. If it's null then who cares, but otherwise package the ContentParent plus the PrincipalInfo in a runnable that you pass to the main thread. Once there, call |PrincipalInfoToPrincipal()| to get a principal, then |AssertAppPrincipal(contentParent, principal)| to make sure that the principal could have come from that process. Then you need to somehow reconcile that principal with your ServiceWorkerRegistrationData... ::: ipc/glue/BackgroundParentImpl.h @@ +61,5 @@ > MOZ_OVERRIDE; > + > + virtual bool > + RecvRegisterServiceWorker(const PrincipalInfo& aPrincipalInfo, > + const ServiceWorkerRegistrationData& aData) MOZ_OVERRIDE; Nit: Wrap at 80 chars ::: ipc/glue/BackgroundUtils.cpp @@ +135,5 @@ > return rv; > } > > + bool unknownAppId; > + uint32_t appId = nsIScriptSecurityManager::UNKNOWN_APP_ID; Nit: Move this down to just before |if (!unknownAppId) {|

Comment on attachment 8491395 [details] [diff] [review] move.patch Review of attachment 8491395 [details] [diff] [review]: ----------------------------------------------------------------- This shouldn't be needed if you switch to NS_NewSafeLocalFileOutputStream

> Use |BackgroundParent::GetContentParent()| and if it is non-null that means > the message is coming from a child process. If it's null then who cares, but > otherwise package the ContentParent plus the PrincipalInfo in a runnable > that you pass to the main thread. Once there, call > |PrincipalInfoToPrincipal()| to get a principal, then > |AssertAppPrincipal(contentParent, principal)| to make sure that the > principal could have come from that process. It's not so easy because ContentParent cannot be used from the PBackground thread: Hit MOZ_CRASH(ContentParent not thread-safe) at /home/baku/Sources/m/sw/src/dom/ipc/ContentParent.cpp:2543 > > Then you need to somehow reconcile that principal with your > ServiceWorkerRegistrationData... I don't understand why I should use PrincipalInfo in the registationData.

I fixed all your comments except the 2 open issues written in the previous comment.

Ok, ContentParent issue fixed.

race condition fixed.

Comment on attachment 8501735 [details] [diff] [review] service.patch Review of attachment 8501735 [details] [diff] [review]: ----------------------------------------------------------------- This is very close but there are still problems with threads and writing at shutdown. ::: dom/workers/ServiceWorkerManager.cpp @@ +56,5 @@ > + > + nsCOMPtr<nsIRunnable> mRunnable; > + ServiceWorkerRegistrationData mRegistration; > + nsString mScope; > + nsAutoPtr<mozilla::ipc::PrincipalInfo> mPrincipalInfo; Nit: mozilla::ipc:: shouldn't be needed here. @@ +456,3 @@ > { > + // Register this component to PBackground. > + MOZ_ALWAYS_TRUE(ipc::BackgroundChild::GetOrCreateForCurrentThread(this)); Nit: ipc:: shouldn't be needed here. There's another spot below too. @@ +1187,5 @@ > + PendingOperation* opt = mPendingOperations.AppendElement(); > + opt->mType = PendingOperation::OperationUnregistration; > + opt->mRunnable = unregisterRunnable; > + opt->mScope = aScope; > + opt->mPrincipalInfo = principalInfo; Eek, you need to call forget() here (or Move) or you'll be touching deleted memory later. @@ +1833,5 @@ > + } > + > + default: > + MOZ_CRASH("This should not really happen."); > + break; Nit: no need for break ::: dom/workers/ServiceWorkerRegistrar.cpp @@ +45,5 @@ > + if (XRE_GetProcessType() != GeckoProcessType_Default) { > + return; > + } > + > + MOZ_ASSERT(!gServiceWorkerRegistrar); This could be before the early return @@ +52,5 @@ > + > + nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService(); > + if (obs) { > + obs->AddObserver(gServiceWorkerRegistrar, "profile-after-change", false); > + obs->AddObserver(gServiceWorkerRegistrar, "profile-before-change", false); MOZ_ALWAYS_TRUE these @@ +74,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > +} > + > +ServiceWorkerRegistrar::~ServiceWorkerRegistrar() Maybe assert !mSavingNeeded here @@ +86,5 @@ > + MOZ_ASSERT(NS_IsMainThread()); > + > + TimeStamp startTime = TimeStamp::Now(); > + > + // This operation can be done only if there are not writers and the data has I don't understand this comment about writers... @@ +92,5 @@ > + { > + MonitorAutoLock lock(mMonitor); > + WaitForDataLoaded(); > + > + aValues.Clear(); This can be done outside the lock. @@ +109,5 @@ > + AssertIsOnBackgroundThread(); > + > + { > + MonitorAutoLock lock(mMonitor); > + MOZ_ASSERT(mDataLoaded); How is it possible to assert this? Couldn't a request to Register come in before you're done loading from disk? I think you may have to delay this somehow (without blocking...). @@ +137,5 @@ > + bool deleted = false; > + > + { > + MonitorAutoLock lock(mMonitor); > + MOZ_ASSERT(mDataLoaded); Same here... @@ +161,5 @@ > + MOZ_ASSERT(!mDataLoaded); > + > + nsresult rv = ReadData(); > + > + // Also if the reading failed we have to notify what is waiting for data. This comment doesn't match what the code is doing here. @@ +173,5 @@ > + mMonitor.Notify(); > +} > + > +nsresult > +ServiceWorkerRegistrar::ReadData() Since you can't assert the thread here (or in DeleteData) please just comment in both functions that it should be on a non-main non-PBackground thread during normal operation and whichever thread during gtest. @@ +175,5 @@ > + > +nsresult > +ServiceWorkerRegistrar::ReadData() > +{ > + Nit: blank line @@ +206,5 @@ > + return rv; > + } > + > + nsCOMPtr<nsILineInputStream> lineInputStream = do_QueryInterface(stream); > + if (NS_WARN_IF(!lineInputStream)) { I think this can just be asserted @@ +220,5 @@ > + > + // The file is corrupted ? > + // FIXME: in case we implement a version 2, we should inform the user using > + // the console about this issue. > + if (!version.Equals(SERVICEWORKERREGISTRAR_VERSION)) { EqualsLiteral @@ +232,5 @@ > + rv = lineInputStream->ReadLine(x, &hasMoreLines); \ > + if (NS_WARN_IF(NS_FAILED(rv))) { \ > + return rv; \ > + } \ > + if (!hasMoreLines) { \ NS_WARN_IF here too? @@ +250,5 @@ > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return rv; > + } > + > + if (!terminator.Equals(SERVICEWORKERREGISTRAR_TERMINATOR)) { EqualsLiteral @@ +277,5 @@ > + return; > + } > + > + rv = file->Remove(false); > + if (NS_WARN_IF(NS_FAILED(rv))) { This will fail if the file doesn't exist, and it's not worth first checking to see if it exists, so maybe just special case NS_ERROR_FILE_NOT_FOUND and warn about anything else? @@ +300,5 @@ > +{ > + if (!NS_IsMainThread()) { > + nsRefPtr<nsRunnable> runnable = > + new ServiceWorkerRegistrarScheduleSaveDataRunnable(); > + MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(runnable))); Hrm, why is this necessary? Can't you just dispatch to the stream transport service directly from here? @@ +305,5 @@ > + return; > + } > + > + if (mSavingRunnable) { > + mSavingNeeded = true; Wait, this is being modified on the main thread? @@ +309,5 @@ > + mSavingNeeded = true; > + return; > + } > + > + // FIXME: would be nice that this can run with a timeout after X secs and not This can be done with timers... Is it important? @@ +349,5 @@ > + MOZ_ASSERT(!NS_IsMainThread()); > + > + nsresult rv = WriteData(); > + if (NS_WARN_IF(NS_FAILED(rv))) { > + NS_WARNING("Failed to write data for the ServiceWorker Registations."); Nit: You're warning twice here. @@ +350,5 @@ > + > + nsresult rv = WriteData(); > + if (NS_WARN_IF(NS_FAILED(rv))) { > + NS_WARNING("Failed to write data for the ServiceWorker Registations."); > + DeleteData(); Hrm, with the way DeleteData modifies mData this isn't safe. The other thread could be iterating/modifying mData, and mDataLoaded has long since been set to true, so there would be nothing preventing a crash here. @@ +404,5 @@ > + > + for (uint32_t i = 0, len = data.Length(); i < len; ++i) { > + buffer.Truncate(); > + > + buffer.Append(data[i].origin()); For this first one you could remove |buffer.Truncate()| and just make this |buffer.Assign(data[i].origin())| @@ +478,5 @@ > + } > + } > + > + if (!mSavingRunnable) { > + ScheduleSaveData(); This might be unnecessary right? If nothing has changed since the last write there's no way we want to re-write it all at shutdown. @@ +485,5 @@ > + nsCOMPtr<nsIThread> thread(do_GetCurrentThread()); > + bool processed = true; > + nsresult rv = NS_OK; > + while (mSavingRunnable && NS_SUCCEEDED(rv)) { > + rv = thread->ProcessNextEvent(true, &processed); Simpler is: while (mSavingRunnable) { MOZ_ALWAYS_TRUE(NS_ProcessNextEvent(thread)); } @@ +523,5 @@ > + return NS_ERROR_UNEXPECTED; > +} > + > +void > +ServiceWorkerRegistrar::WaitForDataLoaded() This is only called in that one spot, why not just inline this? ::: dom/workers/ServiceWorkerRegistrar.h @@ +4,5 @@ > + > +#ifndef mozilla_dom_workers_ServiceWorkerRegistrar_h > +#define mozilla_dom_workers_ServiceWorkerRegistrar_h > + > +#include "ipc/IPCMessageUtils.h" Hm, why is this needed for this header? @@ +10,5 @@ > +#include "mozilla/Telemetry.h" > +#include "nsClassHashtable.h" > +#include "nsIObserver.h" > +#include "nsCOMPtr.h" > +#include "nsString.h" You need nsRefPtr.h and nsTArray.h here too. @@ +16,5 @@ > +#define SERVICEWORKERREGISTRAR_FILE "serviceworker.txt" > +#define SERVICEWORKERREGISTRAR_VERSION "1" > +#define SERVICEWORKERREGISTRAR_TERMINATOR "#" > + > +class nsRunnable; Should forward-declare nsIFile too. @@ +26,5 @@ > + > +class ServiceWorkerRegistrar : public nsIObserver > +{ > + friend class ServiceWorkerRegistrarScheduleSaveDataRunnable; > + friend class ServiceWorkerRegistrarUnsetSavingRunnable; I'd also make these nested classes. @@ +62,5 @@ > + void ProfileStopped(); > + > + void ScheduleSaveData(); > + > + mozilla::Monitor mMonitor; Nit: No need for mozilla:: @@ +69,5 @@ > + > +protected: > + // FIXME: this should be an hashtable. > + > + // mData and mDataLoaded are protected by mMonitor. Actually mData isn't *always* protected any more. It's modified on your I/O thread without holding the lock, and it's using the mDataLoaded flag to signal other threads that it's safe to touch mData. And then it's modified on the background thread while holding the lock. This all needs to be spelled out more explicitly or otherwise you could just always lock when you modify mData... ::: ipc/glue/BackgroundParentImpl.cpp @@ +210,5 @@ > delete static_cast<FileDescriptorSetParent*>(aActor); > return true; > } > > +namespace { Any reason you can't just put these class definitions in the existing anonymous namespace block? No big deal. @@ +212,5 @@ > } > > +namespace { > + > +class CallbackBase Can you just remove this class and use nsRunnable as the base for all the others? It seems wasteful to have another class that does the same thing. @@ +233,5 @@ > + AssertIsInMainProcess(); > + AssertIsOnBackgroundThread(); > + } > + > + virtual void Run() MOZ_OVERRIDE Nit: For all of these put return values on their own line. @@ +278,5 @@ > + > +class CheckPrincipalRunnable : public nsRunnable > +{ > +public: > + CheckPrincipalRunnable(nsRefPtr<ContentParent>& aParent, Nit: I like it better if this is |already_AddRefed<ContentParent>|, then at the call site you see what's going on because you have to do |new CheckPrincipalRunnable(cp.forget())|. Otherwise it's sort of a surprise that you modified the argument. @@ +333,5 @@ > + nsRefPtr<RegisterServiceWorkerCallback> callback = > + new RegisterServiceWorkerCallback(aData); > + > + nsRefPtr<ContentParent> parent = BackgroundParent::GetContentParent(this); > + if (!parent) { Maybe comment that a null ContentParent means that you're dealing with a same-process actor and that we therefore trust whatever it sends us. @@ +338,5 @@ > + callback->Run(); > + return true; > + } > + > + nsRefPtr<CheckPrincipalRunnable> runnable = Before sending this to the main thread are there any checks you can run on aData that could potentially kill the child? Like, if any of the strings are empty? Or something like that. @@ +340,5 @@ > + } > + > + nsRefPtr<CheckPrincipalRunnable> runnable = > + new CheckPrincipalRunnable(parent, aPrincipalInfo, callback); > + NS_DispatchToMainThread(runnable); MOZ_ALWAYS_TRUE here. @@ +362,5 @@ > + callback->Run(); > + return true; > + } > + > + nsRefPtr<CheckPrincipalRunnable> runnable = Same here, can you verify that aScope is not empty before dispatching to the main thread? @@ +364,5 @@ > + } > + > + nsRefPtr<CheckPrincipalRunnable> runnable = > + new CheckPrincipalRunnable(parent, aPrincipalInfo, callback); > + NS_DispatchToMainThread(runnable); MOZ_ALWAYS_TRUE here. ::: ipc/glue/BackgroundUtils.cpp @@ +135,5 @@ > return rv; > } > > + bool unknownAppId; > + uint32_t appId = nsIScriptSecurityManager::UNKNOWN_APP_ID; Let's just set appId once after we know the value of unknownAppId.

(In reply to Andrea Marchesini (:baku) from comment #14) > Correct... but do we care? Flush is an heavy operation. This should be handled by the SafeFileOutputStream now.

> > + MonitorAutoLock lock(mMonitor); > > + MOZ_ASSERT(mDataLoaded); > > How is it possible to assert this? Couldn't a request to Register come in > before you're done loading from disk? I think you may have to delay this > somehow (without blocking...). No. this cannot happen. The registration of a ServiceWorker happens from the ServiceWorkerManager and it's called only when GetRegistrations() has been called. So mDataLoaded is always true. > > @@ +300,5 @@ > > +{ > > + if (!NS_IsMainThread()) { > > + nsRefPtr<nsRunnable> runnable = > > + new ServiceWorkerRegistrarScheduleSaveDataRunnable(); > > + MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(runnable))); > > Hrm, why is this necessary? Can't you just dispatch to the stream transport > service directly from here? No. msavingNeeded is touched only on the main-thread. This is the reason why we have this runnable. > Wait, this is being modified on the main thread? This is exactly the point. > Actually mData isn't *always* protected any more. It's modified on your I/O Still it is. I have to add a mutex in DeleteData() but for the rest is ok. > Any reason you can't just put these class definitions in the existing > anonymous namespace block? No big deal. The anonymous namespace is not into mozilla::ipc... I can change it.. but we can also have 2 anonymous namespaces.

Comment on attachment 8522206 [details] [diff] [review] service.patch Review of attachment 8522206 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerRegistrar.cpp @@ +509,5 @@ > + } > + } > + > + if (!mSavingRunnable) { > + ScheduleSaveData(); This will still block every shutdown while we write the file again, regardless of whether or not the registrations have changed since the last save.

Also, I really think we should avoid going through the main thread. The whole point of PBackground is to skip the main thread when possible.

Correct, the mDataSaved was used wrongly.

Comment on attachment 8522757 [details] [diff] [review] service.patch We chatted about this, let's avoid the main thread for saving data.

Still unclear how to use IsOtherProcessActor()

Comment on attachment 8532106 [details] [diff] [review] service.patch Review of attachment 8532106 [details] [diff] [review]: ----------------------------------------------------------------- r- because this doesn't work, see the part about the PBackground actor not existing on the STS. ::: dom/workers/ServiceWorkerRegistrar.cpp @@ +56,5 @@ > + nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService(); > + if (obs) { > + nsresult rv = obs->AddObserver(gServiceWorkerRegistrar, > + "profile-after-change", false); > + MOZ_ALWAYS_TRUE(NS_SUCCEEDED(rv)); MOZ_ASSERT, but make rv be DebugOnly<nsresult> @@ +94,5 @@ > +ServiceWorkerRegistrar::GetRegistrations( > + nsTArray<ServiceWorkerRegistrationData>& aValues) > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + aValues.Clear(); Maybe just assert that aValues is empty? @@ +133,5 @@ > + > + bool found = false; > + for (uint32_t i = 0, len = mData.Length(); i < len; ++i) { > + if (mData[i].scope() == aData.scope()) { > + mData[i] = aData; Can you avoid saving data if all of the registration info is identical? That seems likely, right? @@ +235,5 @@ > + nsCOMPtr<nsILineInputStream> lineInputStream = do_QueryInterface(stream); > + MOZ_ASSERT(lineInputStream); > + > + nsAutoCString version; > + bool hasMoreLines = true; This doesn't need to be initialized @@ +260,5 @@ > + if (NS_WARN_IF(!hasMoreLines)) { \ > + return NS_ERROR_FAILURE; \ > + } > + > + Nit: extra line here @@ +357,5 @@ > + NS_WARNING("Failed to write data for the ServiceWorker Registations."); > + DeleteData(); > + } > + > + PBackgroundChild* child = BackgroundChild::GetForCurrentThread(); This won't work. You're running on the STS, it's a thread pool that doesn't have a PBackground connection. Have you tested this? ::: dom/workers/ServiceWorkerRegistrar.h @@ +75,5 @@ > + nsTArray<ServiceWorkerRegistrationData> mData; > + bool mDataLoaded; > + > + bool mShuttingDown; > + bool* mShuttingDownCompleted; Let's rename to 'mShutdownCompleteFlag'

Comment on attachment 8532260 [details] [diff] [review] service.patch Review of attachment 8532260 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerRegistrar.cpp @@ +96,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + MOZ_ASSERT(aValues.IsEmpty()); > + > + TimeStamp startTime = TimeStamp::Now(); NowLoRes() @@ +315,5 @@ > + return; > + } > +} > + > +class ServiceWorkerRegistrarSaveDataRunnable : public nsRunnable MOZ_FINAL @@ +321,5 @@ > +public: > + ServiceWorkerRegistrarSaveDataRunnable() > + { > + AssertIsOnBackgroundThread(); > + mThread = do_GetCurrentThread(); This can go in an initializer list @@ +328,5 @@ > + NS_IMETHODIMP > + Run() > + { > + nsRefPtr<ServiceWorkerRegistrar> service = ServiceWorkerRegistrar::Get(); > + if (!service) { This should be assertable @@ +335,5 @@ > + > + service->SaveData(); > + > + nsRefPtr<nsRunnable> runnable = > + NS_NewRunnableMethod(service, &ServiceWorkerRegistrar::SaveData); DataSaved @@ +336,5 @@ > + service->SaveData(); > + > + nsRefPtr<nsRunnable> runnable = > + NS_NewRunnableMethod(service, &ServiceWorkerRegistrar::SaveData); > + mThread->Dispatch(runnable, NS_DISPATCH_NORMAL); MOZ_ALWAYS_TRUE(NS_SUCCEEDED()) And then null out mThread @@ +348,5 @@ > + > +void > +ServiceWorkerRegistrar::ScheduleSaveData() > +{ > + AssertIsOnBackgroundThread(); MOZ_ASSERT(!mShuttingDown) @@ +352,5 @@ > + AssertIsOnBackgroundThread(); > + > + nsCOMPtr<nsIEventTarget> target = > + do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID); > + NS_ASSERTION(target, "Must have stream transport service"); MOZ_ASSERT @@ +407,5 @@ > + } > + > + nsRefPtr<nsRunnable> runnable = > + NS_NewRunnableMethod(this, &ServiceWorkerRegistrar::ShutdownCompleted); > + NS_DispatchToMainThread(runnable); MOZ_ALWAYS_TRUE(NS_SUCCEEDED()) Nit: 2 spaces @@ +507,5 @@ > + } > + > + nsCOMPtr<nsIEventTarget> target = > + do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID); > + NS_ASSERTION(target, "Must have stream transport service"); MOZ_ASSERT @@ +544,5 @@ > + nsCOMPtr<nsIThread> thread(do_GetCurrentThread()); > + while (true) { > + MOZ_ALWAYS_TRUE(NS_ProcessNextEvent(thread)); > + > + MonitorAutoLock lock(mMonitor); No need to lock here. @@ +545,5 @@ > + while (true) { > + MOZ_ALWAYS_TRUE(NS_ProcessNextEvent(thread)); > + > + MonitorAutoLock lock(mMonitor); > + if (completed) break; Needs braces and multiple lines. @@ +554,5 @@ > +ServiceWorkerRegistrar::Shutdown() > +{ > + AssertIsOnBackgroundThread(); > + > + mShuttingDown = true; MOZ_ASERT(!mShuttingDown) ::: ipc/glue/BackgroundParentImpl.cpp @@ +271,5 @@ > + > +class CheckPrincipalRunnable : public nsRunnable > +{ > +public: > + CheckPrincipalRunnable(already_AddRefed<ContentParent> aParent, Let's file a followup to make this verify all the data too. Everything has to be same-origin as the principal. @@ +299,5 @@ > + return NS_OK; > + } > + > + AssertIsOnBackgroundThread(); > + mCallback->Run(); Set mCallback to null here.

nsm, can you take a look at the integration of ServiceWorkerRegistar and the manager? bent, I added the serialization of the principal. The changes in dom/cache/CacheStorageParent.h are needed to compile.

Comment on attachment 8537816 [details] [diff] [review] service.patch Review of attachment 8537816 [details] [diff] [review]: ----------------------------------------------------------------- r- for the invalid initialization in LoadRegistrations. ::: dom/workers/ServiceWorkerManager.cpp @@ +1664,5 @@ > + return NS_ERROR_DOM_SECURITY_ERR; > + } > + > + if (mActor) { > + mActor->SendUnregisterServiceWorker(*principalInfo, nsString(aScope)); Can UnregisterRunnable::Run() call this? Just a suggestion, not binding. @@ +1837,5 @@ > + > + NS_ConvertUTF16toUTF8 origin(tmp); > + > + nsRefPtr<ServiceWorkerDomainInfo> domainInfo; > + MOZ_ASSERT(!mDomainMap.Get(origin, getter_AddRefs(domainInfo))); This will fail if there are multiple registrations for the same origin right? @@ +1848,5 @@ > + > + registration->mScriptSpec = aRegistrations[i].scriptSpec(); > + > + registration->mActiveWorker->SetScriptSpec( > + aRegistrations[i].currentWorkerURL()); mActiveWorker is not initialized as far as I can tell. Also, since you call StoreRegistration() in RegisterJob::Start() once, before there is an active worker, it is possible there is no stored active worker. In that case we are going to have an invalid active worker. If it is possible, please get rid of SetScriptSpec() and just use the ServiceWorkerInfo ctor. @@ +1855,5 @@ > + > +void > +ServiceWorkerManager::ActorFailed() > +{ > + MOZ_CRASH("Failed to create a PBackgroundChild actor!"); Can this only happen on OOM? Otherwise please file a followup saying we should simply run without ServiceWorkers. @@ +1884,5 @@ > + NS_WARNING("Failed to dispatch a runnable."); > + return; > + } > + mActor->SendUnregisterServiceWorker(*mPendingOperations[i].mPrincipalInfo, > + mPendingOperations[i].mScope); If UnregisterRunnable can indeed call this, you could unify the two switches.

Comment on attachment 8537816 [details] [diff] [review] service.patch Sorry, I flipped the wrong flag to r-. bent, you are still on the hook :)

I applied your comment, but I need more info about this ActiveWorker. Maybe we can talk on IRC to find a better solution because it's not clear to me know to fix it.

Comment on attachment 8540603 [details] [diff] [review] service.patch Review of attachment 8540603 [details] [diff] [review]: ----------------------------------------------------------------- r=me for the rest of ServiceWorkerManager integration. ::: dom/workers/ServiceWorkerManager.cpp @@ +1833,5 @@ > + > + registration->mScriptSpec = aRegistrations[i].scriptSpec(); > + > + registration->mActiveWorker->SetScriptSpec( > + aRegistrations[i].currentWorkerURL()); I only meant that you'll need to do registration->mActiveWorker = new ServiceWorkerInfo(registration, aRegistrations[i].currentWorkerURL()); If there is something else that needs to be discussed I'll be back to work on 2nd Jan.

Ok, then the patch is ready to land. I'll ping bent just to have a feedback about how I serialize the principal before landing it.

Comment on attachment 8541830 [details] [diff] [review] service.patch Review of attachment 8541830 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerManager.cpp @@ +107,5 @@ > +{ > + MOZ_ASSERT(aPrincipal); > + MOZ_ASSERT(aRegistration); > + > + nsresult rv = PrincipalToPrincipalInfo(aPrincipal, &aData.principal()); I think for now it's safer to block null principals. It's not clear to me how that is supposed to work, and it looks like the current code would allow two pages with null principals to use the same serviceworker... ::: dom/workers/ServiceWorkerRegistrar.cpp @@ +244,5 @@ > + > + nsCOMPtr<nsILineInputStream> lineInputStream = do_QueryInterface(stream); > + MOZ_ASSERT(lineInputStream); > + > + nsAutoCString version; Rather than making 6 autostrings in this function can't you just reuse a single one (maybe called 'currentline' or something?). You don't really need to keep each line in memory at the same time. @@ +273,5 @@ > + > + nsAutoCString type; > + GET_LINE(type); > + > + if (type.EqualsASCII(SERVICEWORKERREGISTRAR_NULL_PRINCIPAL)) { These should all use EqualsLiteral :) The difference is that EqualsASCII uses strcmp at runtime to determine length. But in these cases the string is hardcoded into the source so the compiler can determine the length for you at compile time if you use EqualsLiteral. @@ +294,5 @@ > + nsAutoCString strIsInBrowserElement; > + GET_LINE(strIsInBrowserElement); > + > + if (!strIsInBrowserElement.EqualsASCII(SERVICEWORKERREGISTRAR_TRUE) && > + !strIsInBrowserElement.EqualsASCII(SERVICEWORKERREGISTRAR_FALSE)) { EqualsLiteral @@ +299,5 @@ > + return NS_ERROR_FAILURE; > + } > + > + bool isInBrowserElement = > + strIsInBrowserElement.EqualsASCII(SERVICEWORKERREGISTRAR_TRUE); EqualsLiteral @@ +509,5 @@ > + return NS_ERROR_UNEXPECTED; > + } > + > + for (uint32_t i = 0, len = data.Length(); i < len; ++i) { > + mozilla::ipc::PrincipalInfo& info = data[i].principal(); Nit: const @@ +516,5 @@ > + buffer.AssignLiteral(SERVICEWORKERREGISTRAR_NULL_PRINCIPAL); > + } else if (info.type() == mozilla::ipc::PrincipalInfo::TSystemPrincipalInfo) { > + buffer.AssignLiteral(SERVICEWORKERREGISTRAR_SYSTEM_PRINCIPAL); > + } else { > + const mozilla::ipc::ContentPrincipalInfo& cInfo = info; Nit: use |info.get_ContentPrincipalInfo()| here rather than the implicit conversion, it makes it more clear what's happening. @@ +518,5 @@ > + buffer.AssignLiteral(SERVICEWORKERREGISTRAR_SYSTEM_PRINCIPAL); > + } else { > + const mozilla::ipc::ContentPrincipalInfo& cInfo = info; > + > + MOZ_ASSERT(info.type() == mozilla::ipc::PrincipalInfo::TContentPrincipalInfo); I'd assert this before you call |get_ContentPrincipalInfo()| above ::: dom/workers/ServiceWorkerRegistrar.h @@ +19,5 @@ > +#define SERVICEWORKERREGISTRAR_TRUE "true" > +#define SERVICEWORKERREGISTRAR_FALSE "false" > +#define SERVICEWORKERREGISTRAR_NULL_PRINCIPAL "null" > +#define SERVICEWORKERREGISTRAR_SYSTEM_PRINCIPAL "system" > +#define SERVICEWORKERREGISTRAR_CONTENT_PRINCIPAL "content" Most of these later #defines can be moved to the cpp right?

Comment on attachment 8544111 [details] [diff] [review] service.patch Review of attachment 8544111 [details] [diff] [review]: ----------------------------------------------------------------- r=me with these things fixed: ::: dom/workers/ServiceWorkerManager.cpp @@ +114,5 @@ > + return rv; > + } > + > + // No null principals. > + if (isNullPrincipal) { Maybe NS_WARN_IF here? @@ +1890,5 @@ > + } > + > + PrincipalInfo principalInfo; > + if (NS_WARN_IF(NS_FAILED(PrincipalToPrincipalInfo(aPrincipal, > + &principalInfo)))) { You shouldn't need to call this any more, you have the PrincipalInfo in 'data' now. ::: dom/workers/ServiceWorkerRegistrar.cpp @@ +285,5 @@ > + uint32_t appId = line.ToInteger(&rv); > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return rv; > + } > + Nit: Whitespace problems here and below @@ +293,5 @@ > + !line.EqualsLiteral(SERVICEWORKERREGISTRAR_FALSE)) { > + return NS_ERROR_FAILURE; > + } > + > + bool isInBrowserElement = line.EqualsLiteral(SERVICEWORKERREGISTRAR_TRUE); I think you should be more cautious here and return NS_ERROR_FAILURE if the line doesn't equal 'true' or 'false', don't just accept anything non-'true' to be 'false' here. ::: ipc/glue/BackgroundParentImpl.cpp @@ +394,5 @@ > + > +} // anonymous namespace > + > +bool > +BackgroundParentImpl::RecvRegisterServiceWorker( This needs to return false if passes a null principal now. You blocked it in the DOM, but a hacked process could still send a null principal here too. @@ +428,5 @@ > + return true; > +} > + > +bool > +BackgroundParentImpl::RecvUnregisterServiceWorker( Same here. ::: ipc/glue/PBackground.ipdl @@ +55,5 @@ > PCacheStorage(Namespace aNamespace, PrincipalInfo aPrincipalInfo); > > PMessagePort(); > > + RegisterServiceWorker(PrincipalInfo principalInfo, You don't need this anymore since ServiceWorkerRegistrationData now contains a PrincipalInfo... And having only one will make this much safer :)

Ready to land. Maple, right?

Andrea, do you want to try this on central?

Yes, I'm planning to rebase my patches on top of m-i and push them to try.

Comment on attachment 8559472 [details] [diff] [review] m-i patch Review of attachment 8559472 [details] [diff] [review]: ----------------------------------------------------------------- What are the parts that changed? Only UnregisterJob? There are also a couple of other places where we may clean up a registration that is pending uninstall, places where RemoveRegistration() is called, where we should also clean up on disk. It should be possible to grab valid principals at those places (for example the activate after unloading can use the principal of the doc it just stopped controlling). But some places we may have to wait until we get a good story for principals for a ServiceWorker, although Bug 1107516 may have fixed it.

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #51) > the principal of the doc it just stopped controlling). But some places we > may have to wait until we get a good story for principals for a > ServiceWorker, although Bug 1107516 may have fixed it. We have the ability to set the principal/load group on the service worker, but you still need to get the right principal first. I thought we agreed to serialize the registering document's PrincipalInfo when the registration is persisted. Then you would deserialize it back to an nsIPrincipal and use it here: https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#2144

> What are the parts that changed? Only UnregisterJob? No. many other parts. If you can review the ServiceWorkerManager totally would be great.

Comment on attachment 8559472 [details] [diff] [review] m-i patch Review of attachment 8559472 [details] [diff] [review]: ----------------------------------------------------------------- I believe we may come across some edge cases once we've got more testing in, but there is more value having this in the tree right now. For now this looks good with one caveat - Could you add some chrome only interface to use through mochitests that will allow all registration data to be deleted immediately? It would be useful for testing. This can be done in a follow up. Thanks for this patch.

https://tbpl.mozilla.org/?tree=Try&rev=2d5f2c35a08a I agree with you. Let's see if it's green on try. The I'll do this follow-up.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3da63ccbadef https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc89239a1711

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c786269f3cc9 - TestAppShellSteadyState, a cppunit test, didn't like you on Windows (XP and 7 opt, 8 mostly debug, which is a bizarre pattern), https://treeherder.mozilla.org/logviewer.html#?job_id=6374839&repo=mozilla-inbound

I applied this patch to my combined patch queue: https://github.com/wanderview/gecko-patches/tree/serviceworkers On trained-to-thrill in non-e10s mode, I see the registration persistence fail with: ###!!! [Parent][DispatchAsyncMessage] Error: (msgtype=0x12000B,name=PBackground::Msg_RegisterServiceWorker) Processing error: message was deserialized, but the handler returned false (indicating failure) Its failing because aData.currentWorkerURL().IsEmpty() is true in RecvRegisterServiceWorker(). Any thoughts Andrea?

Better to ask nsm. Can it happen that the currentworker URL is empty? If not, it seems a bug somewhere else. I'm going to fight against window8 this week and land the patch.

Andrea, I'm also seeing the ServiceWorkerRegistrar gtest failing. It seems to exit the process before completing TestWriteData. Seems to be hitting: Assertion failure: info.type() == mozilla::ipc::PrincipalInfo::TContentPrincipalInfo, at /builds/slave/try-l64-d-00000000000000000000/build/src/dom/workers/ServiceWorkerRegistrar.cpp:509

This patch also defines GetOriginFromWindow(), but never uses it which causes clang builds to fail on try.

Oh. The patch pushed m-i has a bunch of things fixed that weren't uploaded here. Let me switch to using that patch.

bkelly, yes, please use the m-i patch. That should be green on try. Except for a window8 issue.

Yea, the pushed m-i patch definitely works better. I still get the issue from comment 58, though. I get that failure message on my mac, but not on my windows machine. Some kind of race?

I don't know if it helps, but I actually had trouble launching firefox.exe at all on windows with this patch included. Its weird because ./mach run works, but ./mach package and the resulting firefox.exe do not.

I should say, I could not get firefox.exe to launch at all with this patch included and dom.serviceworkers.enabled set to true and e10s disabled. I did not try in other configurations.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed6136ac7ec6 This patch is fully green on try.

Removing flag since Bug 1113003 will not allow this to work anymore. Sorry Andrea. I'm happy to fix up the patch if you are busy.

I'm fixing this to handle domain info

Fixes the two places in SWM where domainInfo was used: 1) LoadRegistrations used to create a domain info. 2) CreateNewRegistration() is now on SWM. That was the only change, unfortunately I couldn't get a good interdiff.

Your patch, merged with mine. 3 files were missing. https://hg.mozilla.org/integration/mozilla-inbound/rev/b3a1efe7900a

sorry had to back this out for causing cpp unitest failures like https://treeherder.mozilla.org/logviewer.html#?job_id=6515771&repo=mozilla-inbound

https://treeherder.mozilla.org/#/jobs?repo=try&revision=73c7e17a7c1e Fix written. Pushed to try.

The latest patch is green on try and the cpp tests are fully green. Ready to land it again.