723712 - JS_ASSERT(!target->isCachedEval) with Firebug

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

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

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: 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.

Comment 2

[Igor Bukanov]

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

[Jason Orendorff [: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.)

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: 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?

Comment 5

[Steve Fink [:sfink] [:s:]]

Attachment: 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.

Comment 6

[Steve Fink [:sfink] [:s:]]

Attachment: jsd/xpcshell -d fixes

Oops, I had fragments in a separate place. Folded.

Comment 7

[Steve Fink [:sfink] [:s:]]

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.

Comment 8

[Steve Fink [:sfink] [:s:]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7765bdafab86
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d37395000a7

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4d37395000a7
https://hg.mozilla.org/mozilla-central/rev/7765bdafab86