Bug 23085 - [jsfunfuzz] Over released ScopeChainNode
Summary: [jsfunfuzz] Over released ScopeChainNode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P1 Normal
Assignee: Oliver Hunt
URL:
Keywords: InRadar
Depends on:
Blocks: 13638
  Show dependency treegraph
 
Reported: 2009-01-02 21:26 PST by Oliver Hunt
Modified: 2009-01-06 12:34 PST (History)
4 users (show)

See Also:


Attachments
Remove use of ScopeChain for the scope chain unwinding (4.27 KB, patch)
2009-01-06 12:25 PST, Oliver Hunt
zwarich: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2009-01-02 21:26:50 PST
(function(){
    try{
        with({})
            throw this(function(){})
    } catch(x) {
    }
})()
gc()
Comment 1 Oliver Hunt 2009-01-02 23:27:16 PST
erk, it turns out this test case doesn't reproduce with all of jsfunfuzz included before it, however i've found another reduction like thing that produces the same crash
Comment 2 Oliver Hunt 2009-01-02 23:41:51 PST
Okay, here we go:
function tryRunning(f){
    try{
        f()
    }catch(r){
    }
}
function tryItOut(){
    function f() {
        try {
            throw "";
        } catch(y) {
            this(function(){})
        } finally {
        }
    };
    v = tryRunning(f)
}
tryItOut();
gc();

Comment 3 Gavin Barraclough 2009-01-02 23:49:36 PST
function f() {
    try {
        throw "";
    } catch(y) {
        this(function(){})
    } finally {
    }
};
try {
    f()
} catch(r) {
}
gc();

Comment 4 Gavin Barraclough 2009-01-03 00:24:40 PST
^^ release builds only :-(

Comment 5 Gavin Barraclough 2009-01-03 00:24:46 PST
try{
(function() {
    try {
        throw "";
    } catch(y) {
        throw (function(){});
    } finally {
    }
})()
}catch(r){
}
(function(){})()
gc();
Comment 6 Oliver Hunt 2009-01-03 02:20:27 PST
The problem is that a scope node is being deleted prematurely, i believe the scope node being removed in the most recent reduction is the activation for the first function.

I honestly can't work out how/why the ref counting scheme we use for scopechainnodes works, but i blame it for the badness.
Comment 7 Cameron Zwarich (cpst) 2009-01-03 02:26:39 PST
I'll take a look at this. This one hurts.
Comment 8 Oliver Hunt 2009-01-05 15:18:41 PST
<rdar://problem/6474110>
Comment 9 Oliver Hunt 2009-01-06 08:54:42 PST
Okay, so the issue is that the finally block is derefing the activation incorrectly 
Comment 10 Oliver Hunt 2009-01-06 10:05:11 PST
Scope chain unwinding creates a ScopeChain to wrap the ScopeChainNode, when the ScopeChain is destroyed it does a full deref of the top node, but it has not necessarily ref'd that node.

Basically this is the path to badness
  * Scope chain is represented as {scope object, ref count} -> next scopechainnode  
  * ScopeChainNode* scopeChain = {someScope, 1}->{activation, 1}->not relevant
  * ScopeChain sc(scopeChain) => {someScope, 2}->{activation, 1}->not relevant
  * sc.pop() => {activation, 1}->not relevant
  * sc.~ScopeChain => {activation, 0}->not relevant

So we end up leaking the top of stack, and over releasing whateer is the ToS at the end
   
Comment 11 Oliver Hunt 2009-01-06 10:08:09 PST
Ah, whooops, not actually correct, the problem is that ScopeChain refs() the origin ToS,  and then derefs() the final ToS.  but pop() and deref() have different behaviour -- deref() on a node that was not explicitly ref'd is basically wrong
Comment 12 Oliver Hunt 2009-01-06 12:25:48 PST
Created attachment 26465 [details]
Remove use of ScopeChain for the scope chain unwinding

Fixeration
Comment 13 Cameron Zwarich (cpst) 2009-01-06 12:28:03 PST
Comment on attachment 26465 [details]
Remove use of ScopeChain for the scope chain unwinding

r=me
Comment 14 Oliver Hunt 2009-01-06 12:34:20 PST
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	JavaScriptCore/ChangeLog
	M	JavaScriptCore/interpreter/Interpreter.cpp
	M	LayoutTests/ChangeLog
	M	LayoutTests/fast/js/exception-try-finally-scope-error-expected.txt
	M	LayoutTests/fast/js/resources/exception-try-finally-scope-error.js
Committed r39660