JavaScriptCore:
authorbarraclough@apple.com <barraclough@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 12 May 2009 09:18:44 +0000 (09:18 +0000)
committerbarraclough@apple.com <barraclough@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 12 May 2009 09:18:44 +0000 (09:18 +0000)
2009-05-12  Gavin Barraclough  <barraclough@apple.com>

        Reviewed by Oliver Hunt.

        instanceof should throw if the constructor being tested does not implement
        'HasInstance" (i.e. is a function).  Instead we were returning false.

        * interpreter/Interpreter.cpp:
        (JSC::isInvalidParamForIn):
        (JSC::isInvalidParamForInstanceOf):
        (JSC::Interpreter::privateExecute):
        * jit/JITStubs.cpp:
        (JSC::JITStubs::cti_op_instanceof):
        * tests/mozilla/ecma_2/instanceof/instanceof-003.js:
            Fix broken test case.
        * tests/mozilla/ecma_2/instanceof/regress-7635.js:
            Remove broken test case (was an exact duplicate of a test in instanceof-003.js).

LayoutTests:

2009-05-12  Gavin Barraclough  <barraclough@apple.com>

        Reviewed by Oliver Hunt.

        Test was checked in with one test case disabled since it exposed an existing bug;
        enable it now.

        * fast/js/instance-of-immediates-expected.txt:
        * fast/js/resources/instance-of-immediates.js:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@43560 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JavaScriptCore/ChangeLog
JavaScriptCore/interpreter/Interpreter.cpp
JavaScriptCore/jit/JITStubs.cpp
JavaScriptCore/tests/mozilla/ecma_2/instanceof/instanceof-003.js
JavaScriptCore/tests/mozilla/ecma_2/instanceof/regress-7635.js
LayoutTests/ChangeLog
LayoutTests/fast/js/instance-of-immediates-expected.txt
LayoutTests/fast/js/resources/instance-of-immediates.js

index 2c7dcd4..213bf02 100644 (file)
@@ -1,3 +1,21 @@
+2009-05-12  Gavin Barraclough  <barraclough@apple.com>
+
+        Reviewed by Oliver Hunt.
+
+        instanceof should throw if the constructor being tested does not implement
+        'HasInstance" (i.e. is a function).  Instead we were returning false.
+
+        * interpreter/Interpreter.cpp:
+        (JSC::isInvalidParamForIn):
+        (JSC::isInvalidParamForInstanceOf):
+        (JSC::Interpreter::privateExecute):
+        * jit/JITStubs.cpp:
+        (JSC::JITStubs::cti_op_instanceof):
+        * tests/mozilla/ecma_2/instanceof/instanceof-003.js:
+            Fix broken test case.
+        * tests/mozilla/ecma_2/instanceof/regress-7635.js:
+            Remove broken test case (was an exact duplicate of a test in instanceof-003.js).
+
 2009-05-12  Oliver Hunt  <oliver@apple.com>
 
         Reviewed by Gavin Barraclough.
index 87b9afb..e3a0171 100644 (file)
@@ -317,11 +317,19 @@ ALWAYS_INLINE CallFrame* Interpreter::slideRegisterWindowForCall(CodeBlock* newC
     return CallFrame::create(r);
 }
 
-static NEVER_INLINE bool isNotObject(CallFrame* callFrame, bool forInstanceOf, CodeBlock* codeBlock, const Instruction* vPC, JSValue value, JSValue& exceptionData)
+static NEVER_INLINE bool isInvalidParamForIn(CallFrame* callFrame, CodeBlock* codeBlock, const Instruction* vPC, JSValue value, JSValue& exceptionData)
 {
     if (value.isObject())
         return false;
-    exceptionData = createInvalidParamError(callFrame, forInstanceOf ? "instanceof" : "in" , value, vPC - codeBlock->instructions().begin(), codeBlock);
+    exceptionData = createInvalidParamError(callFrame, "in" , value, vPC - codeBlock->instructions().begin(), codeBlock);
+    return true;
+}
+
+static NEVER_INLINE bool isInvalidParamForInstanceOf(CallFrame* callFrame, CodeBlock* codeBlock, const Instruction* vPC, JSValue value, JSValue& exceptionData)
+{
+    if (value.isObject() && asObject(value)->structure()->typeInfo().implementsHasInstance())
+        return false;
+    exceptionData = createInvalidParamError(callFrame, "instanceof" , value, vPC - codeBlock->instructions().begin(), codeBlock);
     return true;
 }
 
@@ -1812,16 +1820,12 @@ JSValue Interpreter::privateExecute(ExecutionFlag flag, RegisterFile* registerFi
 
         JSValue baseVal = callFrame[base].jsValue();
 
-        if (isNotObject(callFrame, true, callFrame->codeBlock(), vPC, baseVal, exceptionValue))
+        if (isInvalidParamForInstanceOf(callFrame, callFrame->codeBlock(), vPC, baseVal, exceptionValue))
             goto vm_throw;
 
-        JSObject* baseObj = asObject(baseVal);
-        if (baseObj->structure()->typeInfo().implementsHasInstance()) {
-            bool result = baseObj->hasInstance(callFrame, callFrame[value].jsValue(), callFrame[baseProto].jsValue());
-            CHECK_FOR_EXCEPTION();
-            callFrame[dst] = jsBoolean(result);
-        } else
-            callFrame[dst] = jsBoolean(false);
+        bool result = asObject(baseVal)->hasInstance(callFrame, callFrame[value].jsValue(), callFrame[baseProto].jsValue());
+        CHECK_FOR_EXCEPTION();
+        callFrame[dst] = jsBoolean(result);
 
         vPC += 5;
         NEXT_INSTRUCTION();
@@ -1938,7 +1942,7 @@ JSValue Interpreter::privateExecute(ExecutionFlag flag, RegisterFile* registerFi
         int base = (++vPC)->u.operand;
 
         JSValue baseVal = callFrame[base].jsValue();
-        if (isNotObject(callFrame, false, callFrame->codeBlock(), vPC, baseVal, exceptionValue))
+        if (isInvalidParamForIn(callFrame, callFrame->codeBlock(), vPC, baseVal, exceptionValue))
             goto vm_throw;
 
         JSObject* baseObj = asObject(baseVal);
index 187217b..b635e7b 100644 (file)
@@ -893,23 +893,23 @@ EncodedJSValue JITStubs::cti_op_instanceof(STUB_ARGS_DECLARATION)
     JSValue baseVal = stackFrame.args[1].jsValue();
     JSValue proto = stackFrame.args[2].jsValue();
 
-    // at least one of these checks must have failed to get to the slow case
+    // At least one of these checks must have failed to get to the slow case.
     ASSERT(!value.isCell() || !baseVal.isCell() || !proto.isCell()
            || !value.isObject() || !baseVal.isObject() || !proto.isObject() 
            || (asObject(baseVal)->structure()->typeInfo().flags() & (ImplementsHasInstance | OverridesHasInstance)) != ImplementsHasInstance);
 
-    if (!baseVal.isObject()) {
+
+    // ECMA-262 15.3.5.3:
+    // Throw an exception either if baseVal is not an object, or if it does not implement 'HasInstance' (i.e. is a function).
+    TypeInfo typeInfo(UnspecifiedType, 0);
+    if (!baseVal.isObject() || !(typeInfo = asObject(baseVal)->structure()->typeInfo()).implementsHasInstance()) {
         CallFrame* callFrame = stackFrame.callFrame;
         CodeBlock* codeBlock = callFrame->codeBlock();
         unsigned vPCIndex = codeBlock->getBytecodeIndex(callFrame, STUB_RETURN_ADDRESS);
         stackFrame.globalData->exception = createInvalidParamError(callFrame, "instanceof", baseVal, vPCIndex, codeBlock);
         VM_THROW_EXCEPTION();
     }
-
-    JSObject* baseObj = asObject(baseVal);
-    TypeInfo typeInfo = baseObj->structure()->typeInfo();
-    if (!typeInfo.implementsHasInstance())
-        return JSValue::encode(jsBoolean(false));
+    ASSERT(typeInfo.type() != UnspecifiedType);
 
     if (!typeInfo.overridesHasInstance()) {
         if (!value.isObject())
@@ -921,7 +921,7 @@ EncodedJSValue JITStubs::cti_op_instanceof(STUB_ARGS_DECLARATION)
         }
     }
 
-    JSValue result = jsBoolean(baseObj->hasInstance(callFrame, value, proto));
+    JSValue result = jsBoolean(asObject(baseVal)->hasInstance(callFrame, value, proto));
     CHECK_FOR_EXCEPTION_AT_END();
 
     return JSValue::encode(result);
index 1b0a01b..c8f84ba 100644 (file)
@@ -16,6 +16,19 @@ I think this should be 'false'
 
     Author:             christine@netscape.com
     Date:               12 november 1997
+
+
+The test case described above is correct, however the second test case in this file is not,
+'o instanceof o' should thow an exception.  According to ECMA-262:
+
+    8.6.2 Internal Properties and Methods:
+        "... only Function objects implement [[HasInstance]]"
+    11.8.6 The instanceof operator:
+        "6.If Result(4) does not have a [[HasInstance]] method, throw a TypeError exception."
+
+{} does not implement [[HasInstance]] (since it is not a function), so passing it as the
+constructor to be tested to instanceof should result in a TypeError being thrown.
+
 */
     var SECTION = "instanceof-003";
     var VERSION = "ECMA_2";
@@ -35,12 +48,10 @@ I think this should be 'false'
         theproto instanceof Foo );
 
 
-    var o = {};
-
     AddTestCase(
         "o = {}; o instanceof o",
-        false,
-        o instanceof o );
+        "EXCEPTION",
+        (function(){ try { var o = {}; o instanceof o; return "no exception"; } catch (e) { return "EXCEPTION"; } } )() );
 
 
     test();
index 4ccb9d4..cab6ed9 100644 (file)
  *  Author:             
  */
 
-    var SECTION = "instanceof";       // provide a document reference (ie, ECMA section)
-    var VERSION = "ECMA_2"; // Version of JavaScript or ECMA
-    var TITLE   = "Regression test for Bugzilla #7635";       // Provide ECMA section title or a description
-    var BUGNUMBER = "http://bugzilla.mozilla.org/show_bug.cgi?id=7635";     // Provide URL to bugsplat or bugzilla report
+var SECTION = "instanceof";       // provide a document reference (ie, ECMA section)
+var VERSION = "ECMA_2"; // Version of JavaScript or ECMA
+var TITLE   = "Regression test for Bugzilla #7635";       // Provide ECMA section title or a description
+var BUGNUMBER = "http://bugzilla.mozilla.org/show_bug.cgi?id=7635";     // Provide URL to bugsplat or bugzilla report
 
-    startTest();               // leave this alone
+startTest();               // leave this alone
 
-    /*
-     * Calls to AddTestCase here. AddTestCase is a function that is defined
-     * in shell.js and takes three arguments:
-     * - a string representation of what is being tested
-     * - the expected result
-     * - the actual result
-     *
-     * For example, a test might look like this:
-     *
-     * var zip = /[\d]{5}$/;
-     *
-     * AddTestCase(
-     * "zip = /[\d]{5}$/; \"PO Box 12345 Boston, MA 02134\".match(zip)",   // description of the test
-     *  "02134",                                                           // expected result
-     *  "PO Box 12345 Boston, MA 02134".match(zip) );                      // actual result
-     *
-     */
-
-       function Foo() {}
-       theproto = {};
-       Foo.prototype = theproto
-       theproto instanceof Foo
+/*
+ * Calls to AddTestCase here. AddTestCase is a function that is defined
+ * in shell.js and takes three arguments:
+ * - a string representation of what is being tested
+ * - the expected result
+ * - the actual result
+ *
+ * For example, a test might look like this:
+ *
+ * var zip = /[\d]{5}$/;
+ *
+ * AddTestCase(
+ * "zip = /[\d]{5}$/; \"PO Box 12345 Boston, MA 02134\".match(zip)",   // description of the test
+ *  "02134",                                                           // expected result
+ *  "PO Box 12345 Boston, MA 02134".match(zip) );                      // actual result
+ *
+ */
 
+function Foo() {}
+theproto = {};
+Foo.prototype = theproto
+theproto instanceof Foo
 
-       AddTestCase( "function Foo() {}; theproto = {}; Foo.prototype = theproto; theproto instanceof Foo",
-                       false,
-                       theproto instanceof Foo );
-       
-       var o  = {};
 
-       AddTestCase( "var o = {}; o instanceof o", false, o instanceof o );
+AddTestCase( "function Foo() {}; theproto = {}; Foo.prototype = theproto; theproto instanceof Foo",
+        false,
+        theproto instanceof Foo );
 
-       var f = new Function();
+var f = new Function();
 
-       AddTestCase( "var f = new Function(); f instanceof f", false, f instanceof f );
+AddTestCase( "var f = new Function(); f instanceof f", false, f instanceof f );
 
 
-    test();       // leave this alone.  this executes the test cases and
-                  // displays results.
+test();       // leave this alone.  this executes the test cases and
+              // displays results.
index 47c074a..009522d 100644 (file)
@@ -1,3 +1,13 @@
+2009-05-12  Gavin Barraclough  <barraclough@apple.com>
+
+        Reviewed by Oliver Hunt.
+
+        Test was checked in with one test case disabled since it exposed an existing bug;
+        enable it now.
+
+        * fast/js/instance-of-immediates-expected.txt:
+        * fast/js/resources/instance-of-immediates.js:
+
 2009-05-11  Brady Eidson  <beidson@apple.com>
 
         Add a third copy of the test results for this test to make Tiger bots happy.
index 36ced1f..54c4fd4 100644 (file)
@@ -6,6 +6,9 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE
 PASS (1 instanceof 1) threw exception TypeError: Result of expression '1' [1] is not a valid argument for 'instanceof'..
 PASS ({} instanceof 1) threw exception TypeError: Result of expression '1' [1] is not a valid argument for 'instanceof'..
 PASS (obj instanceof 1) threw exception TypeError: Result of expression '1' [1] is not a valid argument for 'instanceof'..
+PASS (1 instanceof {}) threw exception TypeError: Result of expression '{}' [[object Object]] is not a valid argument for 'instanceof'..
+PASS ({} instanceof {}) threw exception TypeError: Result of expression '{}' [[object Object]] is not a valid argument for 'instanceof'..
+PASS (obj instanceof {}) threw exception TypeError: Result of expression '{}' [[object Object]] is not a valid argument for 'instanceof'..
 PASS (1 instanceof Constructor) is false
 PASS ({} instanceof Constructor) is false
 PASS (obj instanceof Constructor) is true
index 09723c6..95079e3 100644 (file)
@@ -17,9 +17,8 @@ function testSet(constructor, testMethod)
 testSet("1", { "1":shouldThrow, "{}":shouldThrow, "obj":shouldThrow });
 
 // Test set 2, test passsing an empty object ({}) as the constructor to be tested for.
-// Fixme: these should thow exceptions, since {} is not function (specifically, since {} does not implemet HasInstance).
-// Currently WebKit returns false in these three cases.
-//testSet("{}", { "1":shouldThrow, "{}":shouldThrow, "obj":shouldThrow });
+// As well as being an object, the constructor must implement 'HasInstance' (i.e. be a function), so these should all throw too.
+testSet("{}", { "1":shouldThrow, "{}":shouldThrow, "obj":shouldThrow });
 
 // Test set 3, test passsing Constructor as the constructor to be tested for.
 // Nothing should except, the third test should pass, since obj is an instance of Constructor.