JavaScriptCore:
authorggaren@apple.com <ggaren@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 22 Jan 2008 06:18:10 +0000 (06:18 +0000)
committerggaren@apple.com <ggaren@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 22 Jan 2008 06:18:10 +0000 (06:18 +0000)
        Reviewed by Maciej Stachowiak.

        Fixed http://bugs.webkit.org/show_bug.cgi?id=16909
        REGRESSION: Amazon.com crash (ActivationImp)

        (and a bunch of other crashes)

        Plus, a .7% SunSpider speedup to boot.

        Replaced the buggy currentExec and savedExec mechanisms with an
        explicit ExecState stack.

        * kjs/collector.cpp:
        (KJS::Collector::collect): Explicitly mark the ExecState stack.

        (KJS::Collector::reportOutOfMemoryToAllExecStates): Slight change in
        behavior: We no longer throw an exception in any global ExecStates,
        since global ExecStates are more like pseudo-ExecStates, and aren't
        used for script execution. (It's unclear what would happen if you left
        an exception waiting around in a global ExecState, but it probably
        wouldn't be good.)

WebCore:

        Reviewed by Maciej Stachowiak.

        Adapted WebCore to the fix for http://bugs.webkit.org/show_bug.cgi?id=16909
        REGRESSION: Amazon.com crash (ActivationImp)

        * bindings/js/kjs_proxy.cpp:
        (WebCore::KJSProxy::~KJSProxy): No convenient way to make this assertion
        anymore. (It wasn't firing for anyone, anyway, so it's no big loss.)

        * bindings/objc/WebScriptObject.mm:
        (+[WebScriptObject throwException:]): Use the ExecState stack, instead
        of currentExec.
        (-[WebScriptObject setException:]): ditto. Also, a slight change in
        behavior: If no ExecStates are active, we no longer throw an exception
        in the global ExecState. The JavaScriptCore ChangeLog explains why.
        This also matches the behavior of +throwException.

LayoutTests:

        Layout test for http://bugs.webkit.org/show_bug.cgi?id=16909
        REGRESSION: Amazon.com crash (ActivationImp)

        * fast/js/exec-state-marking-expected.txt: Added.
        * fast/js/exec-state-marking.html: Added.

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

14 files changed:
JavaScriptCore/ChangeLog
JavaScriptCore/JavaScriptCore.exp
JavaScriptCore/kjs/ExecState.cpp
JavaScriptCore/kjs/ExecState.h
JavaScriptCore/kjs/JSGlobalObject.cpp
JavaScriptCore/kjs/JSGlobalObject.h
JavaScriptCore/kjs/collector.cpp
LayoutTests/ChangeLog
LayoutTests/fast/js/exec-state-marking-expected.txt [new file with mode: 0644]
LayoutTests/fast/js/exec-state-marking.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/WebCore.xcodeproj/project.pbxproj
WebCore/bindings/js/kjs_proxy.cpp
WebCore/bindings/objc/WebScriptObject.mm

index b2f6a54cd11040367d175a95973ef8439237b86d..097937729f3d037be2c77e975cac168a36c9a174 100644 (file)
@@ -1,3 +1,27 @@
+2008-01-21  Geoffrey Garen  <ggaren@apple.com>
+
+        Reviewed by Maciej Stachowiak.
+        
+        Fixed http://bugs.webkit.org/show_bug.cgi?id=16909
+        REGRESSION: Amazon.com crash (ActivationImp)
+        
+        (and a bunch of other crashes)
+        
+        Plus, a .7% SunSpider speedup to boot.
+        
+        Replaced the buggy currentExec and savedExec mechanisms with an
+        explicit ExecState stack.
+
+        * kjs/collector.cpp:
+        (KJS::Collector::collect): Explicitly mark the ExecState stack.
+
+        (KJS::Collector::reportOutOfMemoryToAllExecStates): Slight change in
+        behavior: We no longer throw an exception in any global ExecStates,
+        since global ExecStates are more like pseudo-ExecStates, and aren't
+        used for script execution. (It's unclear what would happen if you left
+        an exception waiting around in a global ExecState, but it probably
+        wouldn't be good.)
+
 2008-01-21  Jan Michael Alonzo  <jmalonzo@unpluggable.com>
 
         Reviewed by Alp Toker.
index b587b71cf8e68710bd112f9979557194a3fd1c17..e7d5d5f63810908a3d6888abd5694feee5595c55 100644 (file)
@@ -232,6 +232,8 @@ __ZN3KJS9Collector4sizeEv
 __ZN3KJS9Collector7collectEv
 __ZN3KJS9Collector7protectEPNS_7JSValueE
 __ZN3KJS9Collector9unprotectEPNS_7JSValueE
+__ZN3KJS9ExecState16activeExecStatesEv
+__ZN3KJS9ExecStateC1EPNS_14JSGlobalObjectE
 __ZN3KJS9ExecStateC1EPNS_14JSGlobalObjectEPNS_8JSObjectEPNS_11ProgramNodeE
 __ZN3KJS9ExecStateD1Ev
 __ZN3KJSeqERKNS_7UStringEPKc
index 942da8815d3810fe5dbc83f6566cae5d44f85dbf..d9471a726737a531ffa809f8ab66bc104807aab4 100644 (file)
@@ -41,13 +41,33 @@ static inline List* globalEmptyList()
 
 // ECMA 10.2
 
+// The constructor for the globalExec pseudo-ExecState
+ExecState::ExecState(JSGlobalObject* globalObject)
+    : m_globalObject(globalObject)
+    , m_exception(0)
+    , m_propertyNames(CommonIdentifiers::shared())
+    , m_emptyList(globalEmptyList())
+    , m_callingExec(0)
+    , m_scopeNode(0)
+    , m_function(0)
+    , m_arguments(0)
+    , m_activation(0)
+    , m_localStorage(&globalObject->localStorage())
+    , m_variableObject(globalObject)
+    , m_thisVal(globalObject)
+    , m_iterationDepth(0)
+    , m_switchDepth(0) 
+    , m_codeType(GlobalCode)
+{
+    m_scopeChain.push(globalObject);
+}
+
 ExecState::ExecState(JSGlobalObject* globalObject, JSObject* /*thisObject*/, ProgramNode* programNode)
     : m_globalObject(globalObject)
     , m_exception(0)
     , m_propertyNames(CommonIdentifiers::shared())
     , m_emptyList(globalEmptyList())
     , m_callingExec(0)
-    , m_savedExec(0)
     , m_scopeNode(programNode)
     , m_function(0)
     , m_arguments(0)
@@ -62,9 +82,9 @@ ExecState::ExecState(JSGlobalObject* globalObject, JSObject* /*thisObject*/, Pro
     // FIXME: This function ignores the "thisObject" parameter, which means that the API for evaluating
     // a script with a this object that's not the same as the global object is broken, and probably
     // has been for some time.
+    ASSERT(m_scopeNode);
+    activeExecStates().append(this);
     m_scopeChain.push(globalObject);
-    if (programNode)
-        globalObject->setCurrentExec(this);
 }
 
 ExecState::ExecState(JSGlobalObject* globalObject, EvalNode* evalNode, ExecState* callingExec)
@@ -73,7 +93,6 @@ ExecState::ExecState(JSGlobalObject* globalObject, EvalNode* evalNode, ExecState
     , m_propertyNames(callingExec->m_propertyNames)
     , m_emptyList(callingExec->m_emptyList)
     , m_callingExec(callingExec)
-    , m_savedExec(globalObject->currentExec())
     , m_scopeNode(evalNode)
     , m_function(0)
     , m_arguments(0)
@@ -86,7 +105,8 @@ ExecState::ExecState(JSGlobalObject* globalObject, EvalNode* evalNode, ExecState
     , m_switchDepth(0) 
     , m_codeType(EvalCode)
 {    
-    globalObject->setCurrentExec(this);
+    ASSERT(m_scopeNode);
+    activeExecStates().append(this);
 }
 
 ExecState::ExecState(JSGlobalObject* globalObject, JSObject* thisObject, 
@@ -97,7 +117,6 @@ ExecState::ExecState(JSGlobalObject* globalObject, JSObject* thisObject,
     , m_propertyNames(callingExec->m_propertyNames)
     , m_emptyList(callingExec->m_emptyList)
     , m_callingExec(callingExec)
-    , m_savedExec(globalObject->currentExec())
     , m_scopeNode(functionBodyNode)
     , m_function(func)
     , m_arguments(&args)
@@ -107,30 +126,24 @@ ExecState::ExecState(JSGlobalObject* globalObject, JSObject* thisObject,
     , m_switchDepth(0) 
     , m_codeType(FunctionCode)
 {
+    ASSERT(m_scopeNode);
+    activeExecStates().append(this);
+
     ActivationImp* activation = globalObject->pushActivation(this);
     m_activation = activation;
     m_localStorage = &activation->localStorage();
     m_variableObject = activation;
     m_scopeChain.push(activation);
-    globalObject->setCurrentExec(this);
 }
 
 ExecState::~ExecState()
 {
-    if (m_codeType == FunctionCode && m_activation->needsPop())
-        m_globalObject->popActivation();
-    
-    m_globalObject->setCurrentExec(m_savedExec);
-}
-
-void ExecState::mark()
-{
-    for (ExecState* exec = this; exec; exec = exec->m_callingExec) {
-        exec->m_scopeChain.mark();
+    ASSERT(m_scopeNode && activeExecStates().last() == this || !m_scopeNode);
+    if (m_scopeNode)
+        activeExecStates().removeLast();
 
-        if (exec->m_savedExec != exec->m_callingExec && exec->m_savedExec)
-            exec->m_savedExec->mark();
-    }
+    if (m_activation && m_activation->needsPop())
+        m_globalObject->popActivation();
 }
 
 JSGlobalObject* ExecState::lexicalGlobalObject() const
@@ -145,4 +158,17 @@ JSGlobalObject* ExecState::lexicalGlobalObject() const
     return dynamicGlobalObject();
 }
 
+void ExecState::markActiveExecStates() 
+{
+    ExecStateStack::const_iterator end = activeExecStates().end();
+    for (ExecStateStack::const_iterator it = activeExecStates().begin(); it != end; ++it)
+        (*it)->m_scopeChain.mark();
+}
+
+ExecStateStack& ExecState::activeExecStates()
+{
+    static ExecStateStack staticActiveExecStates;
+    return staticActiveExecStates;
+}
+
 } // namespace KJS
index 911d7f2e24fa2330347215eb265b2f244430c208..6efc74b2a2676a90bcb6044739abee6c8ea09d7b 100644 (file)
@@ -51,6 +51,8 @@ namespace KJS  {
     class ScopeNode;
     struct LocalStorageEntry;
     
+    typedef Vector<ExecState*, 16> ExecStateStack;
+
     /**
      * Represents the current state of script execution. This is
      * passed as the first argument to most functions.
@@ -85,7 +87,6 @@ namespace KJS  {
         JSObject* thisValue() const { return m_thisVal; }
         
         ExecState* callingExecState() { return m_callingExec; }
-        ExecState* savedExec() { return m_savedExec; }
         
         ActivationImp* activationObject() { return m_activation; }
         void setActivationObject(ActivationImp* a) { m_activation = a; }
@@ -106,8 +107,6 @@ namespace KJS  {
         void popSwitch() { m_switchDepth--; }
         bool inSwitch() const { return (m_switchDepth > 0); }
 
-        void mark();
-        
         // These pointers are used to avoid accessing global variables for these,
         // to avoid taking PIC branches in Mach-O binaries.
         const CommonIdentifiers& propertyNames() const { return *m_propertyNames; }
@@ -178,12 +177,16 @@ namespace KJS  {
             return 0;
         }
 
+        ExecState(JSGlobalObject*);
         ExecState(JSGlobalObject*, JSObject* thisObject, ProgramNode*);
         ExecState(JSGlobalObject*, EvalNode*, ExecState* callingExecState);
         ExecState(JSGlobalObject*, JSObject* thisObject, FunctionBodyNode*,
             ExecState* callingExecState, FunctionImp*, const List& args);
         ~ExecState();
 
+        static void markActiveExecStates();
+        static ExecStateStack& activeExecStates();
+
     private:
         // ExecStates are always stack-allocated, and the garbage collector
         // marks the stack, so we don't need to protect the objects below from GC.
@@ -194,7 +197,7 @@ namespace KJS  {
         const List* m_emptyList;
 
         ExecState* m_callingExec;
-        ExecState* m_savedExec;
+
         ScopeNode* m_scopeNode;
         
         FunctionImp* m_function;
index bb23a7e03737ca14d31bb685ddfdc27f671ccd6a..ff028530303d4364e2d008019b2c4100f73f1896 100644 (file)
@@ -135,7 +135,6 @@ void JSGlobalObject::init()
     d()->timeoutTime = 0;
     d()->timeoutCheckCount = 0;
 
-    d()->currentExec = 0;
     d()->recursion = 0;
     d()->debugger = 0;
     
@@ -472,9 +471,6 @@ void JSGlobalObject::mark()
 {
     JSVariableObject::mark();
 
-    if (d()->currentExec)
-        d()->currentExec->mark();
-
     markIfNeeded(d()->globalExec.exception());
 
     markIfNeeded(d()->objectConstructor);
index c27e529f9b77d296a4e523af30480fd862188d58..ec79ae29ad6e1b945b53cefccd3f74a6b907a3f0 100644 (file)
@@ -76,7 +76,7 @@ namespace KJS {
         struct JSGlobalObjectData : public JSVariableObjectData {
             JSGlobalObjectData(JSGlobalObject* globalObject)
                 : JSVariableObjectData(&inlineSymbolTable)
-                , globalExec(globalObject, globalObject, 0)
+                , globalExec(globalObject)
             {
             }
 
@@ -87,7 +87,6 @@ namespace KJS {
             CompatMode compatMode;
             
             ExecState globalExec;
-            ExecState* currentExec;
             int recursion;
 
             unsigned timeoutTime;
@@ -211,9 +210,6 @@ namespace KJS {
         Debugger* debugger() const { return d()->debugger; }
         void setDebugger(Debugger* debugger) { d()->debugger = debugger; }
 
-        void setCurrentExec(ExecState* exec) { d()->currentExec = exec; }
-        ExecState* currentExec() const { return d()->currentExec; }
-
         // FIXME: Let's just pick one compatible behavior and go with it.
         void setCompatMode(CompatMode mode) { d()->compatMode = mode; }
         CompatMode compatMode() const { return d()->compatMode; }
index fbb840be32295ee5b4592beef3217b6811c91455..e72541cffb23d14919cb372cb1035ba702adb9fc 100644 (file)
@@ -952,6 +952,7 @@ bool Collector::collect()
 
   markStackObjectsConservatively();
   markProtectedObjects();
+  ExecState::markActiveExecStates();
   List::markProtectedLists();
 #if USE(MULTIPLE_THREADS)
   if (!currentThreadIsMainThread)
@@ -1066,15 +1067,10 @@ bool Collector::isBusy()
 
 void Collector::reportOutOfMemoryToAllExecStates()
 {
-    JSGlobalObject* o = JSGlobalObject::head();
-    if (!o)
-        return;
-    
-    do {
-        ExecState* exec = o->currentExec() ? o->currentExec() : o->globalExec();
-        exec->setException(Error::create(exec, GeneralError, "Out of memory"));
-        o = o->next();
-    } while(o != JSGlobalObject::head());
+    ExecStateStack::const_iterator end = ExecState::activeExecStates().end();
+    for (ExecStateStack::const_iterator it = ExecState::activeExecStates().begin(); it != end; ++it) {
+        (*it)->setException(Error::create(*it, GeneralError, "Out of memory"));
+    }
 }
 
 } // namespace KJS
index 5324a0a02d67621d8189317a319c236b6f2e2fa3..0eca7d16d1abf19b9da3e55e08668b5afa872c15 100644 (file)
@@ -1,3 +1,11 @@
+2008-01-21  Geoffrey Garen  <ggaren@apple.com>
+
+        Layout test for http://bugs.webkit.org/show_bug.cgi?id=16909
+        REGRESSION: Amazon.com crash (ActivationImp)
+
+        * fast/js/exec-state-marking-expected.txt: Added.
+        * fast/js/exec-state-marking.html: Added.
+
 2008-01-21  Nikolas Zimmermann  <zimmermann@kde.org>
 
         Reviewed by Eric.
diff --git a/LayoutTests/fast/js/exec-state-marking-expected.txt b/LayoutTests/fast/js/exec-state-marking-expected.txt
new file mode 100644 (file)
index 0000000..e7ee859
--- /dev/null
@@ -0,0 +1,5 @@
+This page tests for a crash due to insufficient marking of ExecStates. If the test passes, you'll see a series of PASS messages below.
+
+PASS: You didn't crash.
+PASS: f() should be 12345 and is.
+
diff --git a/LayoutTests/fast/js/exec-state-marking.html b/LayoutTests/fast/js/exec-state-marking.html
new file mode 100644 (file)
index 0000000..e801336
--- /dev/null
@@ -0,0 +1,98 @@
+<p>
+This page tests for a crash due to insufficient marking of ExecStates. If
+the test passes, you'll see a series of PASS messages below.
+</p>
+
+<pre id="console">
+PASS: You didn't crash.
+</pre>
+
+<a id="a"></a>
+
+<script>
+function log(s)
+{
+    document.getElementById("console").appendChild(document.createTextNode(s + "\n"));
+}
+
+function shouldBe(a, aDescription, b)
+{
+    if (a == b) {
+        log("PASS: " + aDescription + " should be " + b + " and is.");
+    } else {
+        log("FAIL: " + aDescription + " should be " + b + " but instead is " + a + ".");
+    }
+}
+
+function FailureObject()
+{
+}
+
+FailureObject.prototype.toString = function()
+{
+    return "FAIL"; // A marker to indicate that an object was collected and then overwritten by a FailureObject.
+}
+
+function gc()
+{
+    if (window.GCController)
+        GCController.collect();
+    else
+        for (var i = 0; i < 10000; ++i) // Allocate a sufficient number of objects to force a GC.
+            new FailureObject;
+}
+
+function gc1()
+{
+    var script = document.createElement("script");
+    script.appendChild(document.createTextNode("gc()"));
+    document.body.appendChild(script);
+}
+
+function gc2()
+{
+    var a = document.getElementById("a");
+    a.href = "javascript:gc()";
+    
+    var event = document.createEvent("MouseEvent");
+    event.initMouseEvent("click", true, true, document.defaultView, 1, 0, 0, 0, 0, false, false, false, false, 0, document);
+    a.dispatchEvent(event);
+}
+
+function gc3()
+{
+    document.write("<" + "script" + "/>" + "gc()" + "</" + "script" + ">");
+}
+
+function f()
+{
+    // Allocate some activation data.
+    var v1 = "1";
+    var v2 = "2";
+    var v3 = "3";
+    var v4 = "4";
+    var v5 = "5";
+
+    // Globally evaluate a script that forces GC, using a few different mechanisms.
+    gc1();
+    gc2();
+    gc3();
+
+    /*
+     * Some other ways to globally evaluate a script, which aren't tested here:
+     * NPN_Evaluate
+     * -[WebScriptObject evaluateWebScript:]
+     * JSEvaluateScript
+     * JavaJSObject::eval
+     * javascript: URLs for elements other than <a>
+     */
+
+    // Now verify that our activation data wasn't collected, by using it in an interesting way.
+    return v1 + v2 + v3 + v4 + v5;
+}
+
+if (window.layoutTestController)
+    layoutTestController.dumpAsText();
+
+shouldBe(f(), "f()", "12345");
+</script>
index eda1742ac367ca2fed111a0e7df462d99be7f05d..f5da0d71079f01ad682215a840fcc9655df400c9 100644 (file)
@@ -1,3 +1,22 @@
+2008-01-21  Geoffrey Garen  <ggaren@apple.com>
+
+        Reviewed by Maciej Stachowiak.
+
+        Adapted WebCore to the fix for http://bugs.webkit.org/show_bug.cgi?id=16909
+        REGRESSION: Amazon.com crash (ActivationImp)
+
+        * bindings/js/kjs_proxy.cpp:
+        (WebCore::KJSProxy::~KJSProxy): No convenient way to make this assertion
+        anymore. (It wasn't firing for anyone, anyway, so it's no big loss.)
+
+        * bindings/objc/WebScriptObject.mm:
+        (+[WebScriptObject throwException:]): Use the ExecState stack, instead
+        of currentExec.
+        (-[WebScriptObject setException:]): ditto. Also, a slight change in
+        behavior: If no ExecStates are active, we no longer throw an exception
+        in the global ExecState. The JavaScriptCore ChangeLog explains why.
+        This also matches the behavior of +throwException.
+
 2008-01-21  Nikolas Zimmermann  <zimmermann@kde.org>
 
         Not reviewed. Try to fix Qt build, after the rmdir() fixes.c
index 259126272769d0c7006287234b58123a73e61d05..e93fed5e906efbf28c49f6581726330154d7470d 100644 (file)
                0867D690FE84028FC02AAC07 /* Project object */ = {
                        isa = PBXProject;
                        buildConfigurationList = 149C284308902B11008A9EFC /* Build configuration list for PBXProject "WebCore" */;
+                       compatibilityVersion = "Xcode 2.4";
                        hasScannedForEncodings = 1;
                        knownRegions = (
                                English,
index af559739c3fd92a037a20b4200ccd5c8a777cfc3..6925e18966005fc7898366b7278bc59a9c08b07b 100644 (file)
@@ -54,9 +54,6 @@ KJSProxy::KJSProxy(Frame* frame)
 
 KJSProxy::~KJSProxy()
 {
-    // Check for <rdar://problem/4876466>. In theory, no JS should be executing now.
-    ASSERT(!m_globalObject || !m_globalObject->currentExec());
-    
     if (m_globalObject) {
         m_globalObject = 0;
     
index d9fbaf9e01f567b85ba20dcebd5687bf05e54a1e..f231d29d4adea9b12863a988dbd623bef91aa13f 100644 (file)
@@ -250,24 +250,15 @@ static void _didExecute(WebScriptObject *obj)
 {
     JSLock lock;
     
-    JSGlobalObject* head = JSGlobalObject::head();
-
     // This code assumes that we only ever have one running interpreter.  A
     // good assumption for now, as we depend on that elsewhere.  However,
     // in the future we may have the ability to run multiple interpreters,
     // in which case this will have to change.
-    JSGlobalObject* o = head;
-    do {
-        if (!o)
-            return NO;
-
-        // If the interpreter has a current exec state, we set the exception.
-        if (ExecState* exec = o->currentExec()) {
-            throwError(exec, GeneralError, exceptionMessage);
-            return YES;
-        }
-        o = o->next();
-    } while (o != head);
+    
+    if (ExecState::activeExecStates().size()) {
+        throwError(ExecState::activeExecStates().last(), GeneralError, exceptionMessage);
+        return YES;
+    }
     
     return NO;
 }
@@ -495,11 +486,16 @@ static void getListFromNSArray(ExecState *exec, NSArray *array, RootObject* root
         return;
 
     JSLock lock;
-    
-    if (ExecState* exec = [self _rootObject]->globalObject()->currentExec()) {
+
+    ExecState* exec = 0;
+    JSObject* globalObject = [self _rootObject]->globalObject();
+    ExecStateStack::const_iterator end = ExecState::activeExecStates().end();
+    for (ExecStateStack::const_iterator it = ExecState::activeExecStates().begin(); it != end; ++it)
+        if ((*it)->dynamicGlobalObject() == globalObject)
+            exec = *it;
+            
+    if (exec)
         throwError(exec, GeneralError, description);
-    } else
-        throwError([self _rootObject]->globalObject()->globalExec(), GeneralError, description);
 }
 
 - (JSObjectRef)JSObject