From 40fee0b3417795f50ab76049ad9df802ecd1c576 Mon Sep 17 00:00:00 2001 From: Noemi Erli Date: Tue, 24 Apr 2018 17:23:09 +0300 Subject: [PATCH] Backed out 3 changesets (bug 1456466) for mochitest failures in /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/MozPromise.h on a CLOSED TREE Backed out changeset fd0feb0058fc (bug 1456466) Backed out changeset e3a50afee79e (bug 1456466) Backed out changeset 89ce7df05344 (bug 1456466) --HG-- extra : rebase_source : b3f92fad7452a251948b88e6bf561acda162072b --- .../ServiceWorkerDescriptor.cpp | 11 - dom/serviceworkers/ServiceWorkerDescriptor.h | 4 - .../ServiceWorkerRegistration.cpp | 35 +-- .../ServiceWorkerRegistration.h | 3 +- .../ServiceWorkerRegistrationDescriptor.cpp | 11 - .../ServiceWorkerRegistrationDescriptor.h | 3 - .../ServiceWorkerRegistrationImpl.cpp | 271 +++++++++++------- .../ServiceWorkerRegistrationImpl.h | 6 +- dom/serviceworkers/test/worker_update.js | 2 +- testing/web-platform/meta/MANIFEST.json | 10 - .../service-worker/update-result.https.html | 23 -- 11 files changed, 173 insertions(+), 206 deletions(-) delete mode 100644 testing/web-platform/tests/service-workers/service-worker/update-result.https.html diff --git a/dom/serviceworkers/ServiceWorkerDescriptor.cpp b/dom/serviceworkers/ServiceWorkerDescriptor.cpp index 5b67c327e613..4a72a2f97619 100644 --- a/dom/serviceworkers/ServiceWorkerDescriptor.cpp +++ b/dom/serviceworkers/ServiceWorkerDescriptor.cpp @@ -12,9 +12,6 @@ namespace mozilla { namespace dom { -using mozilla::ipc::PrincipalInfo; -using mozilla::ipc::PrincipalInfoToPrincipal; - ServiceWorkerDescriptor::ServiceWorkerDescriptor(uint64_t aId, nsIPrincipal* aPrincipal, const nsACString& aScope, @@ -98,14 +95,6 @@ ServiceWorkerDescriptor::PrincipalInfo() const return mData->principalInfo(); } -nsCOMPtr -ServiceWorkerDescriptor::GetPrincipal() const -{ - AssertIsOnMainThread(); - nsCOMPtr ref = PrincipalInfoToPrincipal(mData->principalInfo()); - return Move(ref); -} - const nsCString& ServiceWorkerDescriptor::Scope() const { diff --git a/dom/serviceworkers/ServiceWorkerDescriptor.h b/dom/serviceworkers/ServiceWorkerDescriptor.h index 5fdf95a586d5..042def1f30bd 100644 --- a/dom/serviceworkers/ServiceWorkerDescriptor.h +++ b/dom/serviceworkers/ServiceWorkerDescriptor.h @@ -7,7 +7,6 @@ #define _mozilla_dom_ServiceWorkerDescriptor_h #include "mozilla/UniquePtr.h" -#include "nsCOMPtr.h" #include "nsString.h" class nsIPrincipal; @@ -70,9 +69,6 @@ public: const mozilla::ipc::PrincipalInfo& PrincipalInfo() const; - nsCOMPtr - GetPrincipal() const; - const nsCString& Scope() const; diff --git a/dom/serviceworkers/ServiceWorkerRegistration.cpp b/dom/serviceworkers/ServiceWorkerRegistration.cpp index 4155ebafa072..ae739af13ceb 100644 --- a/dom/serviceworkers/ServiceWorkerRegistration.cpp +++ b/dom/serviceworkers/ServiceWorkerRegistration.cpp @@ -198,40 +198,7 @@ ServiceWorkerRegistration::Update(ErrorResult& aRv) aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR); return nullptr; } - - nsIGlobalObject* global = GetParentObject(); - if (!global) { - aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR); - return nullptr; - } - - RefPtr outer = Promise::Create(global, aRv); - if (NS_WARN_IF(aRv.Failed())) { - return nullptr; - } - - RefPtr self = this; - - mInner->Update(aRv)->Then( - global->EventTargetFor(TaskCategory::Other), __func__, - [outer, self](const ServiceWorkerRegistrationDescriptor& aDesc) { - nsIGlobalObject* global = self->GetParentObject(); - if (!global) { - outer->MaybeReject(NS_ERROR_DOM_INVALID_STATE_ERR); - return; - } - RefPtr ref = - global->GetOrCreateServiceWorkerRegistration(aDesc); - if (!ref) { - outer->MaybeReject(NS_ERROR_DOM_INVALID_STATE_ERR); - return; - } - outer->MaybeResolve(ref); - }, [outer] (ErrorResult&& aRv) { - outer->MaybeReject(aRv); - }); - - return outer.forget(); + return mInner->Update(aRv); } already_AddRefed diff --git a/dom/serviceworkers/ServiceWorkerRegistration.h b/dom/serviceworkers/ServiceWorkerRegistration.h index 6ac60a45f278..90b900d85366 100644 --- a/dom/serviceworkers/ServiceWorkerRegistration.h +++ b/dom/serviceworkers/ServiceWorkerRegistration.h @@ -12,7 +12,6 @@ #include "mozilla/dom/ServiceWorkerBinding.h" #include "mozilla/dom/ServiceWorkerRegistrationBinding.h" #include "mozilla/dom/ServiceWorkerRegistrationDescriptor.h" -#include "mozilla/dom/ServiceWorkerUtils.h" // Support for Notification API extension. #include "mozilla/dom/NotificationBinding.h" @@ -44,7 +43,7 @@ public: virtual void ClearServiceWorkerRegistration(ServiceWorkerRegistration* aReg) = 0; - virtual RefPtr + virtual already_AddRefed Update(ErrorResult& aRv) = 0; virtual already_AddRefed diff --git a/dom/serviceworkers/ServiceWorkerRegistrationDescriptor.cpp b/dom/serviceworkers/ServiceWorkerRegistrationDescriptor.cpp index 080ad9fff777..3820b648e022 100644 --- a/dom/serviceworkers/ServiceWorkerRegistrationDescriptor.cpp +++ b/dom/serviceworkers/ServiceWorkerRegistrationDescriptor.cpp @@ -13,9 +13,6 @@ namespace mozilla { namespace dom { -using mozilla::ipc::PrincipalInfo; -using mozilla::ipc::PrincipalInfoToPrincipal; - Maybe ServiceWorkerRegistrationDescriptor::NewestInternal() const { @@ -133,14 +130,6 @@ ServiceWorkerRegistrationDescriptor::PrincipalInfo() const return mData->principalInfo(); } -nsCOMPtr -ServiceWorkerRegistrationDescriptor::GetPrincipal() const -{ - AssertIsOnMainThread(); - nsCOMPtr ref = PrincipalInfoToPrincipal(mData->principalInfo()); - return Move(ref); -} - const nsCString& ServiceWorkerRegistrationDescriptor::Scope() const { diff --git a/dom/serviceworkers/ServiceWorkerRegistrationDescriptor.h b/dom/serviceworkers/ServiceWorkerRegistrationDescriptor.h index c664462b0a83..f68fac8ebc9c 100644 --- a/dom/serviceworkers/ServiceWorkerRegistrationDescriptor.h +++ b/dom/serviceworkers/ServiceWorkerRegistrationDescriptor.h @@ -73,9 +73,6 @@ public: const mozilla::ipc::PrincipalInfo& PrincipalInfo() const; - nsCOMPtr - GetPrincipal() const; - const nsCString& Scope() const; diff --git a/dom/serviceworkers/ServiceWorkerRegistrationImpl.cpp b/dom/serviceworkers/ServiceWorkerRegistrationImpl.cpp index 39ec170e4fb6..21b4c8ed7631 100644 --- a/dom/serviceworkers/ServiceWorkerRegistrationImpl.cpp +++ b/dom/serviceworkers/ServiceWorkerRegistrationImpl.cpp @@ -43,7 +43,6 @@ namespace dom { ServiceWorkerRegistrationMainThread::ServiceWorkerRegistrationMainThread(const ServiceWorkerRegistrationDescriptor& aDescriptor) : mOuter(nullptr) - , mDescriptor(aDescriptor) , mScope(NS_ConvertUTF8toUTF16(aDescriptor.Scope())) , mListeningForEvents(false) { @@ -107,7 +106,6 @@ ServiceWorkerRegistrationMainThread::UpdateFound() void ServiceWorkerRegistrationMainThread::UpdateState(const ServiceWorkerRegistrationDescriptor& aDescriptor) { - mDescriptor = aDescriptor; mOuter->UpdateState(aDescriptor); } @@ -170,47 +168,15 @@ UpdateInternal(nsIPrincipal* aPrincipal, class MainThreadUpdateCallback final : public ServiceWorkerUpdateFinishCallback { - RefPtr mPromise; + PromiseWindowProxy mPromise; ~MainThreadUpdateCallback() { } public: - MainThreadUpdateCallback() - : mPromise(new ServiceWorkerRegistrationPromise::Private(__func__)) - { - } - - void - UpdateSucceeded(ServiceWorkerRegistrationInfo* aRegistration) override - { - mPromise->Resolve(aRegistration->Descriptor(), __func__); - } - - void - UpdateFailed(ErrorResult& aStatus) override - { - mPromise->Reject(Move(aStatus), __func__); - } - - RefPtr - Promise() const - { - return mPromise; - } -}; - -class WorkerThreadUpdateCallback final : public ServiceWorkerUpdateFinishCallback -{ - RefPtr mPromise; - - ~WorkerThreadUpdateCallback() - { - } - -public: - explicit WorkerThreadUpdateCallback(ServiceWorkerRegistrationPromise::Private* aPromise) - : mPromise(aPromise) + explicit MainThreadUpdateCallback(nsPIDOMWindowInner* aWindow, + Promise* aPromise) + : mPromise(aWindow, aPromise) { MOZ_ASSERT(NS_IsMainThread()); } @@ -218,13 +184,115 @@ public: void UpdateSucceeded(ServiceWorkerRegistrationInfo* aRegistration) override { - mPromise->Resolve(aRegistration->Descriptor(), __func__); + RefPtr promise = mPromise.Get(); + nsCOMPtr win = mPromise.GetWindow(); + if (!promise || !win) { + return; + } + + nsCOMPtr r = NS_NewRunnableFunction( + "MainThreadUpdateCallback::UpdateSucceeded", + [promise = Move(promise)] () { + promise->MaybeResolveWithUndefined(); + }); + MOZ_ALWAYS_SUCCEEDS( + win->EventTargetFor(TaskCategory::Other)->Dispatch(r.forget())); } void UpdateFailed(ErrorResult& aStatus) override { - mPromise->Reject(Move(aStatus), __func__); + if (RefPtr promise = mPromise.Get()) { + promise->MaybeReject(aStatus); + } + } +}; + +class UpdateResultRunnable final : public WorkerRunnable +{ + RefPtr mPromiseProxy; + IPC::Message mSerializedErrorResult; + + ~UpdateResultRunnable() + {} + +public: + UpdateResultRunnable(PromiseWorkerProxy* aPromiseProxy, ErrorResult& aStatus) + : WorkerRunnable(aPromiseProxy->GetWorkerPrivate()) + , mPromiseProxy(aPromiseProxy) + { + // ErrorResult is not thread safe. Serialize it for transfer across + // threads. + IPC::WriteParam(&mSerializedErrorResult, aStatus); + aStatus.SuppressException(); + } + + bool + WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override + { + // Deserialize the ErrorResult now that we are back in the worker + // thread. + ErrorResult status; + PickleIterator iter = PickleIterator(mSerializedErrorResult); + Unused << IPC::ReadParam(&mSerializedErrorResult, &iter, &status); + + Promise* promise = mPromiseProxy->WorkerPromise(); + if (status.Failed()) { + promise->MaybeReject(status); + } else { + promise->MaybeResolveWithUndefined(); + } + status.SuppressException(); + mPromiseProxy->CleanUp(); + return true; + } +}; + +class WorkerThreadUpdateCallback final : public ServiceWorkerUpdateFinishCallback +{ + RefPtr mPromiseProxy; + + ~WorkerThreadUpdateCallback() + { + } + +public: + explicit WorkerThreadUpdateCallback(PromiseWorkerProxy* aPromiseProxy) + : mPromiseProxy(aPromiseProxy) + { + MOZ_ASSERT(NS_IsMainThread()); + } + + void + UpdateSucceeded(ServiceWorkerRegistrationInfo* aRegistration) override + { + ErrorResult rv(NS_OK); + Finish(rv); + } + + void + UpdateFailed(ErrorResult& aStatus) override + { + Finish(aStatus); + } + + void + Finish(ErrorResult& aStatus) + { + if (!mPromiseProxy) { + return; + } + + RefPtr proxy = mPromiseProxy.forget(); + + MutexAutoLock lock(proxy->Lock()); + if (proxy->CleanedUp()) { + return; + } + + RefPtr r = + new UpdateResultRunnable(proxy, aStatus); + r->Dispatch(); } }; @@ -262,12 +330,19 @@ class SWRUpdateRunnable final : public Runnable }; public: - explicit SWRUpdateRunnable(const ServiceWorkerDescriptor& aDescriptor) + explicit SWRUpdateRunnable(PromiseWorkerProxy* aPromiseProxy) : Runnable("dom::SWRUpdateRunnable") - , mPromise(new ServiceWorkerRegistrationPromise::Private(__func__)) - , mDescriptor(aDescriptor) + , mPromiseProxy(aPromiseProxy) + , mDescriptor(aPromiseProxy->GetWorkerPrivate()->GetServiceWorkerDescriptor()) , mDelayed(false) { + MOZ_ASSERT(mPromiseProxy); + + // This runnable is used for update calls originating from a worker thread, + // which may be delayed in some cases. + MOZ_ASSERT(mPromiseProxy->GetWorkerPrivate()->IsServiceWorker()); + MOZ_ASSERT(mPromiseProxy->GetWorkerPrivate()); + mPromiseProxy->GetWorkerPrivate()->AssertIsOnWorkerThread(); } NS_IMETHOD @@ -276,15 +351,21 @@ public: MOZ_ASSERT(NS_IsMainThread()); ErrorResult result; - nsCOMPtr principal = mDescriptor.GetPrincipal(); - if (NS_WARN_IF(!principal)) { - mPromise->Reject(NS_ERROR_DOM_INVALID_STATE_ERR, __func__); - return NS_OK; + nsCOMPtr principal; + // UpdateInternal may try to reject the promise synchronously leading + // to a deadlock. + { + MutexAutoLock lock(mPromiseProxy->Lock()); + if (mPromiseProxy->CleanedUp()) { + return NS_OK; + } + + principal = mPromiseProxy->GetWorkerPrivate()->GetPrincipal(); } + MOZ_ASSERT(principal); RefPtr swm = ServiceWorkerManager::GetInstance(); if (NS_WARN_IF(!swm)) { - mPromise->Reject(NS_ERROR_DOM_INVALID_STATE_ERR, __func__); return NS_OK; } @@ -330,22 +411,18 @@ public: } RefPtr cb = - new WorkerThreadUpdateCallback(mPromise); + new WorkerThreadUpdateCallback(mPromiseProxy); UpdateInternal(principal, mDescriptor.Scope(), cb); - return NS_OK; } - RefPtr - Promise() const +private: + ~SWRUpdateRunnable() { - return mPromise; + MOZ_ASSERT(NS_IsMainThread()); } -private: - ~SWRUpdateRunnable() = default; - - RefPtr mPromise; + RefPtr mPromiseProxy; const ServiceWorkerDescriptor mDescriptor; bool mDelayed; }; @@ -544,22 +621,31 @@ public: }; } // namespace -RefPtr +already_AddRefed ServiceWorkerRegistrationMainThread::Update(ErrorResult& aRv) { MOZ_ASSERT(NS_IsMainThread()); MOZ_DIAGNOSTIC_ASSERT(mOuter); - nsCOMPtr principal = mDescriptor.GetPrincipal(); - if (!principal) { - return ServiceWorkerRegistrationPromise::CreateAndReject( - NS_ERROR_DOM_INVALID_STATE_ERR, __func__); + nsCOMPtr go = mOuter->GetParentObject(); + if (!go) { + aRv.Throw(NS_ERROR_FAILURE); + return nullptr; } - RefPtr cb = new MainThreadUpdateCallback(); - UpdateInternal(principal, NS_ConvertUTF16toUTF8(mScope), cb); + RefPtr promise = Promise::Create(go, aRv); + if (NS_WARN_IF(aRv.Failed())) { + return nullptr; + } - return cb->Promise(); + nsCOMPtr doc = mOuter->GetOwner()->GetExtantDoc(); + MOZ_ASSERT(doc); + + RefPtr cb = + new MainThreadUpdateCallback(mOuter->GetOwner(), promise); + UpdateInternal(doc->NodePrincipal(), NS_ConvertUTF16toUTF8(mScope), cb); + + return promise.forget(); } already_AddRefed @@ -804,7 +890,6 @@ private: ServiceWorkerRegistrationWorkerThread::ServiceWorkerRegistrationWorkerThread(const ServiceWorkerRegistrationDescriptor& aDescriptor) : mOuter(nullptr) - , mDescriptor(aDescriptor) , mScope(NS_ConvertUTF8toUTF16(aDescriptor.Scope())) { } @@ -843,56 +928,36 @@ ServiceWorkerRegistrationWorkerThread::ClearServiceWorkerRegistration(ServiceWor mOuter = nullptr; } -RefPtr +already_AddRefed ServiceWorkerRegistrationWorkerThread::Update(ErrorResult& aRv) { - if (NS_WARN_IF(!mWorkerRef->GetPrivate())) { - return ServiceWorkerRegistrationPromise::CreateAndReject( - NS_ERROR_DOM_INVALID_STATE_ERR, __func__); - } + WorkerPrivate* worker = GetCurrentThreadWorkerPrivate(); + MOZ_ASSERT(worker); + worker->AssertIsOnWorkerThread(); - RefPtr workerRef = - StrongWorkerRef::Create(mWorkerRef->GetPrivate(), - "ServiceWorkerRegistration::Update"); - if (NS_WARN_IF(!workerRef)) { - return ServiceWorkerRegistrationPromise::CreateAndReject( - NS_ERROR_DOM_INVALID_STATE_ERR, __func__); + RefPtr promise = Promise::Create(worker->GlobalScope(), aRv); + if (aRv.Failed()) { + return nullptr; } // Avoid infinite update loops by ignoring update() calls during top // level script evaluation. See: // https://github.com/slightlyoff/ServiceWorker/issues/800 - if (workerRef->Private()->LoadScriptAsPartOfLoadingServiceWorkerScript()) { - return ServiceWorkerRegistrationPromise::CreateAndResolve(mDescriptor, - __func__); + if (worker->LoadScriptAsPartOfLoadingServiceWorkerScript()) { + promise->MaybeResolveWithUndefined(); + return promise.forget(); } - // Eventually we need to support all workers, but for right now this - // code assumes we're on a service worker global as self.registration. - if (NS_WARN_IF(!workerRef->Private()->IsServiceWorker())) { - return ServiceWorkerRegistrationPromise::CreateAndReject( - NS_ERROR_DOM_INVALID_STATE_ERR, __func__); + RefPtr proxy = PromiseWorkerProxy::Create(worker, promise); + if (!proxy) { + aRv.Throw(NS_ERROR_DOM_ABORT_ERR); + return nullptr; } - RefPtr r = - new SWRUpdateRunnable(workerRef->Private()->GetServiceWorkerDescriptor()); - nsresult rv = workerRef->Private()->DispatchToMainThread(r); - if (NS_FAILED(rv)) { - return ServiceWorkerRegistrationPromise::CreateAndReject( - NS_ERROR_DOM_INVALID_STATE_ERR, __func__); - } + RefPtr r = new SWRUpdateRunnable(proxy); + MOZ_ALWAYS_SUCCEEDS(worker->DispatchToMainThread(r.forget())); - RefPtr outer = - new ServiceWorkerRegistrationPromise::Private(__func__); - - r->Promise()->Then(workerRef->Private()->HybridEventTarget(), __func__, - [workerRef, outer] (const ServiceWorkerRegistrationDescriptor& aDesc) { - outer->Resolve(aDesc, __func__); - }, [workerRef, outer] (ErrorResult&& aRv) { - outer->Reject(Move(aRv), __func__); - }); - - return outer.forget(); + return promise.forget(); } already_AddRefed diff --git a/dom/serviceworkers/ServiceWorkerRegistrationImpl.h b/dom/serviceworkers/ServiceWorkerRegistrationImpl.h index 39b035ff2912..1cdca7847379 100644 --- a/dom/serviceworkers/ServiceWorkerRegistrationImpl.h +++ b/dom/serviceworkers/ServiceWorkerRegistrationImpl.h @@ -42,7 +42,7 @@ public: void ClearServiceWorkerRegistration(ServiceWorkerRegistration* aReg) override; - RefPtr + already_AddRefed Update(ErrorResult& aRv) override; already_AddRefed @@ -93,7 +93,6 @@ private: RegistrationRemovedInternal(); ServiceWorkerRegistration* mOuter; - ServiceWorkerRegistrationDescriptor mDescriptor; const nsString mScope; bool mListeningForEvents; }; @@ -122,7 +121,7 @@ public: void ClearServiceWorkerRegistration(ServiceWorkerRegistration* aReg) override; - RefPtr + already_AddRefed Update(ErrorResult& aRv) override; already_AddRefed @@ -158,7 +157,6 @@ private: GetWorkerPrivate(const MutexAutoLock& aProofOfLock); ServiceWorkerRegistration* mOuter; - const ServiceWorkerRegistrationDescriptor mDescriptor; const nsString mScope; RefPtr mListener; RefPtr mWorkerRef; diff --git a/dom/serviceworkers/test/worker_update.js b/dom/serviceworkers/test/worker_update.js index 3de94e45762a..9f3e55b18a3b 100644 --- a/dom/serviceworkers/test/worker_update.js +++ b/dom/serviceworkers/test/worker_update.js @@ -3,7 +3,7 @@ // test actual update scenarios with a SJS test. onmessage = function(e) { self.registration.update().then(function(v) { - return v instanceof ServiceWorkerRegistration ? 'FINISH' : 'FAIL'; + return v === undefined ? 'FINISH' : 'FAIL'; }).catch(function(e) { return 'FAIL'; }).then(function(result) { diff --git a/testing/web-platform/meta/MANIFEST.json b/testing/web-platform/meta/MANIFEST.json index bf379385d51f..d6ac4ae71432 100644 --- a/testing/web-platform/meta/MANIFEST.json +++ b/testing/web-platform/meta/MANIFEST.json @@ -364339,12 +364339,6 @@ {} ] ], - "service-workers/service-worker/update-result.https.html": [ - [ - "/service-workers/service-worker/update-result.https.html", - {} - ] - ], "service-workers/service-worker/update.https.html": [ [ "/service-workers/service-worker/update.https.html", @@ -599326,10 +599320,6 @@ "aac5705d6844e4a33200418504adb57053a45be2", "testharness" ], - "service-workers/service-worker/update-result.https.html": [ - "06741e887be9746d7354394f74c054dd920d1b60", - "testharness" - ], "service-workers/service-worker/update.https.html": [ "d55da98b05b5885084474ebdbabdf6c0998f8bca", "testharness" diff --git a/testing/web-platform/tests/service-workers/service-worker/update-result.https.html b/testing/web-platform/tests/service-workers/service-worker/update-result.https.html deleted file mode 100644 index d8ed94f77665..000000000000 --- a/testing/web-platform/tests/service-workers/service-worker/update-result.https.html +++ /dev/null @@ -1,23 +0,0 @@ - -Service Worker: update() should resolve a ServiceWorkerRegistration - - - - - -