From c7d6336a9d2f886471198acd7651f6efbeb7ff61 Mon Sep 17 00:00:00 2001 From: Andrew McCreight Date: Thu, 18 Jul 2019 15:02:57 +0000 Subject: [PATCH] Bug 1510760, part 4 - Add transplant support to GetRemoteOuterWindowProxy(). r=peterv When we call GetRemoteOuterWindowProxy in the middle of a transplant, the remote proxy that the function returns will be almost immediately swapped with some other object. Rather than trying to fix up the remote proxy map when that happens, this patch adds a new argument that is a pointer to the object, if any, that the remote proxy is going to be swapped to. This will be used in the remote proxy map. Having a value in the remote proxy map that is not a remote proxy could cause issues if somebody ends up calling GetRemoteOuterWindowProxy() a second time before the transplant has finished. To avoid that, my patch asserts that we are returning an object with the appropriate class. Differential Revision: https://phabricator.services.mozilla.com/D37598 --HG-- extra : moz-landing-system : lando --- docshell/base/BrowsingContext.cpp | 3 ++- docshell/base/BrowsingContext.h | 5 +++++ dom/base/RemoteOuterWindowProxy.cpp | 3 ++- dom/base/WindowProxyHolder.h | 1 + dom/bindings/RemoteObjectProxy.cpp | 20 ++++++++++++++++++-- dom/bindings/RemoteObjectProxy.h | 5 ++++- dom/bindings/ToJSValue.cpp | 3 ++- 7 files changed, 34 insertions(+), 6 deletions(-) diff --git a/docshell/base/BrowsingContext.cpp b/docshell/base/BrowsingContext.cpp index c03d53f7568e..227d9dac3275 100644 --- a/docshell/base/BrowsingContext.cpp +++ b/docshell/base/BrowsingContext.cpp @@ -673,7 +673,8 @@ void BrowsingContext::Location(JSContext* aCx, JS::MutableHandle aLocation, ErrorResult& aError) { aError.MightThrowJSException(); - sSingleton.GetProxyObject(aCx, &mLocation, aLocation); + sSingleton.GetProxyObject(aCx, &mLocation, /* aTransplantTo = */ nullptr, + aLocation); if (!aLocation) { aError.StealExceptionFromJSContext(aCx); } diff --git a/docshell/base/BrowsingContext.h b/docshell/base/BrowsingContext.h index dd248f442605..c91655093276 100644 --- a/docshell/base/BrowsingContext.h +++ b/docshell/base/BrowsingContext.h @@ -499,8 +499,13 @@ class BrowsingContext : public nsWrapperCache, public BrowsingContextBase { * lives in this process, and a same-process WindowProxy should be used (see * nsGlobalWindowOuter). This should only be called by bindings code, ToJSValue * is the right API to get a WindowProxy for a BrowsingContext. + * + * If aTransplantTo is non-null, then the WindowProxy object will eventually be + * transplanted onto it. Therefore it should be used as the value in the remote + * proxy map. */ extern bool GetRemoteOuterWindowProxy(JSContext* aCx, BrowsingContext* aContext, + JS::Handle aTransplantTo, JS::MutableHandle aRetVal); typedef BrowsingContext::Transaction BrowsingContextTransaction; diff --git a/dom/base/RemoteOuterWindowProxy.cpp b/dom/base/RemoteOuterWindowProxy.cpp index 4e6473d4431e..e2fd05d18083 100644 --- a/dom/base/RemoteOuterWindowProxy.cpp +++ b/dom/base/RemoteOuterWindowProxy.cpp @@ -62,11 +62,12 @@ const js::Class RemoteOuterWindowProxy::Base::sClass = PROXY_CLASS_DEF("Proxy", JSCLASS_HAS_RESERVED_SLOTS(2)); bool GetRemoteOuterWindowProxy(JSContext* aCx, BrowsingContext* aContext, + JS::Handle aTransplantTo, JS::MutableHandle aRetVal) { MOZ_ASSERT(!aContext->GetDocShell(), "Why are we creating a RemoteOuterWindowProxy?"); - sSingleton.GetProxyObject(aCx, aContext, aRetVal); + sSingleton.GetProxyObject(aCx, aContext, aTransplantTo, aRetVal); return !!aRetVal; } diff --git a/dom/base/WindowProxyHolder.h b/dom/base/WindowProxyHolder.h index 07efcc3434b0..f4d641e67041 100644 --- a/dom/base/WindowProxyHolder.h +++ b/dom/base/WindowProxyHolder.h @@ -70,6 +70,7 @@ inline void ImplCycleCollectionUnlink(WindowProxyHolder& aProxy) { } extern bool GetRemoteOuterWindowProxy(JSContext* aCx, BrowsingContext* aContext, + JS::Handle aTransplantTo, JS::MutableHandle aValue); } // namespace dom diff --git a/dom/bindings/RemoteObjectProxy.cpp b/dom/bindings/RemoteObjectProxy.cpp index 3812362b5a5b..9229fadfe9da 100644 --- a/dom/bindings/RemoteObjectProxy.cpp +++ b/dom/bindings/RemoteObjectProxy.cpp @@ -172,13 +172,23 @@ const char* RemoteObjectProxyBase::className( void RemoteObjectProxyBase::GetOrCreateProxyObject( JSContext* aCx, void* aNative, const js::Class* aClasp, - JS::MutableHandle aProxy, bool& aNewObjectCreated) const { + JS::Handle aTransplantTo, JS::MutableHandle aProxy, + bool& aNewObjectCreated) const { xpc::CompartmentPrivate* priv = xpc::CompartmentPrivate::Get(JS::CurrentGlobalOrNull(aCx)); xpc::CompartmentPrivate::RemoteProxyMap& map = priv->GetRemoteProxyMap(); auto result = map.lookupForAdd(aNative); if (result) { + MOZ_ASSERT(!aTransplantTo, + "No existing value allowed if we're doing a transplant"); + aProxy.set(result->value()); + + // During a transplant, we put an object that is temporarily not a + // proxy object into the map. Make sure that we don't return one of + // these objects in the middle of a transplant. + MOZ_RELEASE_ASSERT(js::GetObjectClass(aProxy) == aClasp); + return; } @@ -193,7 +203,13 @@ void RemoteObjectProxyBase::GetOrCreateProxyObject( aNewObjectCreated = true; - if (!map.add(result, aNative, obj)) { + // If we're transplanting onto an object, we want to make sure that it does + // not have the same class as aClasp to ensure that the release assert earlier + // in this function will actually fire if we try to return a proxy object in + // the middle of a transplant. + MOZ_ASSERT_IF(aTransplantTo, js::GetObjectClass(aTransplantTo) != aClasp); + + if (!map.add(result, aNative, aTransplantTo ? aTransplantTo : obj)) { return; } diff --git a/dom/bindings/RemoteObjectProxy.h b/dom/bindings/RemoteObjectProxy.h index ba0d94936eee..1256a3abbd8c 100644 --- a/dom/bindings/RemoteObjectProxy.h +++ b/dom/bindings/RemoteObjectProxy.h @@ -117,6 +117,7 @@ class RemoteObjectProxyBase : public js::BaseProxyHandler, */ void GetOrCreateProxyObject(JSContext* aCx, void* aNative, const js::Class* aClasp, + JS::Handle aTransplantTo, JS::MutableHandle aProxy, bool& aNewObjectCreated) const; @@ -149,9 +150,11 @@ class RemoteObjectProxy : public RemoteObjectProxyBase { } void GetProxyObject(JSContext* aCx, Native* aNative, + JS::Handle aTransplantTo, JS::MutableHandle aProxy) const { bool objectCreated = false; - GetOrCreateProxyObject(aCx, aNative, &sClass, aProxy, objectCreated); + GetOrCreateProxyObject(aCx, aNative, &sClass, aTransplantTo, aProxy, + objectCreated); if (objectCreated) { NS_ADDREF(aNative); } diff --git a/dom/bindings/ToJSValue.cpp b/dom/bindings/ToJSValue.cpp index 5773b262ad1f..01b478f41083 100644 --- a/dom/bindings/ToJSValue.cpp +++ b/dom/bindings/ToJSValue.cpp @@ -86,7 +86,8 @@ bool ToJSValue(JSContext* aCx, const WindowProxyHolder& aArgument, return ToJSValue(aCx, windowProxy, aValue); } - if (!GetRemoteOuterWindowProxy(aCx, bc, &windowProxy)) { + if (!GetRemoteOuterWindowProxy(aCx, bc, /* aTransplantTo = */ nullptr, + &windowProxy)) { return false; } aValue.setObjectOrNull(windowProxy);