Bug 1477432 - Part 2: Avoid using nsIJSID in GenerateQI, and produce better diagnostics, r=kmag
authorNika Layzell <nika@thelayzells.com>
Wed, 18 Jul 2018 00:02:28 -0400
changeset 503333 404c4d583acdf448fe77a514e90de26dd5d5893f
parent 503332 2e250aa206d11319cfaab13e9d57c1095940668c
child 503334 ecbe615201fab977a6d753230b146bd04a83b156
push id10290
push userffxbld-merge
push dateMon, 03 Dec 2018 16:23:23 +0000
treeherdermozilla-beta@700bed2445e6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskmag
bugs1477432
milestone65.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1477432 - Part 2: Avoid using nsIJSID in GenerateQI, and produce better diagnostics, r=kmag This is the first part of hiding the implementation of nsIJSID behind the interface added in Part 1, such that we can substitute that implementation out. I had to make a couple of changes to fix the errors caused by the new behaviour in GenerateQI. Differential Revision: https://phabricator.services.mozilla.com/D2279
browser/modules/Sanitizer.jsm
dom/base/ChromeUtils.h
dom/base/MozQueryInterface.cpp
dom/chrome-webidl/ChromeUtils.webidl
editor/composer/test/test_bug1266815.html
js/xpconnect/tests/unit/test_generateQI.js
toolkit/components/satchel/FormHistoryStartup.js
--- a/browser/modules/Sanitizer.jsm
+++ b/browser/modules/Sanitizer.jsm
@@ -288,17 +288,17 @@ var Sanitizer = {
         if (this.shouldSanitizeNewTabContainer) {
           addPendingSanitization("newtab-container", [], {});
         }
       }
     }
   },
 
   QueryInterface: ChromeUtils.generateQI([
-    Ci.nsiObserver,
+    Ci.nsIObserver,
     Ci.nsISupportsWeakReference,
   ]),
 
   // This method is meant to be used by tests.
   async runSanitizeOnShutdown() {
     return sanitizeOnShutdown({ isShutdown: true });
   },
 
--- a/dom/base/ChromeUtils.h
+++ b/dom/base/ChromeUtils.h
@@ -121,17 +121,17 @@ public:
   // Implemented in js/xpconnect/loader/ChromeScriptLoader.cpp
   static already_AddRefed<Promise>
   CompileScript(GlobalObject& aGlobal,
                 const nsAString& aUrl,
                 const dom::CompileScriptOptionsDictionary& aOptions,
                 ErrorResult& aRv);
 
   static MozQueryInterface*
-  GenerateQI(const GlobalObject& global, const Sequence<OwningStringOrIID>& interfaces,
+  GenerateQI(const GlobalObject& global, const Sequence<JS::Value>& interfaces,
              ErrorResult& aRv);
 
   static void WaiveXrays(GlobalObject& aGlobal,
                          JS::HandleValue aVal,
                          JS::MutableHandleValue aRetval,
                          ErrorResult& aRv);
 
   static void UnwaiveXrays(GlobalObject& aGlobal,
--- a/dom/base/MozQueryInterface.cpp
+++ b/dom/base/MozQueryInterface.cpp
@@ -1,16 +1,17 @@
 /* -*- 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/. */
 
 #include "ChromeUtils.h"
 #include "MozQueryInterface.h"
+#include "nsIException.h"
 
 #include <string.h>
 
 #include "jsapi.h"
 
 #include "xpcpublic.h"
 #include "xpcjsid.h"
 
@@ -25,67 +26,47 @@ static_assert(IID_SIZE == 16,
 static int
 CompareIIDs(const nsIID& aA, const nsIID &aB)
 {
   return memcmp((void*)&aA.m0, (void*)&aB.m0, IID_SIZE);
 }
 
 /* static */
 MozQueryInterface*
-ChromeUtils::GenerateQI(const GlobalObject& aGlobal, const Sequence<OwningStringOrIID>& aInterfaces, ErrorResult& aRv)
+ChromeUtils::GenerateQI(const GlobalObject& aGlobal,
+                        const Sequence<JS::Value>& aInterfaces,
+                        ErrorResult& aRv)
 {
   JSContext* cx = aGlobal.Context();
-  JS::RootedObject xpcIfaces(cx);
 
   nsTArray<nsIID> ifaces;
 
-  JS::RootedValue val(cx);
-  for (auto& iface : aInterfaces) {
-    if (iface.IsIID()) {
-      ifaces.AppendElement(*iface.GetAsIID()->GetID());
+  JS::RootedValue iface(cx);
+  for (uint32_t idx = 0; idx < aInterfaces.Length(); ++idx) {
+    iface = aInterfaces[idx];
+
+    // Handle ID objects
+    if (Maybe<nsID> id = xpc::JSValue2ID(cx, iface)) {
+      ifaces.AppendElement(*id);
       continue;
     }
 
-    // If we have a string value, we need to look up the interface name. The
-    // simplest and most efficient way to do this is to just grab the "Ci"
-    // object from the global scope.
-    if (!xpcIfaces) {
-      JS::RootedObject global(cx, aGlobal.Get());
-      if (!JS_GetProperty(cx, global, "Ci", &val)) {
-        aRv.NoteJSContextException(cx);
-        return nullptr;
+    // Accept string valued names
+    if (iface.isString()) {
+      JS::UniqueChars name = JS_EncodeStringToLatin1(cx, iface.toString());
+
+      const nsXPTInterfaceInfo* iinfo = nsXPTInterfaceInfo::ByName(name.get());
+      if (iinfo) {
+        ifaces.AppendElement(iinfo->IID());
+        continue;
       }
-      if (!val.isObject()) {
-        aRv.Throw(NS_ERROR_UNEXPECTED);
-        return nullptr;
-      }
-      xpcIfaces = &val.toObject();
     }
 
-    auto& name = iface.GetAsString();
-    if (!JS_GetUCProperty(cx, xpcIfaces, name.get(), name.Length(), &val)) {
-      aRv.NoteJSContextException(cx);
-      return nullptr;
-    }
-
-    if (val.isNullOrUndefined()) {
-      continue;
-    }
-    if (!val.isObject()) {
-      aRv.Throw(NS_ERROR_INVALID_ARG);
-      return nullptr;
-    }
-
-    nsCOMPtr<nsISupports> base = xpc::UnwrapReflectorToISupports(&val.toObject());
-    nsCOMPtr<nsIJSID> iid = do_QueryInterface(base);
-    if (!iid) {
-      aRv.Throw(NS_ERROR_INVALID_ARG);
-      return nullptr;
-    }
-    ifaces.AppendElement(*iid->GetID());
+    // NOTE: We ignore unknown interfaces here because in some cases we try to
+    // pass them in to support multiple platforms.
   }
 
   MOZ_ASSERT(!ifaces.Contains(NS_GET_IID(nsISupports), CompareIIDs));
   ifaces.AppendElement(NS_GET_IID(nsISupports));
 
   ifaces.Sort(CompareIIDs);
 
   return new MozQueryInterface(std::move(ifaces));
--- a/dom/chrome-webidl/ChromeUtils.webidl
+++ b/dom/chrome-webidl/ChromeUtils.webidl
@@ -210,27 +210,24 @@ partial namespace ChromeUtils {
   Promise<PrecompiledScript>
   compileScript(DOMString url, optional CompileScriptOptionsDictionary options);
 
   /**
    * Returns an optimized QueryInterface method which, when called from
    * JavaScript, acts as an ordinary QueryInterface function call, and when
    * called from XPConnect, circumvents JSAPI entirely.
    *
-   * The list of interfaces may include a mix of nsIJSID objects and interface
-   * name strings. Strings for nonexistent interface names are silently
-   * ignored, as long as they don't refer to any non-IID property of the Ci
-   * global. Any non-IID value is implicitly coerced to a string, and treated
-   * as an interface name.
+   * The list of interfaces may include a mix of JS ID objects and interface
+   * name strings.
    *
    * nsISupports is implicitly supported, and must not be included in the
    * interface list.
    */
   [Affects=Nothing, NewObject, Throws]
-  MozQueryInterface generateQI(sequence<(DOMString or IID)> interfaces);
+  MozQueryInterface generateQI(sequence<any> interfaces);
 
   /**
    * Waive Xray on a given value. Identity op for primitives.
    */
   [Throws]
   any waiveXrays(any val);
 
   /**
--- a/editor/composer/test/test_bug1266815.html
+++ b/editor/composer/test/test_bug1266815.html
@@ -3,16 +3,18 @@
 <head>
   <script type="text/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
   <script type="text/javascript" src="chrome://mochikit/content/tests/SimpleTest/AddTask.js"></script>
   <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css" />
 </head>
 <body>
 <p id="display"></p>
 <script type="text/javascript">
+// XXX(nika): Why are we using SpecialPowers here? If we're a chrome mochitest
+// can't we avoid using the SpecialPowers wrappers?
 const Cc = SpecialPowers.Cc;
 const Ci = SpecialPowers.Ci;
 const Cu = SpecialPowers.Cu;
 
 const {XPCOMUtils} = ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm", {});
 
 const HELPERAPP_DIALOG_CID =
         SpecialPowers.wrap(SpecialPowers.Components)
@@ -33,17 +35,17 @@ var helperAppDlgPromise = new Promise(fu
 
   HelperAppLauncherDialog.prototype = {
     show(aLauncher, aWindowContext, aReason) {
       ok(true, "Whether showing Dialog");
       resolve();
       registrar.unregisterFactory(MOCK_HELPERAPP_DIALOG_CID,
                                   mockHelperAppService);
     },
-    QueryInterface: ChromeUtils.generateQI([Ci.nsIHelperAppLauncherDialog]),
+    QueryInterface: ChromeUtils.generateQI(["nsIHelperAppLauncherDialog"]),
   };
 
   mockHelperAppService = XPCOMUtils._getFactory(HelperAppLauncherDialog);
   registrar.registerFactory(MOCK_HELPERAPP_DIALOG_CID, "",
                             HELPERAPP_DIALOG_CONTRACT_ID,
                             mockHelperAppService);
 });
 
--- a/js/xpconnect/tests/unit/test_generateQI.js
+++ b/js/xpconnect/tests/unit/test_generateQI.js
@@ -19,14 +19,11 @@ add_task(async function test_generateQI(
 
   // Non-IID values get stringified, and don't cause any errors as long
   // as there isn't a non-IID property with the same name on Ci.
   checkQI([Ci.nsIPropertyBag, "nsIPropertyBag2", null, Object], Ci.nsIPropertyBag2);
 
   ChromeUtils.generateQI([])(Ci.nsISupports);
 
   // Test failure scenarios.
-  Assert.throws(() => ChromeUtils.generateQI(["toString"]),
-                e => e.result == Cr.NS_ERROR_INVALID_ARG);
-
   Assert.throws(() => checkQI([], Ci.nsIPropertyBag),
                 e => e.result == Cr.NS_ERROR_NO_INTERFACE);
 });
--- a/toolkit/components/satchel/FormHistoryStartup.js
+++ b/toolkit/components/satchel/FormHistoryStartup.js
@@ -11,17 +11,16 @@ ChromeUtils.defineModuleGetter(this, "Fo
 function FormHistoryStartup() { }
 
 FormHistoryStartup.prototype = {
   classID: Components.ID("{3A0012EB-007F-4BB8-AA81-A07385F77A25}"),
 
   QueryInterface: ChromeUtils.generateQI([
     Ci.nsIObserver,
     Ci.nsISupportsWeakReference,
-    Ci.nsIFrameMessageListener,
   ]),
 
   observe(subject, topic, data) {
     switch (topic) {
       case "nsPref:changed":
         FormHistory.updatePrefs();
         break;
       case "idle-daily":