Closed Bug 723712 Opened 10 years ago Closed 10 years ago

JS_ASSERT(!target->isCachedEval) with Firebug


(Core :: JavaScript Engine, defect)

Not set





(Reporter: luke, Unassigned)




(2 files, 2 obsolete files)

Trying to repro something else, I visited the URL in this crash report:  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
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.
Attachment #600214 - Flags: review?(igor)
(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.
Attachment #600214 - Flags: review?(igor) → review?(jorendorff)
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.)
Attachment #600214 - Flags: review?(jorendorff) → review+
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?
Attached patch jsd/xpcshell -d fixes (obsolete) — Splinter Review
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.
Attached patch jsd/xpcshell -d fixes (obsolete) — Splinter Review
Oops, I had fragments in a separate place. Folded.
Attachment #616344 - Attachment is obsolete: true
Depends on: 751398
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.
Attachment #616348 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.