Bug 22903

Summary: REGRESSION (r36267): visiting this site reliably crashes WebKit nightly
Product: WebKit Reporter: Paul Downey <paul.downey>
Component: JavaScriptCoreAssignee: Oliver Hunt <oliver>
Status: RESOLVED FIXED    
Severity: Normal CC: gsherwood, mjs
Priority: P1 Keywords: InRadar, NeedsReduction, Regression
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
URL: http://tiddlytools.com
Attachments:
Description Flags
Stack trace
none
Fixeration!
none
Fixeration mkII zwarich: review+

Description Paul Downey 2008-12-17 10:26:08 PST
TiddlyTools.com is a large page incoperating a large amount of JavaScript based on the TiddlyWiki framework. Visiting the site reliably crashes WebKit nightly on my machine, and has done so for several months, including the latest.

I've submitted several crash reports, but this is highly reproducible, and seems important!
Comment 1 Alexey Proskuryakov 2008-12-19 12:20:33 PST
Confirmed with a local release build r39393.

Thread 0 Crashed:
0   com.apple.JavaScriptCore      	0x006d8111 JSC::Interpreter::cti_op_eq(void*, ...) + 497 (JSCell.h:181)

Comment 2 Alexey Proskuryakov 2008-12-19 12:21:20 PST
<rdar://problem/6459369>
Comment 3 Cameron Zwarich (cpst) 2008-12-19 14:04:45 PST
I'll take this on.
Comment 4 Cameron Zwarich (cpst) 2008-12-19 15:33:50 PST
Created attachment 26164 [details]
Stack trace
Comment 5 Cameron Zwarich (cpst) 2008-12-19 23:05:46 PST
Strangely, this bug is caused by r36267, the introduction of the eval code cache.
Comment 6 Cameron Zwarich (cpst) 2008-12-20 02:44:41 PST
This is the offending JS being cached:

function _out(place){
	var s=place.lastChild.button.style;
	s.marginLeft=".3em"; s.display="block"; s.padding=".2em";

};_out(w.output);

It has 136 characters, and if I change EvalCodeCache to only cache scripts with 136 chars, it still crashes with this being the only script cached. It also isn't executing the same eval CodeBlock reentrantly, if there was some subtle problem with that.
Comment 7 Geoffrey Garen 2008-12-23 10:58:51 PST
I wonder if there's subtle dependency built into the eval code when it's compiled, which doesn't hold when it's reused.
Comment 8 George Sherwood 2009-01-02 10:44:55 PST
I am getting what might be similar crashes from some blogs such as:

www.instapundit.com

Always reproduceable with builds against libcurl and libsoup.  Seems like it has started over the past couple months.
Comment 9 Oliver Hunt 2009-01-14 08:18:20 PST
I'm wondering if the eval cache goes wrong if the eval defines a function -- eg, an activation captures the scope chain  of the eval... i wonder if this could cause something to go wrong with the scope chain?
Comment 10 Oliver Hunt 2009-01-14 10:09:13 PST
I *think* I have it.  As far as i can tell 
eval("function f(){...}; f()")

will cache code block containing a function f(), but if the function f never escapes the eval, there will be no reference to it from anywhere but the eval cache, and the eval cache does not mark cached data leading to the possibility of the function and/or its constants being collected.
Comment 11 Cameron Zwarich (cpst) 2009-01-14 10:10:42 PST
(In reply to comment #10)
> I *think* I have it.  As far as i can tell 
> eval("function f(){...}; f()")
> 
> will cache code block containing a function f(), but if the function f never
> escapes the eval, there will be no reference to it from anywhere but the eval
> cache, and the eval cache does not mark cached data leading to the possibility
> of the function and/or its constants being collected.

Let me try marking the cached CodeBlock and see if that fixes the bug.
Comment 12 Oliver Hunt 2009-01-14 10:12:50 PST
Well, a patch that should have fixed the marking does not seem to have done the trick :-/
Comment 13 Oliver Hunt 2009-01-14 10:36:31 PST
:-/

As far as i can tell there must be some kind of gc error, but i cannot fathom what it is.
Comment 14 Oliver Hunt 2009-01-14 11:17:28 PST
Okay, my first version of the patch assumed that functions were being kept on the cached codeblock, but this was not the case.

Have now corrected that assumption and everything seems to work.  I cannot for the life of me make a testcase for this and it should be trivial to do so.  grumble.
Comment 15 Oliver Hunt 2009-01-14 11:52:08 PST
Created attachment 26724 [details]
Fixeration!

\o/
Comment 16 Cameron Zwarich (cpst) 2009-01-14 12:22:26 PST
Here is a test case that crashes before Oliver's patch but works afterwards, at least with the JSC shell and COLLECT_ON_EVERY_ALLOCATION enabled:

var s = "function f() { print('foo'); } f();";

var evalString = function()
{
    return eval(s);
}

evalString();

[[[1,2,3],[1,2,3],[1,2,3]],[[1,2,3],[1,2,3]]]

print(evalString());

I'll try to make something for DRT.
Comment 17 Oliver Hunt 2009-01-14 13:41:21 PST
Comment on attachment 26724 [details]
Fixeration!

Have finally managed to make testcase that works
Comment 18 Oliver Hunt 2009-01-14 14:05:34 PST
Created attachment 26730 [details]
Fixeration mkII

Fixed with layout test
Comment 19 Cameron Zwarich (cpst) 2009-01-14 14:10:21 PST
Comment on attachment 26730 [details]
Fixeration mkII

>      // We don't need to mark our own codeblock as the JSGlobalObject takes care of that

"codeblock" should be CodeBlock. Other than that, r=me.
Comment 20 Oliver Hunt 2009-01-14 14:35:11 PST
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	JavaScriptCore/ChangeLog
	M	JavaScriptCore/bytecode/CodeBlock.cpp
	M	JavaScriptCore/bytecode/CodeBlock.h
	M	JavaScriptCore/bytecode/EvalCodeCache.h
	M	JavaScriptCore/parser/Nodes.cpp
	M	JavaScriptCore/parser/Nodes.h
	M	LayoutTests/ChangeLog
	A	LayoutTests/fast/js/cached-eval-gc-expected.txt
	A	LayoutTests/fast/js/cached-eval-gc.html
	A	LayoutTests/fast/js/resources/cached-eval-gc.js
Committed r39910