Bug 1022016 - Redesign nsDependentJSString API to be less of a footgun. r=gabor
authorBobby Holley <bobbyholley@gmail.com>
Tue, 10 Jun 2014 20:15:56 -0700
changeset 188058 c45ab5ebb3937adf38427bd8a3bd9854297df6d6
parent 188057 8860fd09236aaa3bbdf3b1b9ae90ad12b8e94543
child 188059 e54cc67cf015511fdcb0020f80728a1e8a3b587e
push id1
push userroot
push dateMon, 20 Oct 2014 17:29:22 +0000
reviewersgabor
bugs1022016
milestone33.0a1
Bug 1022016 - Redesign nsDependentJSString API to be less of a footgun. r=gabor
dom/base/Navigator.cpp
dom/base/WindowNamedPropertiesHandler.cpp
dom/base/nsDOMClassInfo.cpp
dom/base/nsGlobalWindow.cpp
dom/base/nsJSUtils.h
dom/xbl/nsXBLBinding.cpp
dom/xbl/nsXBLProtoImplField.cpp
js/xpconnect/wrappers/AccessCheck.cpp
js/xpconnect/wrappers/XrayWrapper.cpp
--- a/dom/base/Navigator.cpp
+++ b/dom/base/Navigator.cpp
@@ -1931,26 +1931,28 @@ Navigator::GetMozAudioChannelManager(Err
 }
 #endif
 
 bool
 Navigator::DoNewResolve(JSContext* aCx, JS::Handle<JSObject*> aObject,
                         JS::Handle<jsid> aId,
                         JS::MutableHandle<JSPropertyDescriptor> aDesc)
 {
+  // Note: The infallibleInit call below depends on this check.
   if (!JSID_IS_STRING(aId)) {
     return true;
   }
 
   nsScriptNameSpaceManager* nameSpaceManager = GetNameSpaceManager();
   if (!nameSpaceManager) {
     return Throw(aCx, NS_ERROR_NOT_INITIALIZED);
   }
 
-  nsDependentJSString name(aId);
+  nsDependentJSString name;
+  name.infallibleInit(aId);
 
   const nsGlobalNameStruct* name_struct =
     nameSpaceManager->LookupNavigatorName(name);
   if (!name_struct) {
     return true;
   }
 
   JS::Rooted<JSObject*> naviObj(aCx,
--- a/dom/base/WindowNamedPropertiesHandler.cpp
+++ b/dom/base/WindowNamedPropertiesHandler.cpp
@@ -83,27 +83,29 @@ GetWindowFromGlobal(JSObject* aGlobal)
 }
 
 bool
 WindowNamedPropertiesHandler::getOwnPropertyDescriptor(JSContext* aCx,
                                                        JS::Handle<JSObject*> aProxy,
                                                        JS::Handle<jsid> aId,
                                                        JS::MutableHandle<JSPropertyDescriptor> aDesc)
 {
+  // Note: The infallibleInit call below depends on this check.
   if (!JSID_IS_STRING(aId)) {
     // Nothing to do if we're resolving a non-string property.
     return true;
   }
 
   JS::Rooted<JSObject*> global(aCx, JS_GetGlobalForObject(aCx, aProxy));
   if (HasPropertyOnPrototype(aCx, aProxy, aId)) {
     return true;
   }
 
-  nsDependentJSString str(aId);
+  nsDependentJSString str;
+  str.infallibleInit(aId);
 
   // Grab the DOM window.
   nsGlobalWindow* win = GetWindowFromGlobal(global);
   if (win->Length() > 0) {
     nsCOMPtr<nsIDOMWindow> childWin = win->GetChildWindow(str);
     if (childWin && ShouldExposeChildWindow(str, childWin)) {
       // We found a subframe of the right name. Shadowing via |var foo| in
       // global scope is still allowed, since |var| only looks up |own|
--- a/dom/base/nsDOMClassInfo.cpp
+++ b/dom/base/nsDOMClassInfo.cpp
@@ -2656,17 +2656,20 @@ nsWindowSH::GlobalResolve(nsGlobalWindow
     FillPropertyDescriptor(desc, obj, JS::ObjectValue(*shim), /* readOnly = */ false);
     return NS_OK;
   }
 #endif
 
   nsScriptNameSpaceManager *nameSpaceManager = GetNameSpaceManager();
   NS_ENSURE_TRUE(nameSpaceManager, NS_ERROR_NOT_INITIALIZED);
 
-  nsDependentJSString name(id);
+  // Note - Our only caller is nsGlobalWindow::DoNewResolve, which checks that
+  // JSID_IS_STRING(id) is true.
+  nsDependentJSString name;
+  name.infallibleInit(id);
 
   const char16_t *class_name = nullptr;
   const nsGlobalNameStruct *name_struct =
     nameSpaceManager->LookupName(name, &class_name);
 
   if (!name_struct) {
     return NS_OK;
   }
--- a/dom/base/nsGlobalWindow.cpp
+++ b/dom/base/nsGlobalWindow.cpp
@@ -4265,16 +4265,17 @@ nsGlobalWindow::GetSupportedNames(nsTArr
 
 bool
 nsGlobalWindow::DoNewResolve(JSContext* aCx, JS::Handle<JSObject*> aObj,
                              JS::Handle<jsid> aId,
                              JS::MutableHandle<JSPropertyDescriptor> aDesc)
 {
   MOZ_ASSERT(IsInnerWindow());
 
+  // Note: The infallibleInit call in GlobalResolve depends on this check.
   if (!JSID_IS_STRING(aId)) {
     return true;
   }
 
   nsresult rv = nsWindowSH::GlobalResolve(this, aCx, aObj, aId, aDesc);
   if (NS_FAILED(rv)) {
     return Throw(aCx, rv);
   }
--- a/dom/base/nsJSUtils.h
+++ b/dom/base/nsJSUtils.h
@@ -136,67 +136,76 @@ public:
     }
   }
 };
 
 
 class nsDependentJSString : public nsDependentString
 {
 public:
-  /**
-   * In the case of string ids, getting the string's chars is infallible, so
-   * the dependent string can be constructed directly.
-   */
-  explicit nsDependentJSString(JS::Handle<jsid> id)
-    : nsDependentString(JS_GetInternedStringChars(JSID_TO_STRING(id)),
-                        JS_GetStringLength(JSID_TO_STRING(id)))
-  {
-  }
 
   /**
-   * Ditto for flat strings.
+   * nsDependentJSString should be default constructed, which leaves it empty
+   * (this->IsEmpty()), and initialized with one of the init() or infallibleInit()
+   * methods below.
    */
-  explicit nsDependentJSString(JSFlatString* fstr)
-    : nsDependentString(JS_GetFlatStringChars(fstr),
-                        JS_GetStringLength(JS_FORGET_STRING_FLATNESS(fstr)))
-  {
-  }
-
-  /**
-   * For all other strings, the nsDependentJSString object should be default
-   * constructed, which leaves it empty (this->IsEmpty()), and initialized with
-   * one of the init() methods below.
-   */
-
-  nsDependentJSString()
-  {
-  }
+  nsDependentJSString() {}
 
   bool init(JSContext* aContext, JSString* str)
   {
-      size_t length;
-      const jschar* chars = JS_GetStringCharsZAndLength(aContext, str, &length);
-      if (!chars)
-          return false;
+    size_t length;
+    const jschar* chars = JS_GetStringCharsZAndLength(aContext, str, &length);
+    if (!chars) {
+      return false;
+    }
 
-      NS_ASSERTION(IsEmpty(), "init() on initialized string");
-      nsDependentString* base = this;
-      new(base) nsDependentString(chars, length);
-      return true;
+    infallibleInit(chars, length);
+    return true;
   }
 
   bool init(JSContext* aContext, const JS::Value &v)
   {
+    if (v.isString()) {
       return init(aContext, v.toString());
+    }
+
+    // Stringify, making sure not to run script.
+    JS::Rooted<JSString*> str(aContext);
+    if (v.isObject()) {
+      str = JS_NewStringCopyZ(aContext, "[Object]");
+    } else {
+      JS::Rooted<JS::Value> rootedVal(aContext, v);
+      str = JS::ToString(aContext, rootedVal);
+    }
+
+    return str && init(aContext, str);
+  }
+
+  bool init(JSContext* aContext, jsid id)
+  {
+    JS::Rooted<JS::Value> v(aContext);
+    return JS_IdToValue(aContext, id, &v) && init(aContext, v);
   }
 
-  void init(JSFlatString* fstr)
+  void infallibleInit(const char16_t* aChars, size_t aLength)
   {
-      MOZ_ASSERT(IsEmpty(), "init() on initialized string");
-      new(this) nsDependentJSString(fstr);
+    MOZ_ASSERT(IsEmpty(), "init() on initialized string");
+    nsDependentString* base = this;
+    new (base) nsDependentString(aChars, aLength);
   }
 
-  ~nsDependentJSString()
+  // For use when JSID_IS_STRING(id) is known to be true.
+  void infallibleInit(jsid id)
   {
+    MOZ_ASSERT(JSID_IS_STRING(id));
+    infallibleInit(JS_GetInternedStringChars(JSID_TO_STRING(id)),
+                   JS_GetStringLength(JSID_TO_STRING(id)));
   }
+
+  void infallibleInit(JSFlatString* fstr)
+  {
+    infallibleInit(JS_GetFlatStringChars(fstr), JS_GetStringLength(JS_FORGET_STRING_FLATNESS(fstr)));
+  }
+
+  ~nsDependentJSString() {}
 };
 
 #endif /* nsJSUtils_h__ */
--- a/dom/xbl/nsXBLBinding.cpp
+++ b/dom/xbl/nsXBLBinding.cpp
@@ -1091,20 +1091,23 @@ bool
 nsXBLBinding::LookupMember(JSContext* aCx, JS::Handle<jsid> aId,
                            JS::MutableHandle<JSPropertyDescriptor> aDesc)
 {
   // We should never enter this function with a pre-filled property descriptor.
   MOZ_ASSERT(!aDesc.object());
 
   // Get the string as an nsString before doing anything, so we can make
   // convenient comparisons during our search.
+  //
+  // Note: the infallibleInit call below depends on this check.
   if (!JSID_IS_STRING(aId)) {
     return true;
   }
-  nsDependentJSString name(aId);
+  nsDependentJSString name;
+  name.infallibleInit(aId);
 
   // We have a weak reference to our bound element, so make sure it's alive.
   if (!mBoundElement || !mBoundElement->GetWrapper()) {
     return false;
   }
 
   // Get the scope of mBoundElement and the associated XBL scope. We should only
   // be calling into this machinery if we're running in a separate XBL scope.
--- a/dom/xbl/nsXBLProtoImplField.cpp
+++ b/dom/xbl/nsXBLProtoImplField.cpp
@@ -184,17 +184,17 @@ InstallXBLField(JSContext* cx,
   {
     JSAutoCompartment ac(cx, callee);
 
     JS::Rooted<JSObject*> xblProto(cx);
     xblProto = &js::GetFunctionNativeReserved(callee, XBLPROTO_SLOT).toObject();
 
     JS::Rooted<JS::Value> name(cx, js::GetFunctionNativeReserved(callee, FIELD_SLOT));
     JSFlatString* fieldStr = JS_ASSERT_STRING_IS_FLAT(name.toString());
-    fieldName.init(fieldStr);
+    fieldName.infallibleInit(fieldStr);
 
     MOZ_ALWAYS_TRUE(JS_ValueToId(cx, name, idp));
 
     // If a separate XBL scope is being used, the callee is not same-compartment
     // with the xbl prototype, and the object is a cross-compartment wrapper.
     xblProto = js::UncheckedUnwrap(xblProto);
     JSAutoCompartment ac2(cx, xblProto);
     JS::Value slotVal = ::JS_GetReservedSlot(xblProto, 0);
--- a/js/xpconnect/wrappers/AccessCheck.cpp
+++ b/js/xpconnect/wrappers/AccessCheck.cpp
@@ -143,17 +143,19 @@ IsFrameId(JSContext *cx, JSObject *objAr
     if (!col) {
         return false;
     }
 
     nsCOMPtr<nsIDOMWindow> domwin;
     if (JSID_IS_INT(id)) {
         col->Item(JSID_TO_INT(id), getter_AddRefs(domwin));
     } else if (JSID_IS_STRING(id)) {
-        col->NamedItem(nsDependentJSString(id), getter_AddRefs(domwin));
+        nsDependentJSString idAsString;
+        idAsString.infallibleInit(id);
+        col->NamedItem(idAsString, getter_AddRefs(domwin));
     }
 
     return domwin != nullptr;
 }
 
 static bool
 IsWindow(const char *name)
 {
--- a/js/xpconnect/wrappers/XrayWrapper.cpp
+++ b/js/xpconnect/wrappers/XrayWrapper.cpp
@@ -407,17 +407,19 @@ const JSClass JSXrayTraits::HolderClass 
     JS_PropertyStub, JS_StrictPropertyStub,
     JS_EnumerateStub, JS_ResolveStub, JS_ConvertStub
 };
 
 inline bool
 SilentFailure(JSContext *cx, HandleId id, const char *reason)
 {
 #ifdef DEBUG
-    nsDependentJSString name(id);
+    nsDependentJSString name;
+    if (!name.init(cx, id))
+        return false;
     AutoFilename filename;
     unsigned line = 0;
     DescribeScriptedCaller(cx, &filename, &line);
     NS_WARNING(nsPrintfCString("Denied access to property |%s| on Xrayed Object: %s (@%s:%u)",
                                NS_LossyConvertUTF16toASCII(name).get(), reason,
                                filename.get(), line).get());
 #endif
     return true;
@@ -2140,17 +2142,19 @@ XrayWrapper<Base, Traits>::getPropertyDe
     // we're set up (above) to cache (on the holder) anything that comes out of
     // resolveNativeProperty, which we don't want for something dynamic like
     // named access. So we just handle it separately here.
     nsGlobalWindow *win = nullptr;
     if (!desc.object() &&
         JSID_IS_STRING(id) &&
         (win = AsWindow(cx, wrapper)))
     {
-        nsDependentJSString name(id);
+        // Note - The infallibleInit() depends on the JSID_IS_STRING check above.
+        nsDependentJSString name;
+        name.infallibleInit(id);
         nsCOMPtr<nsIDOMWindow> childDOMWin = win->GetChildWindow(name);
         if (childDOMWin) {
             nsGlobalWindow *cwin = static_cast<nsGlobalWindow*>(childDOMWin.get());
             JSObject *childObj = cwin->FastGetGlobalJSObject();
             if (MOZ_UNLIKELY(!childObj))
                 return xpc::Throw(cx, NS_ERROR_FAILURE);
             FillPropertyDescriptor(desc, wrapper, ObjectValue(*childObj),
                                    /* readOnly = */ true);