Bug 22903 – REGRESSION (r36267): visiting this site reliably crashes WebKit nightly

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Paul Downey]

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!

[Alexey Proskuryakov]

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)

[Alexey Proskuryakov]

<rdar://problem/6459369>

[Cameron Zwarich (cpst)]

I'll take this on.

[Cameron Zwarich (cpst)]

Created attachment 26164 [details]
Stack trace

[Cameron Zwarich (cpst)]

Strangely, this bug is caused by r36267, the introduction of the eval code cache.

[Cameron Zwarich (cpst)]

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.

[Geoffrey Garen]

I wonder if there's subtle dependency built into the eval code when it's compiled, which doesn't hold when it's reused.

[George Sherwood]

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.

[Oliver Hunt]

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?

[Oliver Hunt]

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.

[Cameron Zwarich (cpst)]

(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.

[Oliver Hunt]

Well, a patch that should have fixed the marking does not seem to have done the trick :-/

[Oliver Hunt]

:-/

As far as i can tell there must be some kind of gc error, but i cannot fathom what it is.

[Oliver Hunt]

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.

[Oliver Hunt]

Created attachment 26724 [details]
Fixeration!

\o/

[Cameron Zwarich (cpst)]

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.

[Oliver Hunt]

Comment on attachment 26724 [details]
Fixeration!

Have finally managed to make testcase that works

[Oliver Hunt]

Created attachment 26730 [details]
Fixeration mkII

Fixed with layout test

[Cameron Zwarich (cpst)]

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.

[Oliver Hunt]

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