Bug 22903 - REGRESSION (r36267): visiting this site reliably crashes WebKit nightly
Summary: REGRESSION (r36267): visiting this site reliably crashes WebKit nightly
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: http://tiddlytools.com
Keywords: InRadar, NeedsReduction, Regression
Depends on:
Blocks:
 
Reported: 2008-12-17 10:26 PST by Paul Downey
Modified: 2009-01-14 14:35 PST (History)
2 users (show)

See Also:


Attachments
Stack trace (7.09 KB, text/plain)
2008-12-19 15:33 PST, Cameron Zwarich (cpst)
no flags Details
Fixeration! (5.60 KB, patch)
2009-01-14 11:52 PST, Oliver Hunt
no flags Details | Formatted Diff | Diff
Fixeration mkII (8.64 KB, patch)
2009-01-14 14:05 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 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