We keep accidentally introducing new rooting hazards across JSAutoCompartment because it is both not obvious that it can GC and generally comes first. Bill had an excellent idea: why not just wrap lazily when we get the exception, rather than on every compartment boundary. This fixes the above issue and saves us two branches on every compartment transition. I've verified all the bustage in this try run was present on inbound at the time, so it looks like this tiny patch was actually green on the first attempt: https://tbpl.mozilla.org/?tree=Try&rev=04d01048cabf

Won't this create GC hazard without special logic to trace the pending exception on all contexts during a compartmental GC?

Comment on attachment 8348890 [details] [diff] [review] lazy_wrap_exception-v0.diff Review of attachment 8348890 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me, but I'd like Luke to take a look. I'm kind of curious if there's a reason why we didn't do this in the first place; it seems like the more obvious way. Regarding Bobby's concern, the code should work fine. RootMarking.cpp unconditionally marks the exceptions for all contexts, regardless of whether it's a compartmental GC.

Also, thanks for exploring this Terrence.

Comment on attachment 8348890 [details] [diff] [review] lazy_wrap_exception-v0.diff Review of attachment 8348890 [details] [diff] [review]: ----------------------------------------------------------------- Cool! Nice to see those wrapPendingException calls go. ::: js/src/jscntxt.cpp @@ +1109,5 @@ > JS_ASSERT(!resolvingList); > } > > +js::Value > +JSContext::getPendingException() Perhaps I'm not aware of the relevant style, but I'd expect this to take a MutableValueHandle outparam isntead of returning a Value by value.

Since JSContext isn't exactly a slender class, it might be nice to rename 'exception' to 'unwrappedException_' to ensure everyone is cognizant of this.

Heh. I just got a mid-air when I was requesting that exception at least be commented as not necessarily being in the same compartment as cx anymore. Luke's idea is better.

https://hg.mozilla.org/integration/mozilla-inbound/rev/caf902c15026 Great recommendations! With the interface explicitly handling errors, we're actually responding to cross-compartment wrapping OOMs as errors rather than just losing the exception. It will be interesting to see how this shakes out.

(In reply to Bill McCloskey (:billm) from comment #2) > This looks good to me, but I'd like Luke to take a look. I'm kind of curious > if there's a reason why we didn't do this in the first place; it seems like > the more obvious way. Compartments are all about invariants. And in particular assertable invariants about limiting cross-compartment edges to (ideally) wrappers (but in practice, to stuff we've at least carefully considered). cx->exception is an edge. Having cx->exception always be in the same compartment as cx, like most every other edge, seemed like the "obvious" invariant at the time. The lazy invariant introduced here didn't occur to me as a possibility. I like this patch though, so I suppose you're right. :)

(In reply to Jason Orendorff [:jorendorff] from comment #9) > Compartments are all about invariants. And in particular assertable > invariants about limiting cross-compartment edges to (ideally) wrappers (but > in practice, to stuff we've at least carefully considered). This is still sort of important, of course. All edges into a zone must be reachable from the roots of a zone GC. Happily cx->exception poses no problems there; we mark all contexts unconditionally.