Last Comment Bug 723712 - JS_ASSERT(!target->isCachedEval) with Firebug
: JS_ASSERT(!target->isCachedEval) with Firebug
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: general
:
Mentors:
Depends on: 751398
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-02 14:08 PST by Luke Wagner [:luke]
Modified: 2012-05-16 03:34 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Allow entering a compartment with a cached eval script (965 bytes, patch)
2012-02-23 15:44 PST, Steve Fink [:sfink] [:s:]
jorendorff: review+
Details | Diff | Review
isEvalCached assertion test (1.34 KB, patch)
2012-04-18 16:33 PDT, Steve Fink [:sfink] [:s:]
no flags Details | Diff | Review
jsd/xpcshell -d fixes (6.33 KB, patch)
2012-04-18 16:35 PDT, Steve Fink [:sfink] [:s:]
no flags Details | Diff | Review
jsd/xpcshell -d fixes (6.36 KB, patch)
2012-04-18 16:38 PDT, Steve Fink [:sfink] [:s:]
no flags Details | Diff | Review

Description Luke Wagner [:luke] 2012-02-02 14:08:24 PST
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:] 2012-02-23 15:44:59 PST
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.
Comment 2 Igor Bukanov 2012-02-24 00:32:29 PST
(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] 2012-03-30 07:54:56 PDT
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:] 2012-04-18 16:33:49 PDT
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?
Comment 5 Steve Fink [:sfink] [:s:] 2012-04-18 16:35:06 PDT
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.
Comment 6 Steve Fink [:sfink] [:s:] 2012-04-18 16:38:18 PDT
Created attachment 616348 [details] [diff] [review]
jsd/xpcshell -d fixes

Oops, I had fragments in a separate place. Folded.
Comment 7 Steve Fink [:sfink] [:s:] 2012-05-15 12:41:48 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.