From 43ffe0d010c578eb4bc77216b593e336bfbe5e5d Mon Sep 17 00:00:00 2001 From: Cosmin Sabou Date: Fri, 16 Jun 2023 01:22:54 +0300 Subject: [PATCH] Backed out 2 changesets (bug 1798493) for causing wpt failures on partitioned-web-locks.tentative.https.html CLOSED TREE Backed out changeset c8c8ab2aa130 (bug 1798493) Backed out changeset f65876226f15 (bug 1798493) --- caps/ContentPrincipalInfoHashKey.h | 56 +++++++++++++++++++ caps/PrincipalHashKey.h | 4 +- caps/moz.build | 1 + dom/clients/manager/ClientInfo.cpp | 10 ++-- dom/clients/manager/ClientInfo.h | 4 +- dom/locks/LockManager.cpp | 18 +++--- dom/locks/LockManagerParent.cpp | 22 +++++--- dom/locks/LockManagerParent.h | 5 +- ipc/glue/BackgroundParentImpl.cpp | 6 +- ipc/glue/BackgroundParentImpl.h | 2 +- ipc/glue/PBackground.ipdl | 2 +- ...itioned-web-locks.tentative.https.html.ini | 11 +++- ...partitioned-web-locks.tentative.https.html | 36 ++++++------ .../antitracking/test/browser/browser.ini | 3 +- .../browser/browser_partitionedLockManager.js | 24 -------- 15 files changed, 121 insertions(+), 83 deletions(-) create mode 100644 caps/ContentPrincipalInfoHashKey.h delete mode 100644 toolkit/components/antitracking/test/browser/browser_partitionedLockManager.js diff --git a/caps/ContentPrincipalInfoHashKey.h b/caps/ContentPrincipalInfoHashKey.h new file mode 100644 index 000000000000..ed1f79852f15 --- /dev/null +++ b/caps/ContentPrincipalInfoHashKey.h @@ -0,0 +1,56 @@ +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ +/* vim: set ts=8 sts=2 et sw=2 tw=80: */ +/* 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/. */ + +#ifndef CAPS_PRINCIPALHASHKEY_H_ +#define CAPS_PRINCIPALHASHKEY_H_ + +#include "mozilla/ipc/PBackgroundSharedTypes.h" +#include "PLDHashTable.h" +#include "nsHashKeys.h" +#include "nsUnicharUtils.h" + +namespace mozilla { + +class ContentPrincipalInfoHashKey : public PLDHashEntryHdr { + public: + using KeyType = const ipc::ContentPrincipalInfo&; + using KeyTypePointer = const ipc::ContentPrincipalInfo*; + + explicit ContentPrincipalInfoHashKey(KeyTypePointer aKey) + : mPrincipalInfo(*aKey) { + MOZ_COUNT_CTOR(ContentPrincipalInfoHashKey); + } + ContentPrincipalInfoHashKey(ContentPrincipalInfoHashKey&& aOther) noexcept + : mPrincipalInfo(aOther.mPrincipalInfo) { + MOZ_COUNT_CTOR(ContentPrincipalInfoHashKey); + } + + MOZ_COUNTED_DTOR(ContentPrincipalInfoHashKey) + + KeyType GetKey() const { return mPrincipalInfo; } + + bool KeyEquals(KeyTypePointer aKey) const { + // Mocks BasePrincipal::FastEquals() + return mPrincipalInfo.originNoSuffix() == aKey->originNoSuffix() && + mPrincipalInfo.attrs() == aKey->attrs(); + } + + static KeyTypePointer KeyToPointer(KeyType aKey) { return &aKey; } + static PLDHashNumber HashKey(KeyTypePointer aKey) { + nsAutoCString suffix; + aKey->attrs().CreateSuffix(suffix); + return HashGeneric(HashString(aKey->originNoSuffix()), HashString(suffix)); + } + + enum { ALLOW_MEMMOVE = true }; + + protected: + const ipc::ContentPrincipalInfo mPrincipalInfo; +}; + +} // namespace mozilla + +#endif // CAPS_PRINCIPALHASHKEY_H_ diff --git a/caps/PrincipalHashKey.h b/caps/PrincipalHashKey.h index b5167aa11b98..1c245532a607 100644 --- a/caps/PrincipalHashKey.h +++ b/caps/PrincipalHashKey.h @@ -25,7 +25,7 @@ class PrincipalHashKey : public PLDHashEntryHdr { MOZ_ASSERT(aKey); MOZ_COUNT_CTOR(PrincipalHashKey); } - PrincipalHashKey(PrincipalHashKey&& aKey) noexcept + PrincipalHashKey(PrincipalHashKey&& aKey) : mPrincipal(std::move(aKey.mPrincipal)) { MOZ_COUNT_CTOR(PrincipalHashKey); } @@ -43,7 +43,7 @@ class PrincipalHashKey : public PLDHashEntryHdr { return aKey; } static PLDHashNumber HashKey(const nsIPrincipal* aKey) { - const auto* bp = BasePrincipal::Cast(aKey); + auto* bp = BasePrincipal::Cast(aKey); return HashGeneric(bp->GetOriginNoSuffixHash(), bp->GetOriginSuffixHash()); } diff --git a/caps/moz.build b/caps/moz.build index 7fbc71f16923..6d5d95f9345f 100644 --- a/caps/moz.build +++ b/caps/moz.build @@ -31,6 +31,7 @@ EXPORTS += [ EXPORTS.mozilla = [ "BasePrincipal.h", "ContentPrincipal.h", + "ContentPrincipalInfoHashKey.h", "ExpandedPrincipal.h", "NullPrincipal.h", "OriginAttributes.h", diff --git a/dom/clients/manager/ClientInfo.cpp b/dom/clients/manager/ClientInfo.cpp index 6fdcbcce2cef..a87d337f1f6d 100644 --- a/dom/clients/manager/ClientInfo.cpp +++ b/dom/clients/manager/ClientInfo.cpp @@ -34,10 +34,9 @@ ClientInfo& ClientInfo::operator=(const ClientInfo& aRight) { return *this; } -ClientInfo::ClientInfo(ClientInfo&& aRight) noexcept - : mData(std::move(aRight.mData)) {} +ClientInfo::ClientInfo(ClientInfo&& aRight) : mData(std::move(aRight.mData)) {} -ClientInfo& ClientInfo::operator=(ClientInfo&& aRight) noexcept { +ClientInfo& ClientInfo::operator=(ClientInfo&& aRight) { mData.reset(); mData = std::move(aRight.mData); return *this; @@ -90,14 +89,14 @@ const IPCClientInfo& ClientInfo::ToIPC() const { return *mData; } bool ClientInfo::IsPrivateBrowsing() const { switch (PrincipalInfo().type()) { case PrincipalInfo::TContentPrincipalInfo: { - const auto& p = PrincipalInfo().get_ContentPrincipalInfo(); + auto& p = PrincipalInfo().get_ContentPrincipalInfo(); return p.attrs().mPrivateBrowsingId != 0; } case PrincipalInfo::TSystemPrincipalInfo: { return false; } case PrincipalInfo::TNullPrincipalInfo: { - const auto& p = PrincipalInfo().get_NullPrincipalInfo(); + auto& p = PrincipalInfo().get_NullPrincipalInfo(); return p.attrs().mPrivateBrowsingId != 0; } default: { @@ -108,6 +107,7 @@ bool ClientInfo::IsPrivateBrowsing() const { } Result, nsresult> ClientInfo::GetPrincipal() const { + MOZ_ASSERT(NS_IsMainThread()); return PrincipalInfoToPrincipal(PrincipalInfo()); } diff --git a/dom/clients/manager/ClientInfo.h b/dom/clients/manager/ClientInfo.h index 0c0ca859e5b5..fd5f874b09f5 100644 --- a/dom/clients/manager/ClientInfo.h +++ b/dom/clients/manager/ClientInfo.h @@ -45,9 +45,9 @@ class ClientInfo final { ClientInfo& operator=(const ClientInfo& aRight); - ClientInfo(ClientInfo&& aRight) noexcept; + ClientInfo(ClientInfo&& aRight); - ClientInfo& operator=(ClientInfo&& aRight) noexcept; + ClientInfo& operator=(ClientInfo&& aRight); explicit ClientInfo(const IPCClientInfo& aData); diff --git a/dom/locks/LockManager.cpp b/dom/locks/LockManager.cpp index 4d86118de066..c8beff7d56d1 100644 --- a/dom/locks/LockManager.cpp +++ b/dom/locks/LockManager.cpp @@ -39,16 +39,18 @@ LockManager::LockManager(nsIGlobalObject* aGlobal) : mOwner(aGlobal) { return; } - nsCOMPtr principal = - clientInfo->GetPrincipal().unwrapOr(nullptr); - if (!principal || !principal->GetIsContentPrincipal()) { + const mozilla::ipc::PrincipalInfo& principalInfo = + clientInfo->PrincipalInfo(); + + if (principalInfo.type() != + mozilla::ipc::PrincipalInfo::TContentPrincipalInfo) { return; } mozilla::ipc::PBackgroundChild* backgroundActor = mozilla::ipc::BackgroundChild::GetOrCreateForCurrentThread(); mActor = new locks::LockManagerChild(aGlobal); - backgroundActor->SendPLockManagerConstructor(mActor, WrapNotNull(principal), + backgroundActor->SendPLockManagerConstructor(mActor, principalInfo, clientInfo->Id()); if (!NS_IsMainThread()) { @@ -131,13 +133,7 @@ already_AddRefed LockManager::Request(const nsAString& aName, return nullptr; } - const StorageAccess access = mOwner->GetStorageAccess(); - bool allowed = - access > StorageAccess::eDeny || - (StaticPrefs:: - privacy_partition_always_partition_third_party_non_cookie_storage() && - ShouldPartitionStorage(access)); - if (!allowed) { + if (mOwner->GetStorageAccess() <= StorageAccess::eDeny) { // 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 diff --git a/dom/locks/LockManagerParent.cpp b/dom/locks/LockManagerParent.cpp index 4af7fe212914..9083a7fc6c10 100644 --- a/dom/locks/LockManagerParent.cpp +++ b/dom/locks/LockManagerParent.cpp @@ -7,6 +7,7 @@ #include "LockManagerParent.h" #include "LockRequestParent.h" +#include "mozilla/ContentPrincipalInfoHashKey.h" #include "mozilla/PrincipalHashKey.h" #include "mozilla/RefPtr.h" #include "mozilla/StaticPtr.h" @@ -17,24 +18,27 @@ namespace mozilla::dom::locks { -static StaticAutoPtr>> +static StaticAutoPtr< + nsTHashMap>> sManagedLocksMap; using IPCResult = mozilla::ipc::IPCResult; -LockManagerParent::LockManagerParent(NotNull aPrincipal, - const nsID& aClientId) - : mClientId(NSID_TrimBracketsUTF16(aClientId)), mPrincipal(aPrincipal) { +LockManagerParent::LockManagerParent( + const mozilla::ipc::ContentPrincipalInfo& aPrincipalInfo, + const nsID& aClientId) + : mClientId(NSID_TrimBracketsUTF16(aClientId)), + mPrincipalInfo(aPrincipalInfo) { if (!sManagedLocksMap) { sManagedLocksMap = - new nsTHashMap>(); + new nsTHashMap>(); } else { - mManagedLocks = sManagedLocksMap->Get(aPrincipal); + mManagedLocks = sManagedLocksMap->Get(aPrincipalInfo); } if (!mManagedLocks) { mManagedLocks = new ManagedLocks(); - sManagedLocksMap->LookupOrInsert(aPrincipal, mManagedLocks); + sManagedLocksMap->LookupOrInsert(aPrincipalInfo, mManagedLocks); } } @@ -75,8 +79,8 @@ void LockManagerParent::ActorDestroy(ActorDestroyReason aWhy) { mManagedLocks = nullptr; // We just decreased the refcount and potentially deleted it, so check whether // the weak pointer still points to anything and remove the entry if not. - if (!sManagedLocksMap->Get(mPrincipal)) { - sManagedLocksMap->Remove(mPrincipal); + if (!sManagedLocksMap->Get(mPrincipalInfo)) { + sManagedLocksMap->Remove(mPrincipalInfo); } } diff --git a/dom/locks/LockManagerParent.h b/dom/locks/LockManagerParent.h index 0e0f4affb2e3..fef40d66a461 100644 --- a/dom/locks/LockManagerParent.h +++ b/dom/locks/LockManagerParent.h @@ -32,7 +32,8 @@ class LockManagerParent final : public PLockManagerParent { public: NS_INLINE_DECL_REFCOUNTING(LockManagerParent) - LockManagerParent(NotNull aPrincipal, const nsID& aClientId); + LockManagerParent(const mozilla::ipc::ContentPrincipalInfo& aPrincipalInfo, + const nsID& aClientId); void ProcessRequestQueue(nsTArray>& aQueue); bool IsGrantableRequest(const IPCLockRequest& aRequest); @@ -53,7 +54,7 @@ class LockManagerParent final : public PLockManagerParent { RefPtr mManagedLocks; nsString mClientId; - NotNull> mPrincipal; + mozilla::ipc::ContentPrincipalInfo mPrincipalInfo; }; } // namespace mozilla::dom::locks diff --git a/ipc/glue/BackgroundParentImpl.cpp b/ipc/glue/BackgroundParentImpl.cpp index df1c0f122236..5cb45b0a2cce 100644 --- a/ipc/glue/BackgroundParentImpl.cpp +++ b/ipc/glue/BackgroundParentImpl.cpp @@ -1474,9 +1474,9 @@ bool BackgroundParentImpl::DeallocPMediaTransportParent( } already_AddRefed -BackgroundParentImpl::AllocPLockManagerParent(NotNull aPrincipal, - const nsID& aClientId) { - return MakeAndAddRef(aPrincipal, +BackgroundParentImpl::AllocPLockManagerParent( + const ContentPrincipalInfo& aPrincipalInfo, const nsID& aClientId) { + return MakeAndAddRef(aPrincipalInfo, aClientId); } diff --git a/ipc/glue/BackgroundParentImpl.h b/ipc/glue/BackgroundParentImpl.h index 3fc8908d0a84..58693f9e8c72 100644 --- a/ipc/glue/BackgroundParentImpl.h +++ b/ipc/glue/BackgroundParentImpl.h @@ -408,7 +408,7 @@ class BackgroundParentImpl : public PBackgroundParent { PWebSocketConnectionParent* actor, const uint32_t& aListenerId) override; already_AddRefed AllocPLockManagerParent( - NotNull aPrincipal, const nsID& aClientId) final; + const ContentPrincipalInfo& aPrincipalInfo, const nsID& aClientId) final; already_AddRefed AllocPFetchParent() override; }; diff --git a/ipc/glue/PBackground.ipdl b/ipc/glue/PBackground.ipdl index 8ecc230c111f..d9e40ff6d5c6 100644 --- a/ipc/glue/PBackground.ipdl +++ b/ipc/glue/PBackground.ipdl @@ -313,7 +313,7 @@ parent: async PWebSocketConnection(uint32_t aListenerId); - async PLockManager(nsIPrincipal aPrincipalInfo, nsID aClientId); + async PLockManager(ContentPrincipalInfo aPrincipalInfo, nsID aClientId); async PIPCClientCerts(); diff --git a/testing/web-platform/meta/web-locks/partitioned-web-locks.tentative.https.html.ini b/testing/web-platform/meta/web-locks/partitioned-web-locks.tentative.https.html.ini index d40f68656cfe..1579a8db72ef 100644 --- a/testing/web-platform/meta/web-locks/partitioned-web-locks.tentative.https.html.ini +++ b/testing/web-platform/meta/web-locks/partitioned-web-locks.tentative.https.html.ini @@ -1,6 +1,11 @@ -prefs: [privacy.partition.always_partition_third_party_non_cookie_storage:true] - [partitioned-web-locks.tentative.https.html] + expected: + if os == "android": ERROR + TIMEOUT + [WebLocks of an iframe under a 3rd-party site are partitioned] + expected: + if os == "android": FAIL + TIMEOUT [WebLocks of a nested iframe with a cross-site ancestor are partitioned] - expected: FAIL + expected: NOTRUN diff --git a/testing/web-platform/tests/web-locks/partitioned-web-locks.tentative.https.html b/testing/web-platform/tests/web-locks/partitioned-web-locks.tentative.https.html index 680474dea983..2f166e67bc13 100644 --- a/testing/web-platform/tests/web-locks/partitioned-web-locks.tentative.https.html +++ b/testing/web-platform/tests/web-locks/partitioned-web-locks.tentative.https.html @@ -29,10 +29,10 @@ let next_lock_id = 1; // back to the pop-up. // Step 6 (pop-up): intercept the result message from the iframe and // send it to the top-frame. -// Step 7 (top-frame): add cleanup hook. -// Step 8 (top-frame): ensure that the same-site iframe's web-lock +// Step 7 (top-frame): ensure that the same-site iframe's web-lock // request succeeds since it and the top-level site are successfully // partitioned and each can hold an exclusive lock. +// Step 8 (top-frame): clean up. async function third_party_test(t) { let target_url = HTTPS_ORIGIN + '/web-locks/resources/iframe.html'; @@ -59,19 +59,19 @@ async function third_party_test(t) { const result = await new Promise(resolve => window.onmessage = resolve); // Step 7. + // When 3rd party storage partitioning is enabled, the iframe should be able + // to aquire a lock with the same name as one exclusively held by the opener + // of its top window, even when that opener has the same origin. + assert_equals(result.data.failed, undefined, + 'The 1p iframe failed to acquire the lock'); + + // Step 8. t.add_cleanup(() => { w.close() for(let i = 1; i < next_lock_id; i++){ held.get(i)(); } }); - - // Step 8. - // When 3rd party storage partitioning is enabled, the iframe should be able - // to acquire a lock with the same name as one exclusively held by the opener - // of its top window, even when that opener has the same origin. - assert_equals(result.data.failed, undefined, - 'The 1p iframe failed to acquire the lock'); } promise_test(t => { @@ -101,10 +101,10 @@ promise_test(t => { // child iframe and send it to the pop-up. // Nested Step 9 (pop-up): intercept the result message from the parent // iframe and send it to the top frame. -// Nested Step 10 (top frame): add cleanup hook -// Nested Step 11 (top frame): ensure that the same-site iframe's web-lock +// Nested Step 10 (top frame): ensure that the same-site iframe's web-lock // request succeeds since it and the top-level are successfully // partitioned and each can hold an exclusive lock. +// Nested Step 11 (top frame): clean up. // Map of lock_id => function that releases a lock. const held_2 = new Map(); @@ -145,19 +145,19 @@ async function nested_iframe_test(t) { const result = await new Promise(resolve => window.onmessage = resolve); // Nested Step 10. + // With third-party storage partitioning enabled, the same-site iframe + // should be able to acquire the lock as it has a cross-site ancestor + // and is partitioned separately from the top-level site. + assert_equals(result.data.failed, undefined, + 'The 1p iframe failed to acquire the lock'); + + // Nested Step 11. t.add_cleanup(() => { w.close() for(let i = 1; i < next_lock_id_2; i++){ held_2.get(i)(); } }); - - // Nested Step 11. - // With third-party storage partitioning enabled, the same-site iframe - // should be able to acquire the lock as it has a cross-site ancestor - // and is partitioned separately from the top-level site. - assert_equals(result.data.failed, undefined, - 'The 1p iframe failed to acquire the lock'); } promise_test(t => { diff --git a/toolkit/components/antitracking/test/browser/browser.ini b/toolkit/components/antitracking/test/browser/browser.ini index c7d9bcbd74b9..68f561e664d2 100644 --- a/toolkit/components/antitracking/test/browser/browser.ini +++ b/toolkit/components/antitracking/test/browser/browser.ini @@ -142,7 +142,6 @@ skip-if = debug # Bug 1700551 [browser_partitionedLocalStorage.js] [browser_partitionedLocalStorage_events.js] support-files = localStorageEvents.html -[browser_partitionedLockManager.js] [browser_workerPropagation.js] support-files = workerIframe.html [browser_cookieBetweenTabs.js] @@ -206,7 +205,7 @@ support-files = browser_staticPartition_CORS_preflight.sjs [browser_staticPartition_HSTS.js] support-files = browser_staticPartition_HSTS.sjs [browser_staticPartition_saveAs.js] -skip-if = +skip-if = os == "linux" && bits == 64 # Bug 1775746 win10_2004 && debug # Bug 1775746 support-files = diff --git a/toolkit/components/antitracking/test/browser/browser_partitionedLockManager.js b/toolkit/components/antitracking/test/browser/browser_partitionedLockManager.js deleted file mode 100644 index df1106903b5a..000000000000 --- a/toolkit/components/antitracking/test/browser/browser_partitionedLockManager.js +++ /dev/null @@ -1,24 +0,0 @@ -/* import-globals-from partitionedstorage_head.js */ - -PartitionedStorageHelper.runTest( - "LockManager works in both first and third party contexts", - async (win3rdParty, win1stParty, allowed) => { - let locks = []; - ok(win1stParty.isSecureContext, "1st party is in a secure context"); - ok(win3rdParty.isSecureContext, "3rd party is in a secure context"); - await win1stParty.navigator.locks.request("foo", lock => { - locks.push(lock); - ok(true, "locks.request succeeded for 1st party"); - }); - - await win3rdParty.navigator.locks.request("foo", lock => { - locks.push(lock); - ok(true, "locks.request succeeded for 3rd party"); - }); - - is(locks.length, 2, "We should have granted 2 lock requests at this point"); - }, - /* cleanupFunction */ undefined, - /* extraPrefs */ undefined, - { runInSecureContext: true } -);