Backed out 2 changesets (bug 1471496) for causing CycleCollectedJSRuntime.cpp perma failures CLOSED TREE
authorCiure Andrei <aciure@mozilla.com>
Sat, 02 Feb 2019 20:44:08 +0200
changeset 456560 63348118ef1d564a659f793c0ec9afe5d7f1cc8b
parent 456559 0883d6f9b77a71662a16614b1265409ea57e2108
child 456561 bc6d44d38030f2411519d96ecb5c3d2a2c9548a3
child 456563 da2e173c2991062bf80488c2c40118b8e3831041
push id111657
push userrmaries@mozilla.com
push dateSat, 02 Feb 2019 21:41:09 +0000
treeherdermozilla-inbound@63348118ef1d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1471496
milestone67.0a1
backs out9658187a54fbd9e7c5bda51fb4e5a041a7af77be
2ff333373fe4cc5cdd2a05ff4686f1640be5b280
first release with
nightly linux32
63348118ef1d / 67.0a1 / 20190202213603 / files
nightly linux64
63348118ef1d / 67.0a1 / 20190202213603 / files
nightly mac
63348118ef1d / 67.0a1 / 20190202213603 / files
nightly win32
63348118ef1d / 67.0a1 / 20190202213603 / files
nightly win64
63348118ef1d / 67.0a1 / 20190202213603 / files
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
releases
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Backed out 2 changesets (bug 1471496) for causing CycleCollectedJSRuntime.cpp perma failures CLOSED TREE Backed out changeset 9658187a54fb (bug 1471496) Backed out changeset 2ff333373fe4 (bug 1471496)
dom/base/MaybeCrossOriginObject.cpp
js/xpconnect/wrappers/AccessCheck.cpp
js/xpconnect/wrappers/AccessCheck.h
js/xpconnect/wrappers/WrapperFactory.cpp
--- a/dom/base/MaybeCrossOriginObject.cpp
+++ b/dom/base/MaybeCrossOriginObject.cpp
@@ -36,48 +36,33 @@ bool MaybeCrossOriginObjectMixins::IsPla
   MOZ_ASSERT(js::GetNonCCWObjectRealm(obj) ==
                  // "true" for second arg means to unwrap WindowProxy to
                  // get at the Window.
                  js::GetNonCCWObjectRealm(js::UncheckedUnwrap(obj, true)),
              "WindowProxy not same-Realm as Window?");
 
   BasePrincipal* subjectPrincipal =
       BasePrincipal::Cast(nsContentUtils::SubjectPrincipal(cx));
-  BasePrincipal* objectPrincipal =
-      BasePrincipal::Cast(nsContentUtils::ObjectPrincipal(obj));
+  nsIPrincipal* objectPrincipal = nsContentUtils::ObjectPrincipal(obj);
 
   // The spec effectively has an EqualsConsideringDomain check here,
   // because the spec has no concept of asymmetric security
   // relationships.  But we shouldn't ever end up here in the
   // asymmetric case anyway: That case should end up with Xrays, which
   // don't call into this code.
   //
   // Let's assert that EqualsConsideringDomain and
   // SubsumesConsideringDomain give the same results and use
   // EqualsConsideringDomain for the check we actually do, since it's
   // stricter and more closely matches the spec.
-  //
-  // That said, if the (not very well named)
-  // OriginAttributes::IsRestrictOpenerAccessForFPI() method returns
-  // false, we want to use FastSubsumesConsideringDomainIgnoringFPD
-  // instead of FastEqualsConsideringDomain, because in that case we
-  // still want to treat things which are in different first-party
-  // contexts as same-origin.
   MOZ_ASSERT(
       subjectPrincipal->FastEqualsConsideringDomain(objectPrincipal) ==
           subjectPrincipal->FastSubsumesConsideringDomain(objectPrincipal),
       "Why are we in an asymmetric case here?");
-  if (OriginAttributes::IsRestrictOpenerAccessForFPI()) {
-    return subjectPrincipal->FastEqualsConsideringDomain(objectPrincipal);
-  }
-
-  return subjectPrincipal->FastSubsumesConsideringDomainIgnoringFPD(
-             objectPrincipal) &&
-         objectPrincipal->FastSubsumesConsideringDomainIgnoringFPD(
-             subjectPrincipal);
+  return subjectPrincipal->FastEqualsConsideringDomain(objectPrincipal);
 }
 
 bool MaybeCrossOriginObjectMixins::CrossOriginGetOwnPropertyHelper(
     JSContext* cx, JS::Handle<JSObject*> obj, JS::Handle<jsid> id,
     JS::MutableHandle<JS::PropertyDescriptor> desc) const {
   MOZ_ASSERT(!IsPlatformObjectSameOrigin(cx, obj) || IsRemoteObjectProxy(obj),
              "Why did we get called?");
   // First check for an IDL-defined cross-origin property with the given name.
--- a/js/xpconnect/wrappers/AccessCheck.cpp
+++ b/js/xpconnect/wrappers/AccessCheck.cpp
@@ -84,22 +84,28 @@ bool AccessCheck::wrapperSubsumes(JSObje
 bool AccessCheck::isChrome(JS::Compartment* compartment) {
   return js::IsSystemCompartment(compartment);
 }
 
 bool AccessCheck::isChrome(JSObject* obj) {
   return isChrome(js::GetObjectCompartment(obj));
 }
 
-bool IsCrossOriginAccessibleObject(JSObject* obj) {
+CrossOriginObjectType IdentifyCrossOriginObject(JSObject* obj) {
   obj = js::UncheckedUnwrap(obj, /* stopAtWindowProxy = */ false);
   const js::Class* clasp = js::GetObjectClass(obj);
 
-  return (clasp->name[0] == 'L' && !strcmp(clasp->name, "Location")) ||
-         (clasp->name[0] == 'W' && !strcmp(clasp->name, "Window"));
+  if (clasp->name[0] == 'L' && !strcmp(clasp->name, "Location")) {
+    return CrossOriginLocation;
+  }
+  if (clasp->name[0] == 'W' && !strcmp(clasp->name, "Window")) {
+    return CrossOriginWindow;
+  }
+
+  return CrossOriginOpaque;
 }
 
 bool AccessCheck::checkPassToPrivilegedCode(JSContext* cx, HandleObject wrapper,
                                             HandleValue v) {
   // Primitives are fine.
   if (!v.isObject()) {
     return true;
   }
--- a/js/xpconnect/wrappers/AccessCheck.h
+++ b/js/xpconnect/wrappers/AccessCheck.h
@@ -30,23 +30,22 @@ class AccessCheck {
                                         const JS::CallArgs& args);
   // Called to report the correct sort of exception when our policy denies and
   // should throw.  The accessType argument should be one of "access",
   // "define", "delete", depending on which operation is being denied.
   static void reportCrossOriginDenial(JSContext* cx, JS::HandleId id,
                                       const nsACString& accessType);
 };
 
-/**
- * Returns true if the given object (which is expected to be stripped of
- * cross-compartment wrappers in practice, but this function doesn't assume
- * that) is a WindowProxy or Location object, which need special wrapping
- * behavior due to being usable cross-origin in limited ways.
- */
-bool IsCrossOriginAccessibleObject(JSObject* obj);
+enum CrossOriginObjectType {
+  CrossOriginWindow,
+  CrossOriginLocation,
+  CrossOriginOpaque
+};
+CrossOriginObjectType IdentifyCrossOriginObject(JSObject* obj);
 
 struct Policy {
   static bool checkCall(JSContext* cx, JS::HandleObject wrapper,
                         const JS::CallArgs& args) {
     MOZ_CRASH("As a rule, filtering wrappers are non-callable");
   }
 };
 
--- a/js/xpconnect/wrappers/WrapperFactory.cpp
+++ b/js/xpconnect/wrappers/WrapperFactory.cpp
@@ -334,45 +334,32 @@ static void DEBUG_CheckUnwrapSafety(Hand
                                     JS::Compartment* target) {
   if (!js::AllowNewWrapper(target, obj)) {
     // The JS engine should have returned a dead wrapper in this case and we
     // shouldn't even get here.
     MOZ_ASSERT_UNREACHABLE("CheckUnwrapSafety called for a dead wrapper");
   } else if (AccessCheck::isChrome(target) ||
              xpc::IsUniversalXPConnectEnabled(target)) {
     // If the caller is chrome (or effectively so), unwrap should always be
-    // allowed, but we might have a CrossOriginObjectWrapper here which allows
-    // it dynamically.
-    MOZ_ASSERT(!handler->hasSecurityPolicy() ||
-               handler == &CrossOriginObjectWrapper::singleton);
+    // allowed.
+    MOZ_ASSERT(!handler->hasSecurityPolicy());
   } else if (RealmPrivate::Get(origin)->forcePermissiveCOWs) {
     // Similarly, if this is a privileged scope that has opted to make itself
     // accessible to the world (allowed only during automation), unwrap should
-    // be allowed.  Again, it might be allowed dynamically.
-    MOZ_ASSERT(!handler->hasSecurityPolicy() ||
-               handler == &CrossOriginObjectWrapper::singleton);
+    // be allowed.
+    MOZ_ASSERT(!handler->hasSecurityPolicy());
   } else {
     // Otherwise, it should depend on whether the target subsumes the origin.
     JS::Compartment* originComp = JS::GetCompartmentForRealm(origin);
     bool subsumes =
         (OriginAttributes::IsRestrictOpenerAccessForFPI()
              ? AccessCheck::subsumesConsideringDomain(target, originComp)
              : AccessCheck::subsumesConsideringDomainIgnoringFPD(target,
                                                                  originComp));
-    if (!subsumes) {
-      // If the target (which is where the wrapper lives) does not subsume the
-      // origin (which is where the wrapped object lives), then we should have a
-      // security check on the wrapper here.
-      MOZ_ASSERT(handler->hasSecurityPolicy());
-    } else {
-      // Even if target subsumes origin, we might have a wrapper with a security
-      // policy here, if it happens to be a CrossOriginObjectWrapper.
-      MOZ_ASSERT(!handler->hasSecurityPolicy() ||
-                 handler == &CrossOriginObjectWrapper::singleton);
-    }
+    MOZ_ASSERT(handler->hasSecurityPolicy() == !subsumes);
   }
 }
 #else
 #  define DEBUG_CheckUnwrapSafety(obj, handler, origin, target) \
     {}
 #endif
 
 const CrossOriginObjectWrapper CrossOriginObjectWrapper::singleton;
@@ -412,16 +399,22 @@ static const Wrapper* SelectWrapper(bool
       return &PermissiveXrayDOM::singleton;
     } else if (xrayType == XrayForJSObject) {
       return &PermissiveXrayJS::singleton;
     }
     MOZ_ASSERT(xrayType == XrayForOpaqueObject);
     return &PermissiveXrayOpaque::singleton;
   }
 
+  // This is a security wrapper. Use the security versions and filter.
+  if (xrayType == XrayForDOMObject &&
+      IdentifyCrossOriginObject(obj) != CrossOriginOpaque) {
+    return &CrossOriginObjectWrapper::singleton;
+  }
+
   // There's never any reason to expose other objects to non-subsuming actors.
   // Just use an opaque wrapper in these cases.
   //
   // In general, we don't want opaque function wrappers to be callable.
   // But in the case of XBL, we rely on content being able to invoke
   // functions exposed from the XBL scope. We could remove this exception,
   // if needed, by using ExportFunction to generate the content-side
   // representations of XBL methods.
@@ -503,28 +496,16 @@ JSObject* WrapperFactory::Rewrap(JSConte
 
     // Otherwise we get an opaque wrapper.
     else {
       wrapper =
           &FilteringWrapper<CrossCompartmentSecurityWrapper, Opaque>::singleton;
     }
   }
 
-  // Special handling for the web's cross-origin objects (WindowProxy and
-  // Location).  We only need or want to do this in web-like contexts, where all
-  // security relationships are symmetric and there are no forced Xrays.
-  else if (originSubsumesTarget == targetSubsumesOrigin &&
-           // Check for the more rare case of cross-origin objects before doing
-           // the more-likely-to-pass checks for wantXrays.
-           IsCrossOriginAccessibleObject(obj) &&
-           (!targetSubsumesOrigin || (!originCompartmentPrivate->wantXrays &&
-                                      !targetCompartmentPrivate->wantXrays))) {
-    wrapper = &CrossOriginObjectWrapper::singleton;
-  }
-
   //
   // Now, handle the regular cases.
   //
   // These are wrappers we can compute using a rule-based approach. In order
   // to do so, we need to compute some parameters.
   //
   else {
     // The wrapper is a security wrapper (protecting the wrappee) if and