I don't have a testcase right now, but I ran into this working on another bug. Basically, Policy::check functions are responsible for setting their own exceptions if they deny access.

I think the change to AccessCheck.cpp makes the control flow a little clearer (and it nukes a couple of braces). It's a little unfortunate that this patch actually makes LocationPolicy::check less clear (since the exception for PUNCTURE is "hidden" in a larger boolean expression) but it's the easiest way to share the code that throws the exception.

Comment on attachment 621016 [details] [diff] [review] Proposed fix >--- a/js/xpconnect/wrappers/AccessCheck.cpp >+++ b/js/xpconnect/wrappers/AccessCheck.cpp >@@ -494,12 +494,10 @@ ExposedPropertiesOnly::check(JSContext *cx, JSObject *wrapper, jsid id, Wrapper: > perm = PermitObjectAccess; > return true; > } >- if (act == Wrapper::PUNCTURE) { >- perm = DenyAccess; >- return false; >- } > > perm = DenyAccess; >+ if (act == Wrapper::PUNCTURE) >+ return false; Doesn't this have the same issue?

Oops, yes.

Comment on attachment 621024 [details] [diff] [review] Proposed fix v2 Can we add some tests for this? I'm about to touch this code in a couple of ways (fixing __exposedProps__, and removing universalXPConnect), and this seems like the sort of thing I might regress.

So ... can we land this?

I think so, yes.

Comment on attachment 621024 [details] [diff] [review] Proposed fix v2 https://hg.mozilla.org/integration/mozilla-inbound/rev/8d752d1b621b