From 07b5d9905cac89ce61abbda0f7765d3de676d526 Mon Sep 17 00:00:00 2001 From: Kagami Sascha Rosylight Date: Thu, 13 Jul 2023 12:32:19 +0000 Subject: [PATCH] Bug 1842289 - Part 2: Grab WorkerRef outside LockManager constructor r=dom-worker-reviewers,webidl,smaug Differential Revision: https://phabricator.services.mozilla.com/D183048 --- dom/base/Navigator.cpp | 2 +- dom/locks/LockManager.cpp | 47 +++++++++++++++++++++++-------- dom/locks/LockManager.h | 4 +++ dom/webidl/Lock.webidl | 9 ++++++ dom/webidl/LockManager.webidl | 9 ++++++ dom/webidl/Navigator.webidl | 2 +- dom/webidl/WorkerNavigator.webidl | 2 +- dom/workers/WorkerNavigator.cpp | 2 +- 8 files changed, 61 insertions(+), 16 deletions(-) diff --git a/dom/base/Navigator.cpp b/dom/base/Navigator.cpp index eedeee0364ce..e856d68c61f2 100644 --- a/dom/base/Navigator.cpp +++ b/dom/base/Navigator.cpp @@ -2258,7 +2258,7 @@ webgpu::Instance* Navigator::Gpu() { dom::LockManager* Navigator::Locks() { if (!mLocks) { - mLocks = new dom::LockManager(GetWindow()->AsGlobal()); + mLocks = dom::LockManager::Create(*GetWindow()->AsGlobal()); } return mLocks; } diff --git a/dom/locks/LockManager.cpp b/dom/locks/LockManager.cpp index 4d86118de066..d890da60c293 100644 --- a/dom/locks/LockManager.cpp +++ b/dom/locks/LockManager.cpp @@ -36,32 +36,45 @@ JSObject* LockManager::WrapObject(JSContext* aCx, LockManager::LockManager(nsIGlobalObject* aGlobal) : mOwner(aGlobal) { Maybe clientInfo = aGlobal->GetClientInfo(); if (!clientInfo) { + // Pass the nonworking object and let request()/query() throw. return; } nsCOMPtr principal = clientInfo->GetPrincipal().unwrapOr(nullptr); if (!principal || !principal->GetIsContentPrincipal()) { + // Same, the methods will throw instead of the constructor. return; } mozilla::ipc::PBackgroundChild* backgroundActor = mozilla::ipc::BackgroundChild::GetOrCreateForCurrentThread(); mActor = new locks::LockManagerChild(aGlobal); - backgroundActor->SendPLockManagerConstructor(mActor, WrapNotNull(principal), - clientInfo->Id()); + + if (!backgroundActor->SendPLockManagerConstructor( + mActor, WrapNotNull(principal), clientInfo->Id())) { + // Failed to construct the actor. Pass the nonworking object and let the + // methods throw. + mActor = nullptr; + return; + } +} + +already_AddRefed LockManager::Create(nsIGlobalObject& aGlobal) { + RefPtr manager = new LockManager(&aGlobal); if (!NS_IsMainThread()) { - mWorkerRef = WeakWorkerRef::Create(GetCurrentThreadWorkerPrivate(), - [self = RefPtr(this)]() { - // Others may grab a strong reference - // and block immediate destruction. - // Shutdown early as we don't have to - // wait for them. - self->Shutdown(); - self->mWorkerRef = nullptr; - }); + // Grabbing WorkerRef may fail and that will cause the methods throw later. + manager->mWorkerRef = + WeakWorkerRef::Create(GetCurrentThreadWorkerPrivate(), [manager]() { + // Others may grab a strong reference and block immediate destruction. + // Shutdown early as we don't have to wait for them. + manager->Shutdown(); + manager->mWorkerRef = nullptr; + }); } + + return manager.forget(); } static bool ValidateRequestArguments(const nsAString& name, @@ -140,7 +153,7 @@ already_AddRefed LockManager::Request(const nsAString& aName, if (!allowed) { // Step 4: If origin is an opaque origin, then return a promise rejected // with a "SecurityError" DOMException. - // But per https://wicg.github.io/web-locks/#lock-managers this really means + // But per https://w3c.github.io/web-locks/#lock-managers this really means // whether it has storage access. aRv.ThrowSecurityError("request() is not allowed in this context"); return nullptr; @@ -152,6 +165,11 @@ already_AddRefed LockManager::Request(const nsAString& aName, return nullptr; } + if (!NS_IsMainThread() && !mWorkerRef) { + aRv.ThrowInvalidStateError("request() is not allowed at this point"); + return nullptr; + } + if (!ValidateRequestArguments(aName, aOptions, aRv)) { return nullptr; } @@ -183,6 +201,11 @@ already_AddRefed LockManager::Query(ErrorResult& aRv) { return nullptr; } + if (!NS_IsMainThread() && !mWorkerRef) { + aRv.ThrowInvalidStateError("query() is not allowed at this point"); + return nullptr; + } + RefPtr promise = Promise::Create(mOwner, aRv); if (aRv.Failed()) { return nullptr; diff --git a/dom/locks/LockManager.h b/dom/locks/LockManager.h index 269710dba7b9..19313ef6b87e 100644 --- a/dom/locks/LockManager.h +++ b/dom/locks/LockManager.h @@ -36,8 +36,12 @@ class LockManager final : public nsISupports, public nsWrapperCache { NS_DECL_CYCLE_COLLECTING_ISUPPORTS NS_DECL_CYCLE_COLLECTION_WRAPPERCACHE_CLASS(LockManager) + private: explicit LockManager(nsIGlobalObject* aGlobal); + public: + static already_AddRefed Create(nsIGlobalObject& aGlobal); + nsIGlobalObject* GetParentObject() const { return mOwner; } JSObject* WrapObject(JSContext* aCx, diff --git a/dom/webidl/Lock.webidl b/dom/webidl/Lock.webidl index bed5dc59a296..e03b977629f5 100644 --- a/dom/webidl/Lock.webidl +++ b/dom/webidl/Lock.webidl @@ -1,3 +1,12 @@ +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. + * + * The origin of this IDL file is + * https://w3c.github.io/web-locks/ + */ + [SecureContext, Exposed=(Window,Worker), Pref="dom.weblocks.enabled"] interface Lock { readonly attribute DOMString name; diff --git a/dom/webidl/LockManager.webidl b/dom/webidl/LockManager.webidl index 88102c1d59fa..02fb2fe69c42 100644 --- a/dom/webidl/LockManager.webidl +++ b/dom/webidl/LockManager.webidl @@ -1,3 +1,12 @@ +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. + * + * The origin of this IDL file is + * https://w3c.github.io/web-locks/ + */ + [SecureContext, Exposed=(Window,Worker), Pref="dom.weblocks.enabled"] interface LockManager { [NewObject] diff --git a/dom/webidl/Navigator.webidl b/dom/webidl/Navigator.webidl index 56cf1c0f4592..9e6293a9ced1 100644 --- a/dom/webidl/Navigator.webidl +++ b/dom/webidl/Navigator.webidl @@ -338,7 +338,7 @@ partial interface Navigator { readonly attribute MediaSession mediaSession; }; -// https://wicg.github.io/web-locks/#navigator-mixins +// https://w3c.github.io/web-locks/#navigator-mixins [SecureContext] interface mixin NavigatorLocks { [Pref="dom.weblocks.enabled"] diff --git a/dom/webidl/WorkerNavigator.webidl b/dom/webidl/WorkerNavigator.webidl index 1f544e338256..d1817b09a5e9 100644 --- a/dom/webidl/WorkerNavigator.webidl +++ b/dom/webidl/WorkerNavigator.webidl @@ -28,7 +28,7 @@ partial interface WorkerNavigator { readonly attribute MediaCapabilities mediaCapabilities; }; -// https://wicg.github.io/web-locks/#navigator-mixins +// https://w3c.github.io/web-locks/#navigator-mixins WorkerNavigator includes NavigatorLocks; // https://gpuweb.github.io/gpuweb/#navigator-gpu diff --git a/dom/workers/WorkerNavigator.cpp b/dom/workers/WorkerNavigator.cpp index 8f6e39f59dcb..ed6a09168182 100644 --- a/dom/workers/WorkerNavigator.cpp +++ b/dom/workers/WorkerNavigator.cpp @@ -284,7 +284,7 @@ dom::LockManager* WorkerNavigator::Locks() { nsIGlobalObject* global = workerPrivate->GlobalScope(); MOZ_ASSERT(global); - mLocks = new dom::LockManager(global); + mLocks = dom::LockManager::Create(*global); } return mLocks; }