Lazily wrap pending exceptions so that JSAutoCompartment does not GC

RESOLVED FIXED in mozilla29

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

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
Attachment #8348890 - Flags: review?(wmccloskey)
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.
Attachment #8348890 - Flags: review?(wmccloskey) → review?(luke)
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.
Attachment #8348890 - Flags: review?(luke) → review+
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.
Depends on: 951407
https://hg.mozilla.org/mozilla-central/rev/caf902c15026
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
(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.
Depends on: 960768
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.