Trying to repro something else, I visited the URL in this crash report: https://crash-stats.mozilla.com/report/index/7b37a69f-77b8-462b-9c09-899d42120201. This hit an interesting assert: JS_ASSERT(!target->isCachedEval) that Igor added. The stack seems to be pretty informative: #1 CrashInJS () #2 JS_Assert #3 JS_EnterCrossCompartmentCallScript js/src/jsapi.cpp:1393 #4 jsd_GetClosestPC js/jsd/jsd_scpt.c:544 #5 JSD_GetClosestPC js/jsd/jsdebug.c:348 #6 jsdScript::jsdScript js/jsd/jsd_xpc.cpp:1009 #7 jsdScript::FromPtr js/jsd/jsd_xpc.h:155 #8 jsds_ScriptHookProc js/jsd/jsd_xpc.cpp:733 #9 jsd_NewScriptHookProc js/jsd/jsd_scpt.c:701 #10 js_CallNewScriptHook js/src/jsscript.cpp:1314 #11 EvalScriptGuard::lookupInEvalCache js/src/jsobj.cpp:1018 #12 EvalKernel js/src/jsobj.cpp:1168 #13 js::DirectEval js/src/jsobj.cpp:1255 #14 js::Interpret js/src/jsinterp.cpp:2746 #15 js::RunScript js/src/jsinterp.cpp:454 #16 js::ExecuteKernel js/src/jsinterp.cpp:657 #17 js::Execute js/src/jsinterp.cpp:698 #18 EvaluateUCScriptForPrincipalsCommon js/src/jsapi.cpp:5339 #19 JS_EvaluateUCScriptForPrincipalsVersionOrigin js/src/jsapi.cpp:5376 #20 nsJSContext::EvaluateString dom/base/nsJSEnvironment.cpp:1507 #21 nsScriptLoader::EvaluateScript content/base/src/nsScriptLoader.cpp:903 #22 nsScriptLoader::ProcessRequest content/base/src/nsScriptLoader.cpp:796 #23 nsScriptLoader::ProcessPendingRequests content/base/src/nsScriptLoader.cpp:963 #24 OnStreamComplete content/base/src/nsScriptLoader.cpp:1182 ... etc until main
Created attachment 600214 [details] [diff] [review] Allow entering a compartment with a cached eval script I randomly stumbled across this too (also trying to repo something else). Igor, what's the reason for the assert? It seems like you're avoiding using an eval-cached script's global object, which is reasonable, except that JSD really would like to enter such a script's compartment. And in that case, it seems like the dummy global object creation would be fine. Here, I'll put the question in the form of a patch.
(In reply to Steve Fink [:sfink] from comment #1) > I randomly stumbled across this too (also trying to repo something else). > Igor, what's the reason for the assert? eval script's should not leak under normal circumstances outside the engine and the assert helps to ensure this. Now, the debugger is really a special case and we should support it properly. However, I am not the right person to judge if is OK just to create a dummy global. CC/redirecting to Jason.
Comment on attachment 600214 [details] [diff] [review] Allow entering a compartment with a cached eval script I think getGlobalObjectOrNull() is incorrect; it should also return NULL if the script isActiveEval (in that case, the globalObject field has a don't-care value, possibly left over from when it was isCachedEval). Otherwise this is fine. I wouldn't mind seeing a test. But presumably compartment-per-global will be getting rid of this code very soon. (I tried writing one and couldn't immediately get the bug to trigger.)
Created attachment 616343 [details] [diff] [review] isEvalCached assertion test Here's a test, using the xpcshell -d thing I went to great effort to add and then never really used. It fails before the change, passes after. Well, ok, it actually fails no matter what, because the xpcshell -d cleanup is busted and triggers assertions. I'm still trying to figure out how to do that without triggering a leak instead. How are services supposed to be shut down, anyway?
Created attachment 616344 [details] [diff] [review] jsd/xpcshell -d fixes Just FYI, this is a WIP patch that more or less resolves the shutdown issues with xpcshell -d. But I don't think it's right.
Created attachment 616348 [details] [diff] [review] jsd/xpcshell -d fixes Oops, I had fragments in a separate place. Folded.
Comment on attachment 616348 [details] [diff] [review] jsd/xpcshell -d fixes Obsoleted by bug 751398 - the need to shutdown debug mode was a side effect of creating cycles without telling the cycle collector.