Bug 1529684 - Part 6: Store a mIsInProcess flag to preserve WindowProxy behaviour, r=farre

Currently when we have an in-process WindowProxy object, we will attempt
to either use the cached mWindowProxy value, or fetch the
nsGlobalWindowOuter object from through the nsDocShell. Unfortunately,
when the BrowsingContext is detached, we will fail to get the
nsGlobalWindowOuter object. This happens to be OK for our test cases, as
the cached mWindowProxy value doesn't have the chance to go away, but
isn't acceptable long-term.

These patches exascerbated that issue by causing the nsDocShell pointer
itself to be cleared when it is destroyed, which caused the Remote
WindowProxy logic to be triggered. To deal with that case, this patch
adds a new mIsInProcess flag to continue to act like the old code-path.

In the future, we will need to also handle ensuring that the
nsGlobalWindowOuter lives for long enough, however that is not being
done in this patch in order to land it sooner rather than later.

Depends on D22763

Differential Revision: https://phabricator.services.mozilla.com/D22764
This commit is contained in:
Nika Layzell
2019-03-14 18:50:54 +00:00
parent a77dc03c08
commit 30c74d2ba9
3 changed files with 19 additions and 1 deletions

View File

@@ -161,6 +161,7 @@ BrowsingContext::BrowsingContext(BrowsingContext* aParent,
mType(aType), mType(aType),
mBrowsingContextId(aBrowsingContextId), mBrowsingContextId(aBrowsingContextId),
mParent(aParent), mParent(aParent),
mIsInProcess(false),
mOpener(aOpener), mOpener(aOpener),
mIsActivatedByUserGesture(false) { mIsActivatedByUserGesture(false) {
mCrossOriginPolicy = nsILoadInfo::CROSS_ORIGIN_POLICY_NULL; mCrossOriginPolicy = nsILoadInfo::CROSS_ORIGIN_POLICY_NULL;
@@ -184,6 +185,7 @@ void BrowsingContext::SetDocShell(nsIDocShell* aDocShell) {
// process to the parent & do other validation here. // process to the parent & do other validation here.
MOZ_RELEASE_ASSERT(nsDocShell::Cast(aDocShell)->GetBrowsingContext() == this); MOZ_RELEASE_ASSERT(nsDocShell::Cast(aDocShell)->GetBrowsingContext() == this);
mDocShell = aDocShell; mDocShell = aDocShell;
mIsInProcess = true;
} }
void BrowsingContext::Attach(bool aFromIPC) { void BrowsingContext::Attach(bool aFromIPC) {
@@ -241,6 +243,11 @@ void BrowsingContext::Detach(bool aFromIPC) {
// BrowsingContext - clear our now-dead nsDocShell reference. // BrowsingContext - clear our now-dead nsDocShell reference.
mDocShell = nullptr; mDocShell = nullptr;
// As our nsDocShell is going away, this should implicitly mark us as closed.
// We directly set our member, rather than using a transaction as we're going
// to send a `Detach` message to other processes either way.
mClosed = true;
if (!aFromIPC && XRE_IsContentProcess()) { if (!aFromIPC && XRE_IsContentProcess()) {
auto cc = ContentChild::GetSingleton(); auto cc = ContentChild::GetSingleton();
MOZ_DIAGNOSTIC_ASSERT(cc); MOZ_DIAGNOSTIC_ASSERT(cc);

View File

@@ -165,6 +165,11 @@ class BrowsingContext : public nsWrapperCache,
// Cast this object to a canonical browsing context, and return it. // Cast this object to a canonical browsing context, and return it.
CanonicalBrowsingContext* Canonical(); CanonicalBrowsingContext* Canonical();
// Is the most recent Document in this BrowsingContext loaded within this
// process? This may be true with a null mDocShell after the Window has been
// closed.
bool IsInProcess() const { return mIsInProcess; }
// Get the DocShell for this BrowsingContext if it is in-process, or // Get the DocShell for this BrowsingContext if it is in-process, or
// null if it's not. // null if it's not.
nsIDocShell* GetDocShell() { return mDocShell; } nsIDocShell* GetDocShell() { return mDocShell; }
@@ -374,6 +379,7 @@ class BrowsingContext : public nsWrapperCache,
Children mChildren; Children mChildren;
WeakPtr<BrowsingContext> mOpener; WeakPtr<BrowsingContext> mOpener;
nsCOMPtr<nsIDocShell> mDocShell; nsCOMPtr<nsIDocShell> mDocShell;
// This is not a strong reference, but using a JS::Heap for that should be // This is not a strong reference, but using a JS::Heap for that should be
// fine. The JSObject stored in here should be a proxy with a // fine. The JSObject stored in here should be a proxy with a
// nsOuterWindowProxy handler, which will update the pointer from its // nsOuterWindowProxy handler, which will update the pointer from its
@@ -384,6 +390,11 @@ class BrowsingContext : public nsWrapperCache,
// This flag is only valid in the top level browsing context, it indicates // This flag is only valid in the top level browsing context, it indicates
// whether the corresponding document has been activated by user gesture. // whether the corresponding document has been activated by user gesture.
bool mIsActivatedByUserGesture; bool mIsActivatedByUserGesture;
// Is the most recent Document in this BrowsingContext loaded within this
// process? This may be true with a null mDocShell after the Window has been
// closed.
bool mIsInProcess : 1;
}; };
/** /**

View File

@@ -69,7 +69,7 @@ bool ToJSValue(JSContext* aCx, const WindowProxyHolder& aArgument,
return true; return true;
} }
JS::Rooted<JSObject*> windowProxy(aCx); JS::Rooted<JSObject*> windowProxy(aCx);
if (bc->GetDocShell()) { if (bc->IsInProcess()) {
windowProxy = bc->GetWindowProxy(); windowProxy = bc->GetWindowProxy();
if (!windowProxy) { if (!windowProxy) {
nsPIDOMWindowOuter* window = bc->GetDOMWindow(); nsPIDOMWindowOuter* window = bc->GetDOMWindow();