Overzealous type validation in method_check
authoroliver@apple.com <oliver@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 13 Jul 2011 00:53:17 +0000 (00:53 +0000)
committeroliver@apple.com <oliver@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 13 Jul 2011 00:53:17 +0000 (00:53 +0000)
https://bugs.webkit.org/show_bug.cgi?id=64415

Reviewed by Gavin Barraclough.

../../../../Volumes/Data/git/WebKit/OpenSource/LayoutTests:

Make sure we don't trip any assertions when caching access
to an InternalFunction

* fast/js/script-tests/method-check.js:

../../../../Volumes/Data/git/WebKit/OpenSource/Source/JavaScriptCore:

method_check is essentially just a value look up
optimisation, but it internally stores the value
as a JSFunction, even though it never relies on
this fact.  Under GC validation however we end up
trying to enforce that assumption.  The fix is
simply to store the value as a correct supertype.

* bytecode/CodeBlock.h:
* dfg/DFGRepatch.cpp:
(JSC::DFG::dfgRepatchGetMethodFast):
(JSC::DFG::tryCacheGetMethod):
* jit/JIT.h:
* jit/JITPropertyAccess.cpp:
(JSC::JIT::patchMethodCallProto):
* jit/JITStubs.cpp:
(JSC::DEFINE_STUB_FUNCTION):

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

LayoutTests/ChangeLog
LayoutTests/fast/js/script-tests/method-check.js
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/CodeBlock.h
Source/JavaScriptCore/dfg/DFGRepatch.cpp
Source/JavaScriptCore/jit/JIT.h
Source/JavaScriptCore/jit/JITPropertyAccess.cpp
Source/JavaScriptCore/jit/JITStubs.cpp

index 2243958..861817a 100644 (file)
@@ -1,3 +1,15 @@
+2011-07-12  Oliver Hunt  <oliver@apple.com>
+
+        Overzealous type validation in method_check
+        https://bugs.webkit.org/show_bug.cgi?id=64415
+
+        Reviewed by Gavin Barraclough.
+
+        Make sure we don't trip any assertions when caching access
+        to an InternalFunction
+
+        * fast/js/script-tests/method-check.js:
+
 2011-07-12  Joseph Pecoraro  <joepeck@webkit.org>
 
         Unreviewed. Skipping a few tests which fail due to differing output
index a4f8bdf..af4932a 100644 (file)
@@ -53,4 +53,8 @@ for (var i=0; i<100; ++i)
 totalizer.makeCall(addOneHundred);
 shouldBe('total', '200');
 
+// Check that we don't assert when method_check is applied to a non-JSFunction
+for (var i = 0; i < 10000; i++)
+    Array.constructor(1);
+
 var successfullyParsed = true;
index 78dd7b2..f17c2f1 100644 (file)
@@ -1,3 +1,27 @@
+2011-07-12  Oliver Hunt  <oliver@apple.com>
+
+        Overzealous type validation in method_check
+        https://bugs.webkit.org/show_bug.cgi?id=64415
+
+        Reviewed by Gavin Barraclough.
+
+        method_check is essentially just a value look up
+        optimisation, but it internally stores the value
+        as a JSFunction, even though it never relies on
+        this fact.  Under GC validation however we end up
+        trying to enforce that assumption.  The fix is
+        simply to store the value as a correct supertype.
+
+        * bytecode/CodeBlock.h:
+        * dfg/DFGRepatch.cpp:
+        (JSC::DFG::dfgRepatchGetMethodFast):
+        (JSC::DFG::tryCacheGetMethod):
+        * jit/JIT.h:
+        * jit/JITPropertyAccess.cpp:
+        (JSC::JIT::patchMethodCallProto):
+        * jit/JITStubs.cpp:
+        (JSC::DEFINE_STUB_FUNCTION):
+
 2011-07-12  Filip Pizlo  <fpizlo@apple.com>
 
         COLLECT_ON_EVERY_ALLOCATION no longer works.
index 71ca039..41ee97b 100644 (file)
@@ -146,7 +146,9 @@ namespace JSC {
         CodeLocationCall callReturnLocation;
         JITWriteBarrier<Structure> cachedStructure;
         JITWriteBarrier<Structure> cachedPrototypeStructure;
-        JITWriteBarrier<JSFunction> cachedFunction;
+        // We'd like this to actually be JSFunction, but InternalFunction and JSFunction
+        // don't have a common parent class and we allow specialisation on both
+        JITWriteBarrier<JSObjectWithGlobalObject> cachedFunction;
         JITWriteBarrier<JSObject> cachedPrototype;
         bool seen;
     };
index 72006c6..2e9c30a 100644 (file)
@@ -161,7 +161,7 @@ void dfgRepatchGetByID(ExecState* exec, JSValue baseValue, const Identifier& pro
         dfgRepatchCall(exec->codeBlock(), stubInfo.callReturnLocation, operationGetById);
 }
 
-static void dfgRepatchGetMethodFast(JSGlobalData* globalData, CodeBlock* codeBlock, MethodCallLinkInfo& methodInfo, JSFunction* callee, Structure* structure, JSObject* slotBaseObject)
+static void dfgRepatchGetMethodFast(JSGlobalData* globalData, CodeBlock* codeBlock, MethodCallLinkInfo& methodInfo, JSObjectWithGlobalObject* callee, Structure* structure, JSObject* slotBaseObject)
 {
     ScriptExecutable* owner = codeBlock->ownerExecutable();
     
@@ -190,7 +190,7 @@ static bool tryCacheGetMethod(ExecState* exec, JSValue baseValue, const Identifi
         && (slotBaseObject = asObject(slot.slotBase()))->getPropertySpecificValue(exec, propertyName, specific)
         && specific) {
         
-        JSFunction* callee = (JSFunction*)specific;
+        JSObjectWithGlobalObject* callee = (JSObjectWithGlobalObject*)specific;
         
         // Since we're accessing a prototype in a loop, it's a good bet that it
         // should not be treated as a dictionary.
index 4934f01..de09d1d 100644 (file)
@@ -238,7 +238,7 @@ namespace JSC {
 
         static void patchGetByIdSelf(CodeBlock* codeblock, StructureStubInfo*, Structure*, size_t cachedOffset, ReturnAddressPtr returnAddress);
         static void patchPutByIdReplace(CodeBlock* codeblock, StructureStubInfo*, Structure*, size_t cachedOffset, ReturnAddressPtr returnAddress, bool direct);
-        static void patchMethodCallProto(JSGlobalData&, CodeBlock* codeblock, MethodCallLinkInfo&, JSFunction*, Structure*, JSObject*, ReturnAddressPtr);
+        static void patchMethodCallProto(JSGlobalData&, CodeBlock* codeblock, MethodCallLinkInfo&, JSObjectWithGlobalObject*, Structure*, JSObject*, ReturnAddressPtr);
 
         static void compilePatchGetArrayLength(JSGlobalData* globalData, CodeBlock* codeBlock, ReturnAddressPtr returnAddress)
         {
index aef45ae..92f9771 100644 (file)
@@ -1036,7 +1036,7 @@ void JIT::testPrototype(JSValue prototype, JumpList& failureCases)
     failureCases.append(branchPtr(NotEqual, Address(regT3, JSCell::structureOffset()), TrustedImmPtr(prototype.asCell()->structure())));
 }
 
-void JIT::patchMethodCallProto(JSGlobalData& globalData, CodeBlock* codeBlock, MethodCallLinkInfo& methodCallLinkInfo, JSFunction* callee, Structure* structure, JSObject* proto, ReturnAddressPtr returnAddress)
+void JIT::patchMethodCallProto(JSGlobalData& globalData, CodeBlock* codeBlock, MethodCallLinkInfo& methodCallLinkInfo, JSObjectWithGlobalObject* callee, Structure* structure, JSObject* proto, ReturnAddressPtr returnAddress)
 {
     RepatchBuffer repatchBuffer(codeBlock);
     
index 63b829e..4212080 100644 (file)
@@ -1529,7 +1529,7 @@ DEFINE_STUB_FUNCTION(EncodedJSValue, op_get_by_id_method_check)
         && specific
         ) {
 
-        JSFunction* callee = (JSFunction*)specific;
+        JSObjectWithGlobalObject* callee = (JSObjectWithGlobalObject*)specific;
 
         // Since we're accessing a prototype in a loop, it's a good bet that it
         // should not be treated as a dictionary.