438633 - JSScript atoms shouldn't be subject to GC right away

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

+++ This bug was initially created as a clone of Bug #438052 +++

(The URL links to a newsgroup discussion that raised this question.)

A script created with one of the JS_Compile* functions, and its atoms, should survive at least until the script is passed to JS_DestroyScript or JS_NewScriptObject.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

v1 - Root JS_Compile'd scripts in a runtime-wide hash table.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: jstestapp.cpp - Test program for this bug and bug 438052

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 324647 [details] [diff] [review]
v1

>diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp

Any way to get the effect of diff -p and --git together?

> JS_NewScriptObject(JSContext *cx, JSScript *script)
> {
>-    JSTempValueRooter tvr;
>     JSObject *obj;
> 
>     if (!script)
>         return js_NewObject(cx, &js_ScriptClass, NULL, NULL, 0);
> 
>-    JS_PUSH_TEMP_ROOT_SCRIPT(cx, script, &tvr);
>     obj = js_NewObject(cx, &js_ScriptClass, NULL, NULL, 0);
>     if (obj) {
>         JS_SetPrivate(cx, obj, script);
>+        if (!script->object)
>+            js_UnrootScript(cx, script);
>         script->object = obj;
> #ifdef CHECK_SCRIPT_OWNER
>         script->owner = NULL;
> #endif
>     }
>-    JS_POP_TEMP_ROOT(cx, &tvr);

Let me double-check out loud: the tvr is not needed because js_NewObject can run only a last-ditch GC, which will not collect script's atoms. It won't run a full (NORMAL) GC.

>+    /* Script roots.  See js_RootScript in jsscript.cpp. */
>+    JSDHashTable        gcScriptRootsHash;

Probably not worth making this lazily, given top-level script creation and destruction -- but it could be good for Firefox, which (IIRC) always uses JS_NewScriptObject. Measure basic stats on this table's population in a current build?

>+js_RootScript(JSContext *cx, JSScript *script)
>+{
>+    JSRuntime *rt = cx->runtime;
>+    JSBool ok;
>+    JSDHashEntryStub *entry;
>+
>+    JS_LOCK_GC(rt);
>+    entry = (JSDHashEntryStub *)
>+        JS_DHashTableOperate(&rt->gcScriptRootsHash, script, JS_DHASH_ADD);

Super-nits: usually decls order matches first use; old style eschewed init hiding in decl forest; newer style indents more than one basic offset, to underhang left paren of cast.

>+js_UnrootScript(JSContext *cx, JSScript *script)
>+{
>+    JSRuntime *rt = cx->runtime;
>+

Hard to object to this init, no hiding problems.

>+    JS_LOCK_GC(rt);
>+    JS_DHashTableOperate(&rt->gcScriptRootsHash, script, JS_DHASH_REMOVE);
>+    JS_UNLOCK_GC(rt);

Could use JS_LOCK_GC_VOID(rt, ...) one-liner.

Looks good, just wanted to measure the population of mScriptRootsHash in Firefox and confirm the tvr elimination analysis.

/be

Comment 4

[Igor Bukanov]

(In reply to comment #3)
> Probably not worth making this lazily, given top-level script creation and
> destruction -- but it could be good for Firefox, which (IIRC) always uses
> JS_NewScriptObject.

If so, then the proposed patch would first add the script to a hashtable after compiling a script and than remove it from the hash in JS_NewScriptObject. This does not sound as a good pattern.   

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #4)
> (In reply to comment #3)
> > Probably not worth making this lazily, given top-level script creation and
> > destruction -- but it could be good for Firefox, which (IIRC) always uses
> > JS_NewScriptObject.
> 
> If so, then the proposed patch would first add the script to a hashtable after
> compiling a script and than remove it from the hash in JS_NewScriptObject. This
> does not sound as a good pattern. 

Of course -- that does seem like a problem. But the fix can consist only if new API, which would be more convenient as well as safer to boot: something like JS_Compile*ScriptObject*.

/be

Comment 6

[Igor Bukanov]

(In reply to comment #5)
> But the fix can consist only if new
> API, which would be more convenient as well as safer to boot: something like
> JS_Compile*ScriptObject*.

The fix does not require new API: it is sufficient to initialize hashtable lazily only if there are more then one JSScrit instance that needs rooting and just store the single JSScript as it is. 

Comment 7

[Brendan Eich [:brendan]]

Neat idea -- could do the single-shot script-root cache per thread (JSThread) if JS_THREADSAFE, a la the GSN and property caches.

/be

Comment 8

[Jason Orendorff [:jorendorff]]

(In reply to comment #3)
> Any way to get the effect of diff -p and --git together?

Yes, sorry about that.

I think "hg qdiff -p" will be supported in the next hg feature release, thanks to this comment.  :)

> > JS_NewScriptObject(JSContext *cx, JSScript *script)
> > {
> >-    JSTempValueRooter tvr;
> >     JSObject *obj;
> > 
> >     if (!script)
> >         return js_NewObject(cx, &js_ScriptClass, NULL, NULL, 0);
> > 
> >-    JS_PUSH_TEMP_ROOT_SCRIPT(cx, script, &tvr);
> >     obj = js_NewObject(cx, &js_ScriptClass, NULL, NULL, 0);
> >     if (obj) {
> >         JS_SetPrivate(cx, obj, script);
> >+        if (!script->object)
> >+            js_UnrootScript(cx, script);
> >         script->object = obj;
> > #ifdef CHECK_SCRIPT_OWNER
> >         script->owner = NULL;
> > #endif
> >     }
> >-    JS_POP_TEMP_ROOT(cx, &tvr);
> 
> Let me double-check out loud: the tvr is not needed because js_NewObject can
> run only a last-ditch GC, which will not collect script's atoms. It won't run a
> full (NORMAL) GC.

Better, the script is rooted in the gcScriptRootsHash.

While we're here: I think "if (!script->object)" should be strengthened to a JS_ASSERT.  It's never been OK to call JS_NewScriptObject more than once for a given script, right?  It would be destroyed twice.

Re comment #7: It seems like a per-thread cache would be API-visible.  Without it, you can compile a script, then hand it off to a worker thread that executes and destroys it.  With a cache, if you do that, the next GC would crash (tracing the destroyed script, still in the creating thread's "newborn script" root).

Metering on Monday.

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> > Let me double-check out loud: the tvr is not needed because js_NewObject can
> > run only a last-ditch GC, which will not collect script's atoms. It won't run a
> > full (NORMAL) GC.
> 
> Better, the script is rooted in the gcScriptRootsHash.

Right, that Igor pointed out, which led to the concern about bloating and slowing down things for top-level scripts compiled and then immediately wrapped in an object, even if the hashed root went away once the object wrapped the script.

> While we're here: I think "if (!script->object)" should be strengthened to a
> JS_ASSERT.  It's never been OK to call JS_NewScriptObject more than once for a
> given script, right?  It would be destroyed twice.

Right -- there's no such if test in the top of mozilla-central JS_NewScriptObject code.

> Re comment #7: It seems like a per-thread cache would be API-visible.  Without
> it, you can compile a script, then hand it off to a worker thread that executes
> and destroys it.  With a cache, if you do that, the next GC would crash
> (tracing the destroyed script, still in the creating thread's "newborn script"
> root).

I don't see how (without a script roots hashtable) you can pass a "naked" script between threads safely without rooting it (in the traditional JS_NewScriptObject way: with an object, referenced by a rooted pointer).

/be

Comment 10

[Brendan Eich [:brendan]]

Regarding the need for a script object to safely pass a script from one thread to another, see the CHECK_SCRIPT_OWNER ifdefs in jsscript.{h,cpp}.

/be

Comment 11

[Jason Orendorff [:jorendorff]]

(In reply to comment #7)
> Neat idea -- could do the single-shot script-root cache per thread (JSThread)
> if JS_THREADSAFE, a la the GSN and property caches.

Sorry for the delay.  I planned to go ahead and implement this.  It got unexpectedly ugly because JSThreads are shared by multiple runtimes.  Clearly each JSThread's script-root-cache slot must only be used by one runtime at a time.  I see two possible strategies for marking the cached script-roots.  (1) Give each JSRuntime a linked list of all the JSThreads whose script-root-cache slot it owns.  But the linked list has to be locked with gcLock; this stinks.  (2) Use the existing list of JSContexts.  But then when a thread leaves a request, it'll have to empty the cache slot (by putting the cached script, if any, into the hash table).  That stinks too.

Hope this makes sense.  At the moment I would be thrilled to spend a hundred instructions per CompileScript to avoid this complexity.  If Igor has anything left in his bag of tricks, that would be even better.

Comment 12

[Brendan Eich [:brendan]]

Oh, sorry -- my bias toward at most one runtime per thread is showing again.

Obvious easy out is to put the single-shot root in the cx.

/be

Comment 13

[Jason Orendorff [:jorendorff]]

It's still unclear to me what API I should be trying to restore.  My current guess is:

- What comment 0 says.  JS_Compile*Script* or JS_CompileFile* return a rooted script.  JS_DestroyScript and JS_NewScriptObject unroot it. (Per bug 438052 comment 2, especially the bit about reference counting. I have nothing else to go on as CVS history doesn't go back that far!)

- Do not pass a JSScript to another thread without successfully passing it to JS_NewScriptObject and rooting the result. (Brendan has said this is necessary for GC safety, and the comment in js_DestroyScript agrees. But the script is supposed to be rooted. So with this bug fixed, I don't think this is actually a GC-related restriction. The only remaining reason I see for it is property cache safety.)

What am I missing?

Has this bug really existed ever since the switch from refcounting to GC?

Are these proposed API rules too generous?  (They say that a script is rooted even across leaving and re-entering a request.  They allow you to compile a script in one context, then executing it in another on the same thread, *without* calling JS_NewScriptObject first.)

Comment 14

[Igor Bukanov]

(In reply to comment #12)
> Oh, sorry -- my bias toward at most one runtime per thread is showing again.
> 
> Obvious easy out is to put the single-shot root in the cx.

But one have to remember to transfer that single-shot script to the global table when the context is disconnect from the runtime (see js_ClearContextThread). Otherwise, when one stores the JS_Compile*Script* for consumption on the other thread later, a GC will not see it.

Of cause, all this complexity can be removed if JS_Compile*Script* would return an object. It may not be that bad as API breakage since FF managed not to use the functions.

Comment 15

[Jason Orendorff [:jorendorff]]

I don't know if the single-shot root in cx is safe at all.  Code like this:

  s = JS_CompileScript(cx1, ...);
  ...
  JS_DestroyScript(cx2, s);

would fail to find where s is rooted; it would remain rooted, leading to a crash.

I think it's allowed for a thread to be in a request on multiple contexts at the same time.  If so, this can't be fixed just by flushing to the global table in JS_EndRequest rather than ClearContextThread.

I think making JS_Compile*Script* return an object is equivalent to WONTFIXing this.  To me it sort of depends on how old this bug is.

Comment 16

[Jason Orendorff [:jorendorff]]

I don't mean "is equivalent to" in comment 15.  Rather "implies".

Comment 17

[Igor Bukanov]

(In reply to comment #15)
> I don't know if the single-shot root in cx is safe at all.  Code like this:
> 
>   s = JS_CompileScript(cx1, ...);
>   ...
>   JS_DestroyScript(cx2, s);
> 
> would fail to find where s is rooted; it would remain rooted, leading to a
> crash.

Well, the script that requires an explicit JS_DestroyScript can be marked. Then JS_DestroyScript would just unmark it and delegates the real destruction of the script to the GC. 

Comment 18

[Jason Orendorff [:jorendorff]]

(In reply to comment #17)

Interesting idea. As a side effect of this approach, I think the second restriction I mentioned in comment 13 could be dropped. That would mean all the CHECK_SCRIPT_OWNER checks could be removed.

Comment 19

[Brendan Eich [:brendan]]

This bug is ancient. If we can fix it without changing API and increasing the size of JSScript for all callers (specifically, for those that do not wrap the script in object clothing), I think we should.

Re: comment 15, Igor's comment 17 suggests a way: keep JSContext *creator; in the JSScript (could replace the #ifdef'ed JSThread *owner; member), use a cx one-shot root, if JS_DestroyScript is called on the script on a different cx, clear the one-shot in script->creator if it refers to script still.

If someone destroyed cx1 then that would potentially run GC (last context always does). If cx2 is created and then cx1 is destroyed with the JS_DestroyContextNoGC API, then the script would become unprotected. But this is a hard case, and no worse than the case today (and for many years). The common case is one and only one cx used for the script creation and destruction, again where there is no JS_NewScriptObject call to take over ownership.

/be

Comment 20

[Brendan Eich [:brendan]]

(In reply to comment #18)
> (In reply to comment #17)
> 
> Interesting idea. As a side effect of this approach, I think the second
> restriction I mentioned in comment 13 could be dropped. That would mean all the
> CHECK_SCRIPT_OWNER checks could be removed.

This is not a good idea in light of the scenario in comment 19 paragraph 3 -- cx1 destroyed after cx2 is created without running the GC will lose the one-shot. Oh, but you could move any left-over one-shot to the global script roots hash and clear that from JS_DestroyScript. Win-win?

Again, it's not really a valid API pattern and we don't need to support it as far as I know.

Also, CHECK_SCRIPT_OWNER is there for a deeper reason. See the comment from js_DestroyScript:

     * JS_THREADSAFE note: js_FlushPropertyCacheForScript flushes only the
     * current thread's property cache, so a script not owned by a function
     * or object, which hands off lifetime management for that script to the
     * GC, must be used by only one thread over its lifetime.

/be
/be

Comment 21

[Jason Orendorff [:jorendorff]]

Here's my interpretation of Igor's comment 17:

JSContext gains a one-shot cache.  JSScript gains a bit which I'll call destroyFlag.  It's only meaningful when script->object is NULL, so maybe it can be stored there.

JS_Compile*Script* and JS_CompileFile*: Initialize destroyFlag to 0.  Root using the cx's one-shot cache.

JS_DestroyContext:  Bump the cached script root to the hash.

JS_DestroyScript:  Only set destroyFlag=1, don't call js_DestroyScript, and don't try to remove the script root.

JS_NewScriptObject:  Assert destroyFlag==0.

When tracing a script:  if destroyFlag == 1, call js_DestroyScript instead of js_TraceScript, and remove the script root.

(In reply to comment 20 last paragraph)
I think we can just remove js_FlushPropertyCacheForScript entirely.  With these changes, scripts would only be destroyed during GC, when all property caches are empty.

I'll give this a shot later today.

Comment 22

[Igor Bukanov]

(In reply to comment #21)
> When tracing a script:  if destroyFlag == 1, call js_DestroyScript instead of
> js_TraceScript, and remove the script root.

One more rule is necessary about what to do when tracing the context with an occupied script cache. If destroyFlag == 1, then the cache has to be cleared, otherwise the script should be normally traced.

Comment 23

[Brendan Eich [:brendan]]

Win-win, indeed -- getting rid of js_FlushProprtyCacheForScript avoids an O(n^2) growth hazard as well as reduces code footprint. Great to see this come together!

/be

Comment 24

[Jason Orendorff [:jorendorff]]

Bumping the cached script root to the hash in JS_DestroyContext can fail (adding an entry to the hash table -- out of memory).  Unlikely in practice, but there it is.  I don't see an elegant way around it.  One approach is to leak the context (i.e., to leave them in the runtime's list).  :-P  Another is to make sure there's always enough room in the hash table.  Any other ideas?

Comment 25

[Brendan Eich [:brendan]]

Could we keep an atomic counter in the runtime of how many such single-shot caches there might be in the runtime, and arrange to reserve (that / max_alpha) hash table space?

/be

Comment 26

[Igor Bukanov]

(In reply to comment #25)
> Could we keep an atomic counter in the runtime of how many such single-shot
> caches there might be in the runtime, and arrange to reserve (that / max_alpha)
> hash table space?

As an alternative to a hashtable a double-linked list of JSScript can be used with the linkage nodes allocated and stored in JSScript->object (using the lowest pointer bit to separate the cases) until the script is rooted. The node will also contain a back-reference to its JSScript instance. 

Then JSContext will contain a pointer to such node for the last compiled script. When the script is rooted and its linkage node is in the current JSContext, that node is not released but rather continue to serve as an allocation cache for the future compiled scripts. When a new script is compiled without destroying/rooting the previous one, the old node is transferred to the global linked list. The same happens in JS_DestroyContext.

Since the transfer does not require any allocation, this schema should not safer that allocation-from JS_DetroyContext problem while still not extending JSScript with extra fields.

Yet another suggestion to avoid all these complexity is to add a new compilation API that will create the holding JSOBject*, switch FF code to use it, and then use the hashtable as in the suggested patch for the users who continue to use the old APIs.

Comment 27

[Jason Orendorff [:jorendorff]]

(In reply to comment #26)
> Yet another suggestion to avoid all these complexity is to add a new
> compilation API that will create the holding JSOBject*, switch FF code to use
> it, and then use the hashtable as in the suggested patch for the users who
> continue to use the old APIs.

It occurred to me that JS_CompileFunction is an existing API that's sort of like that, so I did some digging.  The only stack I can find in Mozilla that leads to any JS_Compile*{Script,File}* API is:

  1. JS_CompileUCScriptForPrincipals, called from
  2. nsJSContext::CompileScript (nsJSEnvironment.cpp:1609), called from
  3. nsXULPrototypeScript::Compile (nsXULElement.cpp:3219), called from
  4. nsXULDocument::OnStreamComplete (nsXULDocument.cpp:3350)
  or XULContentSinkImpl::HandleEndElement (nsXULContentSink.cpp:625)

For <script> elements, we use JS_EvaluateUCScriptForPrincipals, and for event handlers, JS_CompileUCFunctionForPrincipals.

Comment 28

[Brendan Eich [:brendan]]

xpconnect/loader/mozJSComponentLoader.cpp:        script = JS_CompileScriptForPrincipals(cx, global,
xpconnect/loader/mozJSComponentLoader.cpp:         * JS_CompileFileHandleForPrincipals().
xpconnect/loader/mozJSComponentLoader.cpp:        script = JS_CompileFileHandleForPrincipals(cx, global,

Other embeddings whose source we can't search easily or at all may have similar dependencies. Rather than make them all change (we could and should for a Mozilla 2 era release), can we safen the existing API a la something like what comment 26 proposes?

/be

Comment 29

[Jason Orendorff [:jorendorff]]

Attachment: v2 - minor fixes

v2 - Still using the hash table, but avoid rooting the temporary script in JS_Evaluate*.

With this, I can run Firefox and log into gmail without triggering a call to js_RootScript.  If I delete XUL.mfasl, I get about 50 calls to js_RootScript total.

Brendan, feel free to minus this and I'll get around to implementing Igor's linked list scheme eventually.

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

Note also bug 440558.

Comment 31

[Brendan Eich [:brendan]]

Comment on attachment 326756 [details] [diff] [review]
v2 - minor fixes

>+    if (script && !js_RootScript(cx, script)) {
>+        js_DestroyScript(cx, script);
>+        js_ReportOutOfMemory(cx);

js_RootScript should report, and does (inconsistently).

>+    /* Script roots.  See js_RootScript in jsscript.cpp. */

Down with French spacing, it's so last millennium. ;-)

Great to hear this doesn't affect warm start. Suggest igor take a review run, if he agrees it's strictly progress to get this in, I'm in favor.

/be

Comment 32

[Jason Orendorff [:jorendorff]]

Attachment: v3

Fixes for comment 31. (That error-reporting protocol gaffe is pretty embarrassing. I chalk it up to fatigue-- there were several false starts and reverts here.)

Comment 33

[Igor Bukanov]

Here is yet another schema that get away from a hashtable or extra fields in the script. The idea is to replace the hashtable in the comment 32 patch via single-linked list. The link field will be shared with JSScript.object. That requires one bit for pointer tagging. 

Then JS_DestroyScript would use another available bit to mark the script object as free delegating the destruction of scripts to the GC. The latter would simply walk the list and call js_DestroyScript on any entry marked as free. Since JS_DestroyScript can only be called once and, when it is called, the script must not be rooted through an object, such marking-as-free can be done without taking any locks.

Comment 34

[Igor Bukanov]

(In reply to comment #33)
> Here is yet another schema that get away from a hashtable or extra fields in
> the script. The idea is to replace the hashtable in the comment 32 patch via
> single-linked list. The link field will be shared with JSScript.object. That
> requires one bit for pointer tagging. 

Please ignore this: with single-linked list it is not possible to remove the script from the list when rooting it through the object.

Comment 35

[Igor Bukanov]

(In reply to comment #33)
> Then JS_DestroyScript would use another available bit to mark the script object
> as free delegating the destruction of scripts to the GC. The latter would
> simply walk the list and call js_DestroyScript on any entry marked as free.
> Since JS_DestroyScript can only be called once and, when it is called, the
> script must not be rooted through an object, such marking-as-free can be done
> without taking any locks.

That comment was not completely wrong as this may work with the hashtable as well. That is, JS_DestroyScript can just set script->object to (void *) 1 without taking any locks. Then js_TraceScriptRoots dueing GC will call js_DestroyScript for such scripts instead of tracing them.

This will remove the need to call js_FlushPropertyCacheForScript and JS_CLEAR_GSN_CACHE in js_DestroyScript.

But this can be done in another bug.

Comment 36

[Igor Bukanov]

To summarize the js_DestroyScript-only-during GC suggestions for the patch:

1. JS_DestroyScript simply marks the script object as ready-for-destruction without doing anything else.

2. JS_NewScriptObject does not call js_UnrootScript removing the need for the function.

3. js_TraceScriptRoots, when called during GC, removes the rooted-through-objects scripts and ready-for-destruction from the hashtable and calls js_DestroyScript for the latter.

Comment 37

[Jason Orendorff [:jorendorff]]

Attachment: v4 - also XDR

The only change here is to fix up JS_XDRScript.

For some reason this wasn't showing up in my Mochitesting, but running Minefield from the app bundle, it crashed right away.  JS_XDRScript also needs to root the new script.  v4 fixes the crash... but this probably invalidates my conclusion that we can the performance cost of locking in js_RootScript is trivial.  I'll take another look later this week.

Comment 38

[Jason Orendorff [:jorendorff]]

I can make linked lists work, but it involves adding a word to each non-function JSScript.  The list linkage can't share the JSScript.object field.

The problem is that JS_NewScriptObject must store the pointer to the new script object in the JSScript.  But it can't break the linked list of script roots, as that would leave everything after it on the list stranded.

JS_NewScriptObject could walk the tail of the list, re-rooting everything; or the per-context list of rooted scripts could instead be a list-of-lists, and then JS_NewScriptObject could re-root the tail of the list in O(1) time.  Both seem worse to me than adding a word.

Comment 39

[Igor Bukanov]

(In reply to comment #38)
> The problem is that JS_NewScriptObject must store the pointer to the new script
> object in the JSScript.  But it can't break the linked list of script roots, as
> that would leave everything after it on the list stranded.

That link can be moved to one of the object fixed slots until the next GC when the scripts with objects can be removed from the list.

Comment 40

[Jason Orendorff [:jorendorff]]

Coming back to this from quick-stub-land.

So, stupid question, but can we just delete JS_DestroyScript and require applications to use JS_NewScriptObject?  It seems to me like this is effectively the case already.  (We would also get to delete js_FlushPropertyCacheForScript and CHECK_SCRIPT_OWNER.)

Another option here is to retain JS_DestroyScript but add an API function, JS_PinScript(cx, script), which you must call immediately after compiling a script, if you intend to destroy it explicitly.

That is, either:
  JS_CompileScript, JS_PinScript, ..., JS_DestroyScript
or:
  JS_CompileScript, JS_NewScriptObject, <root it>, ...

JS_PinScript can then have the simple hash table implementation, locking and all.  The hash table can be lazily initialized.  Applications that use JS_NewScriptObject exclusively would see no performance penalty.

Comment 41

[timeless]

technically you can't make JS_DestroyScript go away, it's an api promise :)

Comment 42

[Jason Orendorff [:jorendorff]]

But read the thread.  The API has been crashingly broken for a long time.  Is anyone using it?  Wouldn't they be better off using something that didn't crash?

Comment 43

[timeless]

http://mediatomb.cc/doxygen/script_8cc-source.html shows that "mediatomb" uses it

Comment 44

[Brendan Eich [:brendan]]

Timeless: JS_DestroyScript is a promise of what, exactly? Prompt free of the JSScript struct itself? That can't be measured reliably.

If we can make JSScript a GC-thing and make JS_DestroyScript a stub, we absolutely should!

I say we can. Who's with me?

/be

Comment 45

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

You have my axe.

Comment 46

[Brendan Eich [:brendan]]

Wait, I thought you were the tall elf chick?

/be

Comment 47

[timeless]

oh, i'm not saying you can't make it do nothing, just that you can't remove it.

I was replying to:
[comment 40]... can we just delete JS_DestroyScript...

As to how's it's used, I think that...

http://www.google.com/codesearch/p?hl=en#y9q18Jz7Yj0/mediatomb-0.9.0/src/layout/js_layout.cc&q=JS_CompileScript&l=658

ContentManager::_addFile(...)
. ContentManager::initLayout()
.. JSLayout::init()
... JS_CompileScript(...)
. JSLayout::processCdsObject(...)
.. JS_NewObject(...)
... JS_ExecuteScript(...)

means that JS_NewObject is called between JS_CompileScript and JS_ExecuteScript, so if JS_NewObject triggers a gc, then mediatomb is unhappy (assuming i've read everything here correctly).

Comment 48

[Igor Bukanov]

(In reply to comment #47)
> ContentManager::_addFile(...)
> . ContentManager::initLayout()
> .. JSLayout::init()
> ... JS_CompileScript(...)
> . JSLayout::processCdsObject(...)
> .. JS_NewObject(...)
> ... JS_ExecuteScript(...)
> 
> means that JS_NewObject is called between JS_CompileScript and
> JS_ExecuteScript, so if JS_NewObject triggers a gc, then mediatomb is unhappy
> (assuming i've read everything here correctly).

This nicely demonstrates the problem with the current JS_CompileScript implementation. It is rather hard to use it correctly. So the API has to be either fixed or removed.

Now, a simple way to fix this is to always create an object for the script in JS_CompileUCScriptForPrincipals and call JS_LockGCThing on that object before JS_Compile returns. This way the script remains rooted until at least js_DestroyScript or js_NewScriptObject (these two functions will call JS_UnlockGCThing).

Comment 49

[Jim Blandy :jimb]

(In reply to comment #48)
> Now, a simple way to fix this is to always create an object for the script in
> JS_CompileUCScriptForPrincipals and call JS_LockGCThing on that object before
> JS_Compile returns. This way the script remains rooted until at least
> js_DestroyScript or js_NewScriptObject (these two functions will call
> JS_UnlockGCThing).

Is it typical for JSAPI function that construct objects to lock them in this way?  It seems to me that JS_Compile*Script* functions should follow the same conventions as JS_NewObject or JS_NewString --- require callers to "root as they go", etc.

Comment 50

[Brendan Eich [:brendan]]

Igor may have suggested locking since JSScript is not a GC-thing and the user of the JS_Compile*Script* APIs won't have the script's object to root as they go. So some implicit GC protection would be needed. Price is a hashtable entry in the runtime -- or compartment now (or soon), I guess.

Making JSScript a GC-thing would change this game and we might then get away with root-the-returned-script-as-you-go -- especially now, with the conservative stack scanner to ease the pain.

But JSScript is allocated together with code/srcnote vectors and other immediate-indexed tables, all in one malloc allocation. The GC won't handle that variable size allocation. Changing things to hang off the script could resolve this, at some cost (could fuse all the things hanging off into one malloc allocation again).

The implicit script object creation with locking is certainly the shortest path to a fix, and that looks like it might be important to fix the purging problems reported in bug 584619. Is there a better fix?

/be

Comment 51

[Brendan Eich [:brendan]]

Implicit script object creation would require JS_NewScriptObject to change to return the pre-created script object, I think. Existing JS_Compile*Script API users do call JS_NewScriptObject and root the object pointer, and we would not want two script objects for a given script.

/be

Comment 52

[Jim Blandy :jimb]

(In reply to comment #50)
> Igor may have suggested locking since JSScript is not a GC-thing and the user
> of the JS_Compile*Script* APIs won't have the script's object to root as they
> go.

There was a simpler solution; is the below flawed, or just forgotten?

Since this bug has existed for so long, it's clear that clients are either calling JS_NewScriptObject, or don't actually need scripts to survive until a distant JS_DestroyScript. So the following new API promises:

1) all script creation immediately creates a script object;
2) JS_NewScriptObject simply hands out that pre-allocated script object; and
3) JS_DestroyScript becomes a no-op

would hurt no clients, and ensure that all JSScripts get freed.

Comment 53

[Brendan Eich [:brendan]]

Yeah, that's better. Igor should say "yea" too.

/be

Comment 54

[Igor Bukanov]

(In reply to comment #52)
> 1) all script creation immediately creates a script object;

For scripts that are not COMPILE_AND_GO the created script object must have the parent and the prototype cleared. Otherwise this is a fine plan.

Comment 55

[Jim Blandy :jimb]

Aye aye.

Comment 56

[Jim Blandy :jimb]

(In reply to comment #54)
> For scripts that are not COMPILE_AND_GO the created script object must have the
> parent and the prototype cleared. Otherwise this is a fine plan.

I will do this, but --- why is it necessary?

Comment 57

[Jim Blandy :jimb]

Just to let people know what's up with this:

I wrote a patch for this, but then realized that the debugger could see the temporary JSScripts created by obj_eval, JS_EvaluateUCInStackFrame, and the like, and might well create script objects for them, so the code to free those scripts needs to be prepared for this.

I need to take off for a kids' activity in a bit, but I'll come back and finish this tonight or tomorrow.

Comment 58

[Brendan Eich [:brendan]]

Is it necessary to gift every script with an object? The eval ones are rooted by their frame while active, and weak in the eval script cache (the GC will collect them). I'd rather the implicit object be created only at the API JS_Compile* entry points. Igor may have a thought too.

/be

Comment 59

[Jim Blandy :jimb]

No, we don't need to create script objects for the temporary scripts created by eval-like functions.

I was worried about debuggers getting their hands on those scripts and noticing that they had no objects. But if any debugger had ever called JS_NewScriptObject on such a temporary eval script, it would be freed twice. And in fact, jsd is careful to release its references to scripts when the destroyScriptHook is invoked, so it doesn't seem that debuggers have any expectation of being able to treat these scripts as first-class objects.

Comment 60

[Jim Blandy :jimb]

(In reply to comment #54)
> (In reply to comment #52)
> > 1) all script creation immediately creates a script object;
> 
> For scripts that are not COMPILE_AND_GO the created script object must have the
> parent and the prototype cleared. Otherwise this is a fine plan.

I did this, but the compartment checking code in js::CompartmentChecker was unhappy to find an object whose scope chain didn't end in a global. So I have taken it out for now.

Comment 61

[Brendan Eich [:brendan]]

The null parent => global rule trumps others.

Igor, were you concerned about memory leaks or bloat due to entrainment? Having a non-null parent seems necessary for the compartmentalized world, if not already.

/be

Comment 62

[Igor Bukanov]

(In reply to comment #61)
> The null parent => global rule trumps others.

What about regexp, block and function objects for !COMPILE_AND_GO scripts? They have null parents. 

> 
> Igor, were you concerned about memory leaks or bloat due to entrainment? Having
> a non-null parent seems necessary for the compartmentalized world, if not
> already.

We dot not yet clone scripts across compartments. So until this is fixed for !COMPILE_AND_GO scripts the Script object should have a null parent or we would get a memory leak.

Comment 63

[Jim Blandy :jimb]

Attachment: JSAPI unit tests: verify that script objects are allocated eagerly.

These tests exercise the new API, in which script objects are allocated
immediately for scripts that can have them.

They depend on the patches in bug 505803.

No review requested while I check the performance, but comments are welcome.

Comment 64

[Jim Blandy :jimb]

Attachment: Give new JSScript objects lifetimes like GCThings.

Attach script objects immediately in all JSAPI script-creating functions;
have JS_NewScriptObject simply return the already-allocated object; and
make JS_DestroyScript a no-op.

Verify that all scripts given to JSAPI script-consuming functions have
objects, or are the canonical empty script object.

No review requested: this patch depends on the patches in bug 585803, and I need to check for perf impact.

Comment 65

[Jim Blandy :jimb]

(In reply to comment #63)
> They depend on the patches in bug 505803.

That should be bug 585803.

Comment 66

[Brendan Eich [:brendan]]

Comment on attachment 464458 [details] [diff] [review]
Give new JSScript objects lifetimes like GCThings.

>-    LAST_FRAME_CHECKS(cx, script);
>+    if (!script) {
>+        LAST_FRAME_CHECKS(cx, script);
>+        return NULL;
>+    }
>+    if (!js_NewScriptObject(cx, script)) {
>+        js_DestroyScript(cx, script);
>+        return NULL;
>+    }
>     return script;

LAST_FRAME_CHECKS needs to happen no matter what, on the way out. So something like

>+    if (script && !js_NewScriptObject(cx, script)) {
>+        js_DestroyScript(cx, script);
>+        script = NULL;
>+    }
>+    LAST_FRAME_CHECKS(cx, script);
>     return script;

Same for other such places in jsapi.cpp.

>+        /*
>+         * A 'script object', of class js_ScriptClass, to ensure the script is
>+         * GC'd.

Could lose the '' quotes and first comma, might fit on one line.

>+         * - All scripts returned by JSAPI functions (JS_CompileScript,
>+         *   JS_CompileFile, etc.) have these objects.
>+         * - Function scripts never have script objects; such scripts are owned
>+         *   by their function objects.
>+         * - Temporary scripts created by obj_eval, JS_EvaluateScript, and 
>+         *   similar functions never have these objects; such scripts are
>+         *   explicitly freed by the code that created them.
>+         * Debugging API functions (JSDebugHooks::newScriptHook;
>+         * JS_GetFunctionScript) may reveal script-object-free Function and

The -free suffix on "script-object-free" is may make readers stumble, since the other meaning of "free" was just used above ("explicitly freed"). Suggest changing "explicitly freed" to explicitly destroyed (could even cite js_DestroyScript), or rewording here.

>+         * temporary scripts to clients, but clients must never call
>+         * JS_NewScriptObject on such scripts: doing so would double-free them.

"once from the explicit destroy call and once from the GC" might be worth adding.

>+         */
>+        JSObject    *object;
>         JSScript    *nextToGC;  /* next to GC in rt->scriptsToGC list */
>     } u;
> #ifdef CHECK_SCRIPT_OWNER

r=me with these addressed.

/be

Comment 67

[Jim Blandy :jimb]

It turns out that clearing the script object's parent breaks security checks like these, from dom/base/nsJSEnvironment.cpp:

  JSObject *scriptObj = (JSObject*)aScriptObject;
  nsCOMPtr<nsIPrincipal> principal;

  rv = sSecurityManager->GetObjectPrincipal(mContext, scriptObj, getter_AddRefs(principal));
  NS_ENSURE_SUCCESS(rv, rv);

So I think the patch needs to only clearProto() for now.

Comment 68

[Jim Blandy :jimb]

Attachment: Give new JSScript objects lifetimes like GCThings.

Revised per brendan's instructions; also revised not to clear script objects' parent links, as those are used for security checks.

I'll be travelling Friday through Sunday, so I can't shepherd this patch through the tests; I'll land it on Monday.

Comment 69

[Jason Orendorff [:jorendorff]]

Comment on attachment 464457 [details] [diff] [review]
JSAPI unit tests: verify that script objects are allocated eagerly.

>+        /* JS_DestroyScript is now a no-op; scripts are always managed by the
>+           collector. */
>+        JS_DestroyScript(cx, script);
>+
>+        /* After a JS_DestroyScript call, the script should still work. */
>+        CHECK(JS_ExecuteScript(cx, global, script, &result));

Are we really making this an API guarantee? I claim we shouldn't and therefore we shouldn't add this to the test.

Reasons we shouldn't make this an API guarantee: (1) there's nothing wrong with the current API guarantee, i.e. we promise nothing; (2) this is perverse and applications should not do it; (3) who knows, we might actually want to go back and make JS_DestroyScript do some prompt cleanup someday, to reduce GC pressure.

Comment 70

[Jason Orendorff [:jorendorff]]

er, r=me with those concerns addressed (by removing that part of the change or by pointing out how wrong I am)

Comment 71

[Brendan Eich [:brendan]]

I agree with Jason -- even if we stick with GC, we shouldn't rule out it nesting in JS_DestroyScript.

/be

Comment 72

[Jim Blandy :jimb]

Attachment: JSAPI unit tests: verify that script objects are allocated eagerly.

Removed test that scripts are still usable after being deleted.

Comment 73

[Jim Blandy :jimb]

http://hg.mozilla.org/tracemonkey/rev/05950abb7223

(Code changes only; unit tests still need r+.)

Comment 74

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/05950abb7223