559006 - Wrong behavior with JSClass::convert hook and JIT

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[spm.devel]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ru; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3 (.NET CLR 3.5.30729)
Build Identifier: 

/*
HERE IS sample code, that reproduce the bug.
just compile and run it
I've commented important places

MAIN PROBLEM IS: when executing such script multiple times:
-------------
function main()
{
	while(1)
	{
		return 0 + createMyObject();
	}
};
-------------
one time, you'll get "0[object MyClass]" instead of expected 123

problem is coupled with use of JSOPTION_JIT
if you switch off it - all is ok
*/
#include "jsapi.h"

#include <string>
#include <iostream>

static JSClass global_class =
{
	"global", JSCLASS_GLOBAL_FLAGS,
	JS_PropertyStub, JS_PropertyStub, JS_PropertyStub, JS_PropertyStub,
	JS_EnumerateStub, JS_ResolveStub, JS_ConvertStub, JS_FinalizeStub,
	JSCLASS_NO_OPTIONAL_MEMBERS
};

void reportError(JSContext *cx, const char *message, JSErrorReport *report)
{
	fprintf(stderr, "%s:%u:%s\n",
		report->filename ? report->filename : "<no filename>",
		(unsigned int) report->lineno,
		message);
}

// conversion function for MyClass
// always returns NUMBER: 123.45
JSBool my_convert(JSContext* context, JSObject* obj, JSType type, jsval* rval)
{
	if (type == JSTYPE_VOID || type == JSTYPE_STRING || type == JSTYPE_NUMBER || type == JSTYPE_BOOLEAN) 
	{
		return JS_NewNumberValue(context, 123, rval);
	}

	return JS_FALSE;
}

JSBool my_construct(JSContext* context, JSObject* obj, uintN argc, jsval* argv, jsval* rval)
{
	*rval = OBJECT_TO_JSVAL(obj);
	return JS_TRUE;
}

static JSClass myClass =
{ 
	"MyClass",
	0,
	JS_PropertyStub,
	JS_PropertyStub,
	JS_PropertyStub,
	JS_PropertyStub,
	JS_EnumerateStub, 
	JS_ResolveStub,   
	my_convert,   
	JS_FinalizeStub,
	JSCLASS_NO_OPTIONAL_MEMBERS
};

static JSBool createMyObject(JSContext* context, JSObject* obj, uintN argc, jsval *argv, jsval* rval) 
{
	JS_BeginRequest(context);

	//JS_GC(context); //<- if we make GC here, all is ok

	JSObject* myObject = JS_NewObject(context, &myClass, NULL, NULL);
	*rval = OBJECT_TO_JSVAL(myObject); 

	JS_EndRequest(context);

	return JS_TRUE;
}

static JSFunctionSpec s_functions[] =
{
	{ "createMyObject", createMyObject, 0 },
	{ 0,0,0,0,0 }
};


////////////////////HERE IS SAMPLE SCRIPT //////////////////////////////////////////////////////
std::string getScript()
{
	return 
	"function main() \
	{ \
		while(1) \
		{ \
			return 0 + createMyObject(); \
		} \
	}";
}

// call "main()" function in JS
void callMain(JSContext* cx)
{
	jsval rval;
	int32 i;
	JS_CallFunctionName(cx, JS_GetGlobalObject(cx), "main", 0, NULL, &rval);
	
	if(JSVAL_IS_NUMBER(rval))
	{
		JS_ValueToInt32(cx, rval, &i);
		std::cout << "return="<<i<<std::endl;
	}
	else if(JSVAL_IS_STRING(rval))
	{
		// ------------------ ERROR! -------------------- ERROR! ----------------- ERROR! --------------------------
		//oops - UNEXPECTED. In script, we always return a number!
		//but now we've got a string with "0[object MyClass]"
		std::cout << "Oops! something bad happens here, we've got string instead of number:"<<std::endl;

		JSString* jstr = JS_ValueToString(cx, rval);
		if (jstr) 
		{
			char* str= JS_GetStringBytes(jstr);
			if (str) 
			{
				std::cout << "return="<<str <<std::endl;
			}
		}

	}
}

int main(int argc, const char *argv[])
{

	//just usual JS initialization
	JSRuntime* rt = JS_NewRuntime(8L * 1024L * 1024L);
	if (rt == NULL)
	{
		return 1;
	}

	JSContext* cx = JS_NewContext(rt, 8192);
	if (cx == NULL)
	{
		return 1;
	}

	JS_SetVersion(cx, JSVERSION_LATEST);
	JS_SetOptions(cx, JS_GetOptions(cx) | JSOPTION_JIT);//<- ENABLE JIT. If we disable it - all is ok

	JS_SetErrorReporter(cx, reportError);

	JS_BeginRequest(cx);

	JSObject* global = JS_NewObject(cx, &global_class, NULL, NULL);
	if (global == NULL)
	{
		return 1;
	}
	if (!JS_InitStandardClasses(cx, global))
	{
		return 1;
	}

	JS_InitClass(cx, JS_GetGlobalObject(cx), NULL,
		&myClass,
		my_construct, 
		0, 0, 0, 0, 0);

	JS_DefineFunctions(cx, JS_GetGlobalObject(cx), s_functions);

	// -------- HERE IS MY CODE ----------------
	std::string script = getScript();

	jsval rval;
	JS_EvaluateScript(cx, JS_GetGlobalObject(cx), script.c_str(), script.length(), NULL,  0, &rval);

	//problem occur on third (sometimes second) call
	//but it depends on stack size (or smth else) i think. So may be you need more calls to reproduce

	//first call - ok
	callMain(cx);

	//second call - ok
	callMain(cx);

	//third call - ERROR!
	callMain(cx);// <-- here is the problem

	// -------- END OF MY CODE ----------------

	JS_EndRequest(cx);

	JS_DestroyContext(cx);
	JS_DestroyRuntime(rt);
	JS_ShutDown();

	return 0;
}

Reproducible: Always

Steps to Reproduce:
see sample code
Actual Results:  
sample script sometimes returns "0[object MyClass]"

Expected Results:  
sample script should always return 123

The bug had appeared after the migration to new version of SpiderMonkey on Jan 25,2010

compiler - MSVC,2008

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

I decided where to put the calls to guardNativeConversion by looking at which imacros call .valueOf methods.

No measurable loss on V8 or SunSpider. I hope this is because my comment about the shape guard being free is true; I haven't actually looked at the LIR and confirmed this yet.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 438896 [details] [diff] [review]
v1

I guess the other option would be to call a builtin to call the convert hook on trace? I don't think it's worth writing the code until proven it's needed, though.

Comment 3

[Brendan Eich [:brendan]]

Good to prove the shape guard commoning works.

Curse that JSClass.convert hook! I blame liveconnect (again).

/be

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: before/after differences

To produce this diff, I did:

TMFLAGS=full d-objdir/js -j -e '(function(a){for(;;)+a;})({})' > spew1.txt
(type ctrl-c to stop the infinite loop)

hg qpush
(build)
TMFLAGS=full d-objdir/js -j -e '(function(a){for(;;)+a;})({})' > spew2.txt
(type ctrl-c to stop the infinite loop)

perl -pi.bak -e 's/0x[0-9a-fA-F]+/0x####/g;' spew[12].txt
perl -pi.bak -e 's/000[0-9a-fA-F]+/000#######/g;' spew[12].txt
perl -pi.bak -e 's/\b[0-9]{9}\b/#########/g;' spew[12].txt
perl -pi.bak -e 's/\b[0-9]{10}\b/##########/g;' spew[12].txt

diff -U8 spew1.txt spew2.txt > bug-559006-spew.txt

All that changes is that if first shape guard fails, we go to an earlier side exit (before entering the imacro!) than the rest of the shape guards; this is precisely the fix we want for the bug at hand.

(Both before and after my patch, we have to record this loop twice. That's because the first time we record the shape guard for Object.prototype, it isn't branded yet. Then we call Object.prototype.toString. I'll file that as a separate bug if we care--it can only happen once.)

Comment 5

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/47949ff2d058

...and the file that was missing from that one:
http://hg.mozilla.org/tracemonkey/rev/a9e269df21ad

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #4)
> (Both before and after my patch, we have to record this loop twice. That's
> because the first time we record the shape guard for Object.prototype, it isn't
> branded yet. Then we call Object.prototype.toString. I'll file that as a
> separate bug if we care--it can only happen once.)

Yes please -- we pre-brand in TR::hasMethod, and the recorder should do that in more cases than just __iterator__ (the comment is too specific in that method). Good one!

/be

Comment 7

[Jason Orendorff [:jorendorff]]

Filed bug 559584.

Comment 8

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/47949ff2d058
http://hg.mozilla.org/mozilla-central/rev/a9e269df21ad