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
☠☠ backed out by b5dc6d9578e7 ☠ ☠
authorBoris Zbarsky <bzbarsky@mit.edu>
Fri, 11 Oct 2013 12:28:23 -0400
changeset 164339 d8636e485e85f186a2d86d2f8c0304c92d802112
parent 164338 877a227c502f07a77a1600edf3285665218ae0fb
child 164340 61092280cb2a3939999c7cb30b69943d4f1182cb
push id3066
push userakeybl@mozilla.com
push dateMon, 09 Dec 2013 19:58:46 +0000
treeherdermozilla-beta@a31a0dce83aa [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskhuey
bugs882541
milestone27.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 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);
       }
     }
   }