+++ This bug was initially created as a clone of Bug #737364 +++ This is a sub-bug for the bug 737364 with the focus on removing the cx usage during finalization/sweeping. In addition this bug is useful for the incremental/background sweeping implementation in the bug 729760. For that bug it is necessary to pass an extra parameters to the finalizers about the way they are invoked and that requires to change the finalizer signatures.

The first part replaces the JSContext argument of the finalizers with the JSFreeOp/FreeOp structs. I named this struct FreeOp and not, for example, FinalizeOp, as the intent here is to deal with free/delete. Also I use structure on the part 3 to deal with code that is shared between the finalizers and non-finalizers.

In part 1 the cx is still passed to the finalizers indirectly via FreeOp structure so the callers can access it if necessary. This part removes that cx usage in dom workers. This patch is untested.

This is completely untested patch and it is here only for references.

Comment on attachment 607505 [details] [diff] [review] part 1 - changing the finalizer signature Review of attachment 607505 [details] [diff] [review]: ----------------------------------------------------------------- Great patch. I've wanted to do this for a while, but I always got bogged down part way through. I really like the new FreeOp structure. One major request: I don't really like that FreeOp is passed around const. The only other place we use const a lot is with Shapes, and I think that was a mistake. There are const_casts on shapes everywhere. Is there any particular reason you used const? ::: js/ipc/ObjectWrapperChild.cpp @@ +410,5 @@ > static const PRUint32 sNextIdIndexSlot = 0; > static const PRUint32 sNumNewEnumerateStateSlots = 1; > > static void > +CPOW_NewEnumerateState_freeIds(JSObject* state) It seems like CPOW_NewEnumerate_FreeIds would be more consistent, maybe. ::: js/src/jsapi.cpp @@ +2232,5 @@ > return cx->free_(p); > } > > JS_PUBLIC_API(void) > +JS_Free(const JSFreeOp *fop, void *p) This is likely to cause some confusion :-). I guess it's okay as long as we deprecate JS_free pretty soon, which seems likely. ::: js/src/jsfriendapi.h @@ +306,5 @@ > > extern JS_FRIEND_API(bool) > GCThingIsMarkedGray(void *thing); > > +class FreeOp : public JSFreeOp { Is it necessary to expose this in jsfriendapi.h? Maybe I missed something, but it looks like the only uses of these definitions outside the JS engine are to fetch the context in a few places. Could we just expose the declaration of js::FreeOp but not the definition? And then provide a jsfriendapi function to fetch the context from a js::FreeOp? @@ +309,5 @@ > > +class FreeOp : public JSFreeOp { > + public: > + bool shouldFreeLater; > + bool backgroundThread; Could you rename this to something like onBackgroundThread so it's clear that it's a bool and not a thread? ::: js/xpconnect/idl/nsIXPCScriptable.idl @@ +160,5 @@ > in JSContextPtr cx, in JSObjectPtr obj, > in PRUint32 type, in JSValPtr vp); > > void finalize(in nsIXPConnectWrappedNative wrapper, > + in JSFreeOpPtr cx, in JSObjectPtr obj); Probably should be called fop, rather than cx.

(In reply to Bill McCloskey (:billm) from comment #4) > One major request: I don't really like that FreeOp is passed around const. > The only other place we use const a lot is with Shapes, and I think that was > a mistake. There are const_casts on shapes everywhere. Is there any > particular reason you used const? I just wanted to make sure that the callee can not modify FreeOp. As JSFreeOp is declared as a C structure the only way to make JSFreeOp::runtime is immutable is via const. But you are right, the patch adds way too much those const declarations. So I will update it with const removed and field in FreeOp made private. I will also add #ifdef C++ to JSFreeOp definition so only for C code it will be a plain struct. C++ will have JSFreeOp::runtime(). > > JS_PUBLIC_API(void) > > +JS_Free(const JSFreeOp *fop, void *p) > > This is likely to cause some confusion :-). I guess it's okay as long as we > deprecate JS_free pretty soon, which seems likely. That was my initial thinking. On the second thought, if JS_free(cx, p) disappears, it is rather likely that we may end up with JS_free(rt, p). So we better have a good name here from the beginning. Does JS_FreeOp(JSFreeOp *fop, void *p) look better? Or should it be JS_freeOp(fop, p)? > Is it necessary to expose this in jsfriendapi.h? Maybe I missed something, > but it looks like the only uses of these definitions outside the JS engine > are to fetch the context in a few places. Could we just expose the > declaration of js::FreeOp but not the definition? And then provide a > jsfriendapi function to fetch the context from a js::FreeOp? FreeOp::context is just a temporary solution to split the patch into separated parts that can be tested on its own. It will be removed in the part 3. But then to compile I indeed may well just add a friend access function to get the cx.

The new version drops const and stops exposing FreeOp via jsfriendapi.h. Temporary I added JSContext * to JSFreeOp to allow to compile and test this part independently.

For references here is the interdiff between a previous version of the patch with const removed and other changes.

This patch removes the usage of cx during finalization outside the JS engine. Currently that cx is extensively used in the dom/workers code where in many places finalizer share the code with explicit destruction. To support that the patch adds a default JSFreeOp that allows to call JS_freeop() outside a finalizer. Also in dom/workers I replaced in few method signatures the current (JSContext *aCx, bool aFromFinilizer) arguments with (JSContext *aCx, JSFreeOp *aFinalizerFop) where exactly one of aCx and aFinalizerFop must be given.

Comment on attachment 607922 [details] [diff] [review] part 1 - changing the finalizer signature Bent, could you also look at dom/workers changes here?

Here is another backup of part 3 - it is no yet ready for a review.

This part removes the usage of JSContext* during the finalization and sweeping. That required to change quite a few methods in type inference, jit and debugger implementation to take a FreeOp rather than JSContext pointer. In turn that also often required to replace cx->compartment usage with extracting the compartment from the passed objects or pass the compartment explicitly. On the plus side it allowed to remove fallible compartment enter code in methods that could be called during finalization.

Comment on attachment 607922 [details] [diff] [review] part 1 - changing the finalizer signature Review of attachment 607922 [details] [diff] [review]: ----------------------------------------------------------------- r=me on worker things!

Comment on attachment 608890 [details] [diff] [review] part 2 - stop using the cx in dom workers during finalization. Review of attachment 608890 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the worker changes!

Comment on attachment 609149 [details] [diff] [review] part 3 - eliminating remaining uses of cx during finalization. Review of attachment 609149 [details] [diff] [review]: ----------------------------------------------------------------- Thanks again, Igor. This looks great. Jason, can you look over the debugger change that I've pointed out below? ::: js/src/jsinfer.cpp @@ +2117,5 @@ > } > } > > void > +TypeCompartment::setPendingNukeTypesNoReport() Is it possible to assert that this one is only called from GC, and that setPendingNukeTypes is never called from GC? Also, it would be nice to have a comment about when one or the other should be used (maybe in the header file). ::: js/src/vm/Debugger.cpp @@ -292,5 @@ > - if (!forTrap) { > - ac.construct(cx, ScriptGlobal(cx, script, scriptGlobal)); > - if (!ac.ref().enter()) > - return false; > - } Removing this basically makes sense to me, since the two functions below it won't take a context. It still scares me a little, though. Why were we only entering the compartment if forTrap was true? Would we have been in the right compartment anyway if !forTrap? ::: js/xpconnect/src/XPCJSRuntime.cpp @@ +250,5 @@ > MOZ_COUNT_DTOR(xpc::CompartmentPrivate); > } > > +static void > +CompartmentCallback(JSFreeOp *fop, JSCompartment *compartment) Could you call this CompartmentDestroyedCallback?

(In reply to Bill McCloskey (:billm) from comment #14) > > void > > +TypeCompartment::setPendingNukeTypesNoReport() > > Is it possible to assert that this one is only called from GC, and that > setPendingNukeTypes is never called from GC? Also, it would be nice to have > a comment about when one or the other should be used (maybe in the header > file). I will try to do that, but do we want to report OOM if we can completely recover from it? I.e. in case the malloc fails during type inference we still AFAIK can compete the evaluation albeit slowly. Clearly, most likely we will fail later with OOM again, but then perhaps we should report OOM only if it changes the evaluation results. > > ::: js/src/vm/Debugger.cpp > @@ -292,5 @@ > > - if (!forTrap) { > > - ac.construct(cx, ScriptGlobal(cx, script, scriptGlobal)); > > - if (!ac.ref().enter()) > > - return false; > > - } > > Removing this basically makes sense to me, since the two functions below it > won't take a context. It still scares me a little, though. Why were we only > entering the compartment if forTrap was true? Would we have been in the > right compartment anyway if !forTrap? forTrap is false during the GC we could be in an arbitrary compartment when cx->compartment is a wrong one. So without the patch due to reliance of cx->compartment it was necessary to do the enter cal during the GC.

(In reply to Igor Bukanov from comment #15) > > Is it possible to assert that this one is only called from GC, and that > > setPendingNukeTypes is never called from GC? Also, it would be nice to have > > a comment about when one or the other should be used (maybe in the header > > file). > > I will try to do that, but do we want to report OOM if we can completely > recover from it? I.e. in case the malloc fails during type inference we > still AFAIK can compete the evaluation albeit slowly. Clearly, most likely > we will fail later with OOM again, but then perhaps we should report OOM > only if it changes the evaluation results. So you're saying we should use the NoReport version even when a GC is not running? I guess that makes sense, although I'd rather do that in a separate bug. > > Removing this basically makes sense to me, since the two functions below it > > won't take a context. It still scares me a little, though. Why were we only > > entering the compartment if forTrap was true? Would we have been in the > > right compartment anyway if !forTrap? > > forTrap is false during the GC we could be in an arbitrary compartment when > cx->compartment is a wrong one. So without the patch due to reliance of > cx->compartment it was necessary to do the enter cal during the GC. OK, maybe I'm just being paranoid.

feedback ping - Jason, could you comment on the comment 14?

Comment on attachment 609149 [details] [diff] [review] part 3 - eliminating remaining uses of cx during finalization. That code makes no sense to me. Go ahead and delete it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5057ff02ffb https://hg.mozilla.org/integration/mozilla-inbound/rev/74053b148a3c https://hg.mozilla.org/integration/mozilla-inbound/rev/d8c5316f513a

This seems to break non-threadsafe shell builds.

https://hg.mozilla.org/integration/mozilla-inbound/rev/06b921a803ac - follow up to fix !JS_THREADSAFE

https://hg.mozilla.org/mozilla-central/rev/d5057ff02ffb https://hg.mozilla.org/mozilla-central/rev/74053b148a3c https://hg.mozilla.org/mozilla-central/rev/d8c5316f513a

Followup: https://hg.mozilla.org/mozilla-central/rev/06b921a803ac

Is it possible that this could have affected to gc behavior? I'm trying to figure out what has caused Bug 743178, and I *think* it is something after http://hg.mozilla.org/mozilla-central/rev/9894cd999781

(In reply to Olli Pettay [:smaug] from comment #24) > Is it possible that this could have affected to gc behavior? > I'm trying to figure out what has caused Bug 743178, and I *think* > it is something after http://hg.mozilla.org/mozilla-central/rev/9894cd999781 This bug should not affect the GC as it just replaces the way the finalizers are called. I tried to avoid any changes to the substance in this relatively big refactoring patch delegating the real changes to the bug 737364.