Bug 882541 part 2. Fix a bug that crept in with overloading a string and a nullable number and then passing in null, due to the number conversion being conditional on the input type in that case. r=khuey
authorBoris Zbarsky <bzbarsky@mit.edu>
Fri, 11 Oct 2013 12:28:23 -0400
changeset 150527 d8636e485e85f186a2d86d2f8c0304c92d802112
parent 150526 877a227c502f07a77a1600edf3285665218ae0fb
child 150528 61092280cb2a3939999c7cb30b69943d4f1182cb
push id3006
push userkwierso@gmail.com
push dateSat, 12 Oct 2013 02:00:44 +0000
treeherderfx-team@3ff6c5ec4dc5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskhuey
bugs882541
milestone27.0a1
Bug 882541 part 2. Fix a bug that crept in with overloading a string and a nullable number and then passing in null, due to the number conversion being conditional on the input type in that case. r=khuey Also removes a footgun conversion operator on Nullable that was causing it to silently convert to int32_t.
dom/bindings/Codegen.py
dom/bindings/Nullable.h
dom/bindings/test/TestBindingHeader.h
dom/bindings/test/TestCodeGen.webidl
dom/bindings/test/TestExampleGen.webidl
dom/bindings/test/TestJSImplGen.webidl
dom/quota/QuotaManager.cpp
--- a/dom/bindings/Codegen.py
+++ b/dom/bindings/Codegen.py
@@ -5106,34 +5106,57 @@ class CGMethodCall(CGThing):
                 # If we got this far, we know we unwrapped to the right
                 # C++ type, so just do the call.  Start conversion with
                 # distinguishingIndex + 1, since we already converted
                 # distinguishingIndex.
                 caseBody.append(CGIndenter(
                         getPerSignatureCall(signature, distinguishingIndex + 1),
                         indent))
 
+            def hasConditionalConversion(type):
+                """
+                Return whether the argument conversion for this type will be
+                conditional on the type of incoming JS value.  For example, for
+                interface types the conversion is conditional on the incoming
+                value being isObject().
+
+                For the types for which this returns false, we do not have to
+                output extra isUndefined() or isNullOrUndefined() cases, because
+                null/undefined values will just fall through into our
+                unconditional conversion.
+                """
+                if type.isString() or type.isEnum():
+                    return False
+                if type.isBoolean():
+                    distinguishingTypes = (distinguishingType(s) for s in
+                                           possibleSignatures)
+                    return any(t.isString() or t.isEnum() or t.isNumeric()
+                               for t in distinguishingTypes)
+                if type.isNumeric():
+                    distinguishingTypes = (distinguishingType(s) for s in
+                                           possibleSignatures)
+                    return any(t.isString() or t.isEnum()
+                               for t in distinguishingTypes)
+                return True
+
             # First check for null or undefined.  That means looking for
             # nullable arguments at the distinguishing index and outputting a
-            # separate branch for them.  But if the nullable argument is a
-            # primitive, string, or enum, we don't need to do that.  The reason
+            # separate branch for them.  But if the nullable argument has an
+            # unconditional conversion, we don't need to do that.  The reason
             # for that is that at most one argument at the distinguishing index
             # is nullable (since two nullable arguments are not
-            # distinguishable), and all the argument types other than
-            # primitive/string/enum end up inside isObject() checks.  So if our
-            # nullable is a primitive/string/enum it's safe to not output the
-            # extra branch: we'll fall through to conversion for those types,
-            # which correctly handles null as needed, because isObject() will be
-            # false for null and undefined.
-            nullOrUndefSigs = [s for s in possibleSignatures
-                               if ((distinguishingType(s).nullable() and not
-                                    distinguishingType(s).isString() and not
-                                    distinguishingType(s).isEnum() and not
-                                   distinguishingType(s).isPrimitive()) or
-                                   distinguishingType(s).isDictionary())]
+            # distinguishable), and null/undefined values will always fall
+            # through to the unconditional conversion we have, if any, since
+            # they will fail whatever the conditions on the input value are for
+            # our other conversions.
+            nullOrUndefSigs = [
+                s for s in possibleSignatures
+                if ((distinguishingType(s).nullable() and
+                     hasConditionalConversion(distinguishingType(s))) or
+                    distinguishingType(s).isDictionary())]
             # Can't have multiple nullable types here
             assert len(nullOrUndefSigs) < 2
             if len(nullOrUndefSigs) > 0:
                 caseBody.append(CGGeneric("if (%s.isNullOrUndefined()) {" %
                                           distinguishingArg))
                 tryCall(nullOrUndefSigs[0], 2, isNullOrUndefined=True)
                 caseBody.append(CGGeneric("}"))
 
--- a/dom/bindings/Nullable.h
+++ b/dom/bindings/Nullable.h
@@ -76,21 +76,16 @@ public:
     return Equals(aOtherNullable);
   }
 
   bool operator!=(const Nullable<T>& aOtherNullable) const
   {
     return !Equals(aOtherNullable);
   }
 
-  operator bool() const
-  {
-    return !mIsNull;
-  }
-
   // Make it possible to use a const Nullable of an array type with other
   // array types.
   template<typename U>
   operator const Nullable< nsTArray<U> >&() const {
     // Make sure that T is ok to reinterpret to nsTArray<U>
     const nsTArray<U>& arr = mValue;
     (void)arr;
     return *reinterpret_cast<const Nullable< nsTArray<U> >*>(this);
--- a/dom/bindings/test/TestBindingHeader.h
+++ b/dom/bindings/test/TestBindingHeader.h
@@ -626,16 +626,26 @@ public:
   void Overload5(TestEnum);
   void Overload6(int32_t);
   void Overload6(bool);
   void Overload7(int32_t);
   void Overload7(bool);
   void Overload7(const nsCString&);
   void Overload8(int32_t);
   void Overload8(TestInterface&);
+  void Overload9(const Nullable<int32_t>&);
+  void Overload9(const nsAString&);
+  void Overload10(const Nullable<int32_t>&);
+  void Overload10(JSContext*, JS::Handle<JSObject*>);
+  void Overload11(int32_t);
+  void Overload11(const nsAString&);
+  void Overload12(int32_t);
+  void Overload12(const Nullable<bool>&);
+  void Overload13(const Nullable<int32_t>&);
+  void Overload13(bool);
 
   // Variadic handling
   void PassVariadicThirdArg(const nsAString&, int32_t,
                             const Sequence<OwningNonNull<TestInterface> >&);
 
   // Conditionally exposed methods/attributes
   bool Prefable1();
   bool Prefable2();
--- a/dom/bindings/test/TestCodeGen.webidl
+++ b/dom/bindings/test/TestCodeGen.webidl
@@ -576,16 +576,26 @@ interface TestInterface {
   void overload5(TestEnum arg);
   void overload6(long arg);
   void overload6(boolean arg);
   void overload7(long arg);
   void overload7(boolean arg);
   void overload7(ByteString arg);
   void overload8(long arg);
   void overload8(TestInterface arg);
+  void overload9(long? arg);
+  void overload9(DOMString arg);
+  void overload10(long? arg);
+  void overload10(object arg);
+  void overload11(long arg);
+  void overload11(DOMString? arg);
+  void overload12(long arg);
+  void overload12(boolean? arg);
+  void overload13(long? arg);
+  void overload13(boolean arg);
 
   // Variadic handling
   void passVariadicThirdArg(DOMString arg1, long arg2, TestInterface... arg3);
 
   // Conditionally exposed methods/attributes
   [Pref="abc.def"]
   readonly attribute boolean prefable1;
   [Pref="abc.def"]
--- a/dom/bindings/test/TestExampleGen.webidl
+++ b/dom/bindings/test/TestExampleGen.webidl
@@ -473,16 +473,26 @@ interface TestExampleInterface {
   void overload5(TestEnum arg);
   void overload6(long arg);
   void overload6(boolean arg);
   void overload7(long arg);
   void overload7(boolean arg);
   void overload7(ByteString arg);
   void overload8(long arg);
   void overload8(TestInterface arg);
+  void overload9(long? arg);
+  void overload9(DOMString arg);
+  void overload10(long? arg);
+  void overload10(object arg);
+  void overload11(long arg);
+  void overload11(DOMString? arg);
+  void overload12(long arg);
+  void overload12(boolean? arg);
+  void overload13(long? arg);
+  void overload13(boolean arg);
 
   // Variadic handling
   void passVariadicThirdArg(DOMString arg1, long arg2, TestInterface... arg3);
 
   // Conditionally exposed methods/attributes
   [Pref="abc.def"]
   readonly attribute boolean prefable1;
   [Pref="abc.def"]
--- a/dom/bindings/test/TestJSImplGen.webidl
+++ b/dom/bindings/test/TestJSImplGen.webidl
@@ -501,16 +501,26 @@ interface TestJSImplInterface {
   void overload5(MyTestEnum arg);
   void overload6(long arg);
   void overload6(boolean arg);
   void overload7(long arg);
   void overload7(boolean arg);
   void overload7(ByteString arg);
   void overload8(long arg);
   void overload8(TestJSImplInterface arg);
+  void overload9(long? arg);
+  void overload9(DOMString arg);
+  void overload10(long? arg);
+  void overload10(object arg);
+  void overload11(long arg);
+  void overload11(DOMString? arg);
+  void overload12(long arg);
+  void overload12(boolean? arg);
+  void overload13(long? arg);
+  void overload13(boolean arg);
 
   // Variadic handling
   void passVariadicThirdArg(DOMString arg1, long arg2, TestJSImplInterface... arg3);
 
   // Miscellania
   [LenientThis] attribute long attrWithLenientThis;
   // FIXME: Bug 863954 Unforgeable things get all confused when
   // non-JS-implemented interfaces inherit from JS-implemented ones or vice
--- a/dom/quota/QuotaManager.cpp
+++ b/dom/quota/QuotaManager.cpp
@@ -2797,17 +2797,17 @@ QuotaManager::RunSynchronizedOp(nsIOffli
 SynchronizedOp*
 QuotaManager::FindSynchronizedOp(const nsACString& aPattern,
                                  Nullable<PersistenceType> aPersistenceType,
                                  nsISupports* aId)
 {
   for (uint32_t index = 0; index < mSynchronizedOps.Length(); index++) {
     const nsAutoPtr<SynchronizedOp>& currentOp = mSynchronizedOps[index];
     if (PatternMatchesOrigin(aPattern, currentOp->mOriginOrPattern) &&
-        (!currentOp->mPersistenceType ||
+        (currentOp->mPersistenceType.IsNull() ||
          currentOp->mPersistenceType == aPersistenceType) &&
         (!currentOp->mId || currentOp->mId == aId)) {
       return currentOp;
     }
   }
 
   return nullptr;
 }
@@ -3062,28 +3062,28 @@ QuotaManager::CollectOriginsForEviction(
   // Collect active origins first.
   OriginCollection originCollection;
 
   // Add patterns and origins that have running or pending synchronized ops.
   // (add patterns first to reduce redundancy in the origin collection).
   uint32_t index;
   for (index = 0; index < mSynchronizedOps.Length(); index++) {
     nsAutoPtr<SynchronizedOp>& op = mSynchronizedOps[index];
-    if (!op->mPersistenceType ||
+    if (op->mPersistenceType.IsNull() ||
         op->mPersistenceType.Value() == PERSISTENCE_TYPE_TEMPORARY) {
       if (op->mOriginOrPattern.IsPattern() &&
           !originCollection.ContainsPattern(op->mOriginOrPattern)) {
         originCollection.AddPattern(op->mOriginOrPattern);
       }
     }
   }
 
   for (index = 0; index < mSynchronizedOps.Length(); index++) {
     nsAutoPtr<SynchronizedOp>& op = mSynchronizedOps[index];
-    if (!op->mPersistenceType ||
+    if (op->mPersistenceType.IsNull() ||
         op->mPersistenceType.Value() == PERSISTENCE_TYPE_TEMPORARY) {
       if (op->mOriginOrPattern.IsOrigin() &&
           !originCollection.ContainsOrigin(op->mOriginOrPattern)) {
         originCollection.AddOrigin(op->mOriginOrPattern);
       }
     }
   }