Closed Bug 559006 Opened 15 years ago Closed 15 years ago

Wrong behavior with JSClass::convert hook and JIT

Categories

(Core :: JavaScript Engine, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: spm.devel, Assigned: jorendorff)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 files)

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
Assignee: general → jorendorff
Summary: Wrong behavior with user-defined objects and JSOPTION_JIT enabled. May be js stack corruption → Wrong behavior with JSClass::convert hook and JIT
Attached patch v1Splinter Review
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.
Attachment #438896 - Flags: review?(mrbkap)
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.
Attachment #438896 - Flags: review?(mrbkap) → review+
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Good to prove the shape guard commoning works. Curse that JSClass.convert hook! I blame liveconnect (again). /be
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.)
Whiteboard: fixed-in-tracemonkey
(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
Filed bug 559584.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: