GNOME Bugzilla – Bug 691038
Fixes for debug builds of mozjs187
Last modified: 2013-01-04 22:08:30 UTC
When trying to use JS_DumpHeap, I found that these additional assertions were added, crashing the shell. We should probably all use debug builds.
Created attachment 232588 [details] [review] function: Add BeginRequest/EndRequest for JS_ValueRoot
Created attachment 232589 [details] [review] gi: Use raw JS_GetPrivate from tracing callbacks We don't need a type-check, as we already know it's the right class. priv_from_js is going to gain BeginRequest/EndRequest, which we can't use from tracing hooks, so omit those for now.
Created attachment 232590 [details] [review] util: Add BeginRequest/EndRequest back to priv_from_js Even though JS_GetInstancePrivate isn't documented as needing a request, it does.
Review of attachment 232590 [details] [review]: Calling begin/end only inside the priv_from_js looks wrong to me: once the context is unlocked, the object might be GCed and the returned value might be garbage. Unless I misunderstand the point of Begin/EndRequest, that is.
Review of attachment 232589 [details] [review]: Yes.
(In reply to comment #4) > Review of attachment 232590 [details] [review]: > > Calling begin/end only inside the priv_from_js looks wrong to me: once the > context is unlocked, the object might be GCed and the returned value might be > garbage. Unless I misunderstand the point of Begin/EndRequest, that is. Stack scanning prevents that from happening.
(In reply to comment #6) > (In reply to comment #4) > > Review of attachment 232590 [details] [review] [details]: > > > > Calling begin/end only inside the priv_from_js looks wrong to me: once the > > context is unlocked, the object might be GCed and the returned value might be > > garbage. Unless I misunderstand the point of Begin/EndRequest, that is. > > Stack scanning prevents that from happening. *If* the returned values lives on the stack, but we really hope it is not, considering how hot is priv_from_js. Anyway, priv_from_js is only called for alive (reachable) objects, and the GC thread cannot make them unreachable, so perhaps this is a false concern.
So does that mean it's ACN?
(In reply to comment #8) > So does that mean it's ACN? It means I'm not sure, so I'd prefer if someone else looked at this too. It's very easy to get GC wrong without immediate consequences, and I'd like to avoid hard to reproduce crashes.
Created attachment 232778 [details] [review] Make sure that all calls to BOOLEAN_TO_JSVAL pass a true boolean Debug builds of libmozjs assert that the value passed to BOOLEAN_TO_JSVAL is not just truthy, but one of JS_FALSE (0) or JS_TRUE (1). We can't possibly fix all API consumers to respect this rule, so do the conversion for them.
(In reply to comment #7) > *If* the returned values lives on the stack, but we really hope it is not, > considering how hot is priv_from_js. There's a bit of code to dump registers onto the stack in that case.
(In reply to comment #11) > (In reply to comment #7) > > *If* the returned values lives on the stack, but we really hope it is not, > > considering how hot is priv_from_js. > > There's a bit of code to dump registers onto the stack in that case. Ah, nice. Acn then.
Review of attachment 232778 [details] [review]: Yes (And it is really bad on the libmozjs side to assert such a thing, when they can just !! themselves)
Attachment 232588 [details] pushed as bac272f - function: Add BeginRequest/EndRequest for JS_ValueRoot Attachment 232589 [details] pushed as fba2a34 - gi: Use raw JS_GetPrivate from tracing callbacks Attachment 232590 [details] pushed as 3937d5f - util: Add BeginRequest/EndRequest back to priv_from_js Attachment 232778 [details] pushed as 46b1443 - Make sure that all calls to BOOLEAN_TO_JSVAL pass a true boolean