These checks are done for toString and .parent, but not the others. So if someone gets hold of an exception thrown in code one is not supposed to have access to, information can be exfiltrated. This is biting us in bug 1107953.

Once this is fixed, the workarounds I'm adding in bug 1107953 may be able to be simplified. Maybe. We'd still need to deal with the caching that JSStackFrame does....

How would one get a SavedFrame if one doesn't have the credentials? I guess it doesn't hurt to add the checks.

> How would one get a SavedFrame if one doesn't have the credentials? Consider the case of a JS-implemented chrome API that is required per spec to throw a DOMException. When it creates the DOMException object (using a constructor on the chrome window), this object snapshots the stack; clearly the chrome code is on the stack at this point. Then this object is thrown to the content script, which can call APIs on it that call getters on the StackFrame. Now we _could_ have those APIs wrap the StackFrame into the compartment of the web page before calling the getter. However that would mean that .filename, say would throw instead of doing the right thing (that being, filter out the chrome frame where the DOMException was created and return the filename of the web page script that called into the chrome API).

I'm going to rate this sec-moderate in general, but depending on what information is leaked specific instances could be sec-high.

Right now we're working around the leaking with DOMExceptions that I described, so sec-moderate seems reasonable.

I thought that this would fix it, but I think we need XRays, as mentioned in bug 1036527. The test in the patch is failing. :bz, does the test assert the behavior that you want/expect?

The test tests the behavior I want, yes. And yes, we probably do need Xrays here. :(

Ok, this /should/ be working, however there's something weird going on here. The test in part 3 is reported as passing, even though execution halts in the middle: "FITZGEN: before" is logged, but "FITZGEN: after" is not, nor is there any error reported. The test just stops early and reports itself as passing. No idea what is going on with that.

Here is a log of the test running: https://pastebin.mozilla.org/8183239

Comment on attachment 8547857 [details] [diff] [review] Part 3: Implement xray support for SavedFrame :bz, any idea what is up with the test behavior I'm seeing in comment 12 and comment 13?

As far as I can tell, evalInSandbox is returning false without setting an exception on the cx. I tracked it down to CreateScriptSourceObject returning nullptr to CompileScript, but haven't dug deeper yet. Will try again tomorrow.

> evalInSandbox is returning false without setting an exception on the cx. Yeah, that would lead to the observed behavior: there would be no onerror callback to trigger a test failure.

Comment on attachment 8547853 [details] [diff] [review] Part 0: Define RootedGlobalObject in GlobalObject.h instead of jsscript.cpp This one really needs review from a JS engine peer.

Comment on attachment 8547854 [details] [diff] [review] Part 1: Make js::SavedFrame have a cached prototype and use js::ClassSpec This one too...

(In reply to Boris Zbarsky [:bz] from comment #21) > Comment on attachment 8547853 [details] [diff] [review] > Part 0: Define RootedGlobalObject in GlobalObject.h instead of jsscript.cpp Maybe define it in js/src/gc/Rooting.h, where all its (JS internal) siblings are?

Comment on attachment 8547856 [details] [diff] [review] Part 2: SavedFrame accessors should always check principals. OK. So key to using this is to make sure to not enter the SavedFrame compartment on the cx but instead to wrap the SavedFrame into the cx compartment (thus picking up xrays), right? >+ args.rval().setNull(); \ It's not clear to me that null is the best choice here. Would undefined be better? Apart from that, this looks good to me, though a JS peer should probably still look over it.

(In reply to Boris Zbarsky [:bz] from comment #24) > Comment on attachment 8547856 [details] [diff] [review] > Part 2: SavedFrame accessors should always check principals. > > OK. So key to using this is to make sure to not enter the SavedFrame > compartment on the cx but instead to wrap the SavedFrame into the cx > compartment (thus picking up xrays), right? Yes, although in the Debugger API we capture the SavedFrame instances in the debuggee compartment anwyays, so (at least in our case) I think entering the debuggee compartment would give you the view you're looking for as well. The big use case that I see for SavedFrame xrays is when we give a SavedFrame from a debuggee to the chrome debugger JS compartment: we can't just AutoCompartment from within JS :) > >+ args.rval().setNull(); \ > > It's not clear to me that null is the best choice here. Would undefined be > better? I used null because that is what we use when there aren't any more parents, but maybe that should be undefined too? ----------------------------------------------------------------------- I'm having a little trouble here. I can get the property descriptor from within the correct compartment with the correct prinicipals (restricted principals) just fine, but when the descriptors accessors/functions get called, it enters into the xpcshell test's compartment with the system principals and so by the time any of the SavedFrame methods get called its too late. I must need to wrap the descriptor's accessors/value so that it enters the target's compartment for the calls somehow, but I haven't figured out how to do that yet. It feels like this xray is pretty different from all the other xrays, but I'll be the first to admit that I don't have a deep understanding of xrays :-/

> I used null because that is what we use when there aren't any more parents For .parent, right? But for .filename and company, I'm not sure. > I think entering the debuggee compartment would give you the view you're looking for as > well Ah, fair. For what it's worth in my case I was doing something else, but then I realized that my SavedFrames are also being created in the content compartment... but while a non-content script is stack-top, which was what caused my issues. So I do need the principal checks for my stuff, but don't actually need Xrays because my objects are in the right compartment already... Bobby is probably the right one to talk to about the Xray issues.

(In reply to Boris Zbarsky [:bz] from comment #29) > > I used null because that is what we use when there aren't any more parents > > For .parent, right? But for .filename and company, I'm not sure. > > > I think entering the debuggee compartment would give you the view you're looking for as > > well > > Ah, fair. For what it's worth in my case I was doing something else, but > then I realized that my SavedFrames are also being created in the content > compartment... but while a non-content script is stack-top, which was what > caused my issues. So I do need the principal checks for my stuff, but don't > actually need Xrays because my objects are in the right compartment > already... Yes, I think part 3 would fix this for you, and xrays are only needed for when a chrome compartment is referencing a content SavedFrame and wants the content's view of the stack. I think we can land parts 1-3 independently of part 4, although I will have to think about how to test part 3. > Bobby is probably the right one to talk to about the Xray issues. Thanks!

For posterity, I drew up this diagram that illustrates the desired behavior.

(In reply to Nick Fitzgerald [:fitzgen] from comment #30) > Yes, I think part 3 would fix this for you, and xrays are only needed for > when a chrome compartment is referencing a content SavedFrame and wants the > content's view of the stack. > > I think we can land parts 1-3 independently of part 4, although I will have > to think about how to test part 3. Err, these patch numbers are all off by one.

Ok, parts 0-2 should address your needs :bz, and should be able to land-able on their own since I was able to write a test for part 2.

OK, there's definitely one case in which the null thing is wrong with these patches. Doing JS::ToString() on a stack that gets filtered out completely ends up returning the string "null". That seems pretty weird to me; I'd expect "". Other than that, parts 0-2 do in fact address my issues, thanks!

(In reply to Boris Zbarsky [:bz] from comment #36) > OK, there's definitely one case in which the null thing is wrong with these > patches. Doing JS::ToString() on a stack that gets filtered out completely > ends up returning the string "null". That seems pretty weird to me; I'd > expect "". > > Other than that, parts 0-2 do in fact address my issues, thanks! Ah, I probably need to make the default return value configurable in the THIS_SAVEDFRAME macro. The accessors could return null or undefined, and the toString method could return the empty string.

I've verified that parts 0-2 from bug 1122238 plus these patches still pass our tests for not exposing the JS-impleemnted webidl stack bits to content

Comment on attachment 8550109 [details] [diff] [review] part 2. Stop caching things in JSStackFrame when we're called over Xrays Gah, wrong bug.

I did a try push of my changes in bug 1122238 on top of patches 0-2 in this bug, and it looks like we do in fact need the xrays too: there are various devtools test failures due to not getting the expected stacks for DOMExceptions. :(

Comment on attachment 8549804 [details] [diff] [review] Part 0: Define RootedGlobalObject in gc/Rooting.h instead of jsscript.cpp Review of attachment 8549804 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. (There are actually a lot of Rooted<GlobalObject*>'s and Handle<GlobalObject*>'s in js/src. I'll file a separate bug to replace those after this lands; doesn't need to happen as part of this sec bug.) ::: js/src/gc/Rooting.h @@ +51,4 @@ > typedef JS::Rooted<PlainObject*> RootedPlainObject; > typedef JS::Rooted<ScriptSourceObject*> RootedScriptSource; > > + Nit: remove extra newline.

Comment on attachment 8549806 [details] [diff] [review] Part 1: Make js::SavedFrame have a cached prototype and use js::ClassSpec Review of attachment 8549806 [details] [diff] [review]: ----------------------------------------------------------------- Nice, r=me with nits addressed. ::: js/src/vm/SavedStacks.cpp @@ +624,2 @@ > return nullptr; > + RootedGlobalObject global(cx, &globalObj->as<GlobalObject>()); Nit: those 4 lines can be just: RootedGlobalObject global(cx, cx->global()); (Without a null check.) @@ +629,5 @@ > + return nullptr; > + RootedValue protov(cx, global->getPrototype(JSProto_SavedFrame)); > + if (!protov.isObject()) > + return nullptr; > + RootedObject proto(cx, &protov.toObject()); Can you add a getOrCreateSavedFramePrototype() to GlobalObject.h, for consistency with the other prototypes? Just copy/paste getOrCreateRegExpPrototype and s/JSProto_RegExp/JSProto_SavedFrame/. Then this becomes: RootedNativeObject proto(cx, GlobalObject::getOrCreateSavedFramePrototype(cx, global)); if (!proto) return nullptr;

Comment on attachment 8549922 [details] [diff] [review] Part 2: SavedFrame accessors should always check principals. Review of attachment 8549922 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, r=me with comments below addressed. ::: js/src/builtin/TestingFunctions.cpp @@ +959,5 @@ > + RootedObject stack(cx); > + { > + AutoCompartment ac(cx, targetCompartment); > + if (!JS::CaptureCurrentStack(cx, &stack, maxFrameCount)) > + return false; Can this function return true and return a nullptr stack? If yes, the wrap() call below should be changed to: if (stack && !cx->...) return false; If no, it'd be simpler to return a JSObject* (nullptr on failure), to match other APIs. In this case the call below can also be setObject instead of setObjectOrNull. ::: js/src/vm/SavedStacks.cpp @@ +351,4 @@ > } > > + frame.set(GetFirstSubsumedFrame(cx, &thisObject.as<SavedFrame>())); > + return true; This can potentially return an entirely different SavedFrame* instance, and we'll then use that. I think it should be something like this: frame.set(GetFirstSubsumedFrame(cx, &thisObject.as<SavedFrame>())); if (frame && frame != subsumed) frame.set(nullptr); return true; Or something similar to ensure we either return |this| or nullptr.

(In reply to Jan de Mooij [:jandem] from comment #44) > frame.set(GetFirstSubsumedFrame(cx, &thisObject.as<SavedFrame>())); > if (frame && frame != subsumed) > frame.set(nullptr); > return true; Er, if (frame != thisObject)

(In reply to Nick Fitzgerald [:fitzgen] from comment #37) > Ah, I probably need to make the default return value configurable in the > THIS_SAVEDFRAME macro. The accessors could return null or undefined, and the > toString method could return the empty string. Please add tests for all these cases :)

ni? me to land parts 0-2 once inbound is open again.

Ok, so I just talked with :bholley. First, we need to CallNonGenericMethod'ize the SavedFrame accessors/methods. Next, we need to be able to pass the principals from the native accessors/methods to their impls. Rather than change a gazillion function signatures to take and pass through and ignore a void*, or templatize the CallNonGenericMethod/nativeCall pipeline and cause a bunch of code bloat, we observe that none of the SavedFrame accessors/methods use the callee value and therefore we can store a PrivateValue in its stead. This PrivateValue can be a pointer to the original caller's principals, thereby successfully shuffling the original caller's principals to the implementations.

Comment on attachment 8549892 [details] diagram Note that I got the xrayed and waived cases backwards under "Desired Results". The test in patch #4 and docs in patch #5 is the correct behavior.

Comment on attachment 8554755 [details] [diff] [review] Part 4: Implement xray support for SavedFrame Review of attachment 8554755 [details] [diff] [review]: ----------------------------------------------------------------- Beautiful. r=bholley ::: js/xpconnect/tests/unit/test_xray_SavedFrame.js @@ +30,5 @@ > + > + mid.highF = () => high.highF(); > + Cu.evalInSandbox("function midF() { return highF(); }", mid); > + > + low.midF = () => mid.midF(); These are going to create intermediate system-principaled frames, right? @@ +59,5 @@ > + "Xrays should always be able to see everything."); > + > + const waived = Cu.waiveXrays(xrayStack); > + equal(waived.functionDisplayName, name, > + "The waived wrapper should give us the stack's compartment's view."); Can you also walk the chain of SavedStacks here and make sure it looks like what we expect? @@ +68,5 @@ > +// > +// We can't use Cu.getJSTestingFunctions().saveStack() because Cu isn't > +// available to sandboxes that don't have the system principal. The easiest way > +// to get the SavedFrame is to use the Debugger API to track allocation sites > +// and then do an allocation. Clever.

Comment on attachment 8554750 [details] [diff] [review] Part 2: SavedFrame accessors should always check principals. Review of attachment 8554750 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/SavedStacks.cpp @@ +317,5 @@ > + JSSubsumesOp subsumes = cx->runtime()->securityCallbacks->subsumes; > + JSPrincipals *principals = cx->compartment()->principals; > + > + while (frame && principals && subsumes && !subsumes(principals, frame->getPrincipals())) > + frame = frame->getParent(); I get that when "cx->runtime()->securityCallbacks->subsumes" is null we should skip all security checks, and the same goes for when "compartment()->principals" is null. I can't find this documented, however. Maybe in this function you can check those for null first instead of in the loop, and add a comment to that effect?

Comment on attachment 8554751 [details] [diff] [review] Part 3: Make SavedFrame accessors and methods use CallNonGenericMethod. Review of attachment 8554751 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review for the interaction between 'callee' thwacking and error handling. But the rest looks great. ::: js/src/vm/SavedStacks.cpp @@ +286,5 @@ > "SavedFrame"); > return false; > } > > +// Get the first frame whose principals are subsumed by the given principals. Let's improve this comment: Return the first SavedFrame in the chain that starts with |frame| whose principles are subsumed by |principals|, according to |subsumes|. If there is no such frame, return nullptr. @@ +294,1 @@ > while (frame && principals && subsumes && !subsumes(principals, frame->getPrincipals())) Pre-existing nit, but: isn't it kind of weird to test things in the loop condition that don't vary as we iterate? I'd just check principals and subsumes up front, and if either are null, return frame immediately. It's not about efficiency (the compiler can see better than me that they're invariant), but more about using idioms to set the reader's expectations helpfully: my first reaction was, "wait, do those change?" Also, from looking around it seems like 'nullptr' is actually a valid principal, depending on the "subsumes" function you use. Things like JS_SetCompartmentPrincipals are prepared for nullptrs. And I think it's left up to the embedding what the null principal means. So I think this code should not test whether principals is null at all, and just use whatever it's given. @@ +303,5 @@ > +// principals. Nor do we want to add a void* parameter to CallNonGenericMethod > +// and every single function called from it, or templatize them, causing code > +// bloat. With the observation that none of the SavedFrame accessors or methods > +// use arguments.callee, we can stuff a PrivateValue containing the original > +// caller's principals on the stack where the callee otherwise would be. Great comment. People can think whatever they like about the code, but certainly everyone will understand what it's doing. It looks to me like, in the case where 'this' is some inappropriate object or value, CallMethodIfWrapped uses ReportIncompatible, which actually consults the callee to produce a name to use in the JSMSG_INCOMPATIBLE_METHOD message. So since we call SavePrincipalsInCallee just before CallNonGenericMethod, we're going to wipe out the callee and get JSMSG_NOT_FUNCTION instead of JSMSG_INCOMPATIBLE_METHOD. That seems unfortunate. It seems to me we could overload CallNonGenericMethod and CallMethodIfWrapped such that if the impl happens to expect an additional JSObject *, they pass the original 'this'. That wouldn't disturb any of the other CallNonGenericMethod uses. @@ +339,3 @@ > } > > +/* static */ MOZ_ALWAYS_INLINE bool Let's avoid the MOZ_ALWAYS_INLINE annotations here, too. @@ +406,5 @@ > +/* static */ MOZ_ALWAYS_INLINE bool > +SavedFrame::parentImpl(JSContext *cx, CallArgs args) > +{ > + THIS_SAVEDFRAME_AND_PRINCIPALS(cx, args, NullValue(), frame, principals); > + auto *subsumes = cx->runtime()->securityCallbacks->subsumes; Do we need the '*'? (I've never used 'auto'...) ::: js/src/vm/SavedStacks.h @@ +57,5 @@ > > class AutoLookupRooter; > class HandleLookup; > > + static MOZ_ALWAYS_INLINE bool Is there some reason this absolutely must be inlined? I think I'd prefer to leave MOZ_ALWAYS_INLINE to the cases where we know we have a problem that it... "fixes". The CallNonGenericMethod wrapper talks about inlining because it's used for some very heavily used primitives. That's not the case here. Also, simply putting a function's body inside the class definition automatically makes it inline, as if you'd written the 'inline' keyword. @@ +63,5 @@ > + { > + if (!v.isObject()) > + return false; > + > + JSObject &thisObject = v.toObject(); Can this be 'const JSObject'?

(In reply to Jim Blandy :jimb from comment #61) > @@ +303,5 @@ > > +// principals. Nor do we want to add a void* parameter to CallNonGenericMethod > > +// and every single function called from it, or templatize them, causing code > > +// bloat. With the observation that none of the SavedFrame accessors or methods > > +// use arguments.callee, we can stuff a PrivateValue containing the original > > +// caller's principals on the stack where the callee otherwise would be. > > Great comment. People can think whatever they like about the code, but > certainly everyone will understand what it's doing. > > It looks to me like, in the case where 'this' is some inappropriate object > or value, CallMethodIfWrapped uses ReportIncompatible, which actually > consults the callee to produce a name to use in the > JSMSG_INCOMPATIBLE_METHOD message. So since we call SavePrincipalsInCallee > just before CallNonGenericMethod, we're going to wipe out the callee and get > JSMSG_NOT_FUNCTION instead of JSMSG_INCOMPATIBLE_METHOD. That seems > unfortunate. > > It seems to me we could overload CallNonGenericMethod and > CallMethodIfWrapped such that if the impl happens to expect an additional > JSObject *, they pass the original 'this'. That wouldn't disturb any of the > other CallNonGenericMethod uses. Will try this out and report back. > @@ +339,3 @@ > > } > > > > +/* static */ MOZ_ALWAYS_INLINE bool > > Let's avoid the MOZ_ALWAYS_INLINE annotations here, too. I was just cribbing off of the `Date` implementation; can remove it. > @@ +406,5 @@ > > +/* static */ MOZ_ALWAYS_INLINE bool > > +SavedFrame::parentImpl(JSContext *cx, CallArgs args) > > +{ > > + THIS_SAVEDFRAME_AND_PRINCIPALS(cx, args, NullValue(), frame, principals); > > + auto *subsumes = cx->runtime()->securityCallbacks->subsumes; > > Do we need the '*'? (I've never used 'auto'...) I believe so. The `auto` keyword follows the same type resolution algorithm that templates use, so you generally have to add '*', '&', and '&&' yourself. Alternatively, I can just add the type explicitly if you prefer. > ::: js/src/vm/SavedStacks.h > @@ +57,5 @@ > > > > class AutoLookupRooter; > > class HandleLookup; > > > > + static MOZ_ALWAYS_INLINE bool > > Is there some reason this absolutely must be inlined? I think I'd prefer to > leave MOZ_ALWAYS_INLINE to the cases where we know we have a problem that > it... "fixes". > > The CallNonGenericMethod wrapper talks about inlining because it's used for > some very heavily used primitives. That's not the case here. > > Also, simply putting a function's body inside the class definition > automatically makes it inline, as if you'd written the 'inline' keyword. Again, simply cribbing from existing `Date` code.

Okay, thanks very much to all for a helpful Vidyo conversation. Here's my writeup of our conclusions: The situation in which `SavedFrame` finds itself is fortunately similar to that of DOM objects, so we can re-use their solution, which takes advantage of the difference between invoking a method on an xray wrapper and invoking a method on a CCW. When we invoke a method on a CCW, the CCW's prototype chain is not even consulted. Instead, we enter the referent's compartment, rewrap `this` and the arguments as appropriate for that compartment, and then carry out the method application normally for the new `this`. The referent's compartment provides the prototype chain and the function actually invoked. In contrast, when we invoke a method on an Xray wrapper, no automatic compartment switch takes place. We consult the wrapper's own prototype chain, as built by the wrapper machinery in the wrapper's compartment. This prototype chain is populated with methods as directed by the `JSFunctionSpecs` referred to by the referent's `JSClass`'s `ClassSpec`. These methods receive the wrapper as their `this` value, and the arguments are presented exactly as passed to the method call on the wrapper. ------------------------------------------------------------------------------------ operation CCW Xray ----------------- ------------------------------- ---------------------------------- method called in referent compartment, in wrapper's compartment, found on referent's prototype found on wrapper's prototype chain chain cx's compartment referent's compartment wrapper's compartment 'this' value referent wrapper arguments rewrapped for referent as passed to method call on wrapper compartment ------------------------------------------------------------------------------------- (The `CallNonGenericMethod` C++ function is designed for use by native functions that need to behave in either one of the above manners, depending on which sort of `this` value they receive. This means that `CallNonGenericMethod` must be non-partisan with respect to the above table: it simply passes control to the wrapper's `callNative` handler, and the handler gets to decide whether to switch compartments, rewrap, and so on.) In the case of `SavedFrame`'s methods and accessors: - When invoked on an actual `SavedFrame` instance, we wish to present the stack as visible to the instance's principals (that is, the instance's compartment's principals). In a content compartment, this would mean omitting chrome frames. - When invoked on a CCW of a `SavedFrame` instance, we wish to present the stack as visible to the CCW's referent's principals. For example, if chrome is referring to a `SavedFrame` in content via a CCW, it should not see chrome frames. This is in keeping with the behavior outlined above: calls on CCWs switch compartments, and then carry out the call as appropriate for that compartment. - When invoked on an Xray wrapper of a `SavedFrame` instance, we wish to present the stack as visible to the *wrapper*'s principals. For example, if chrome is referring to a `SavedFrame` in content via an Xray wrapper, it should see both chrome frames and content frames. To this end: - `SavedFrame`'s `ClassSpec`'s `JSFunctionSpecs` should cite C++ functions that use `CallNonGenericMethod` in the usual way. - The `NativeImpl`s passed to `CallNonGenericMethod` should follow these steps: 1. Note the principals of the given `this` value. 2. Call `CheckedUnwrap` on `this`, and complain if the result is not a `SavedFrame`. 3. Use the noted principals to constrain operations on the unwrapped `this`.

(In reply to Jim Blandy :jimb from comment #63) > - `SavedFrame`'s `ClassSpec`'s `JSFunctionSpecs` should cite C++ functions > that > use `CallNonGenericMethod` in the usual way. I don't think this is right. The problem is that CallNonGenericMethod lands us in the target compartment, at which point the principals aren't what we want - we basically need to _not_ use CallNonGenericMethod, and instead reimplement what it does using CheckedUnwrap instead of nativeCall.

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #72) > (In reply to Jim Blandy :jimb from comment #63) > > - `SavedFrame`'s `ClassSpec`'s `JSFunctionSpecs` should cite C++ functions > > that > > use `CallNonGenericMethod` in the usual way. > > I don't think this is right. The problem is that CallNonGenericMethod lands > us in the target compartment, at which point the principals aren't what we > want - we basically need to _not_ use CallNonGenericMethod, and instead > reimplement what it does using CheckedUnwrap instead of nativeCall. To expand on why we don't need to use CallNonGenericMethod: Xray wrapper case: because part 5 enables xrays for SavedFrame, we don't automatically enter the callee's compartment when calling the SavedFrame js natives. The cx has the caller's compartment, which has the principals we want to check against. CCW case: you've waived xrays and want the callee's view of the SavedFrame. Now the cx will automatically enter the callee's compartment when calling the SavedFrame js natives and the cx->compartment()->principals is the callee's principals, so checking against that is what we want. No wrapper case: we are already in the callee's compartment, cx->compartment()->principals is what we should check against. In every case, cx->compartment()->principals is what we want to check against. All we need to do is make sure to CheckedUnwrap down to the underlying SavedFrame (part 4).

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #72) > I don't think this is right. The problem is that CallNonGenericMethod lands > us in the target compartment, at which point the principals aren't what we > want - we basically need to _not_ use CallNonGenericMethod, and instead > reimplement what it does using CheckedUnwrap instead of nativeCall. This sounds right to me. (With the quibble that it's not CallNonGenericMethod per se that switches compartments, but specific proxy handlers' nativeCall methods.) I am still a little uncomfortable about "going around" nativeCall, because it seems like a violation of the promise we make about proxies: that the handlers get a chance to intercept operations on the proxy's referent. Calling CheckedUnwrap ourselves does seem like the right thing for wrappers; and I guess we don't care about applying those methods and accessors to other kinds of proxies?

Comment on attachment 8559491 [details] [diff] [review] Part 4: SavedFrame::checkThis should unwrap the this value Review of attachment 8559491 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, with one question. ::: js/src/vm/SavedStacks.cpp @@ +337,5 @@ > JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_NOT_NONNULL_OBJECT); > return false; > } > > + JSObject *thisObject = CheckedUnwrap(&thisValue.toObject()); How does this not assert when we pass an Xray, and fetch its parent? The StackFrame we're returning is in the referent's compartment, not the wrapper's, but we never switched compartments. This apparently passes the tests, but I don't really understand why.

(In reply to Jim Blandy :jimb from comment #75) > Comment on attachment 8559491 [details] [diff] [review] > Part 4: SavedFrame::checkThis should unwrap the this value > > Review of attachment 8559491 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks great, with one question. > > ::: js/src/vm/SavedStacks.cpp > @@ +337,5 @@ > > JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_NOT_NONNULL_OBJECT); > > return false; > > } > > > > + JSObject *thisObject = CheckedUnwrap(&thisValue.toObject()); > > How does this not assert when we pass an Xray, and fetch its parent? The > StackFrame we're returning is in the referent's compartment, not the > wrapper's, but we never switched compartments. > > This apparently passes the tests, but I don't really understand why. Good catch, I didn't originally think of that. After some digging as to why the test which exercises this path doesn't assert, it seems the xray machinery automatically re-wraps return values. Thanks, xray machinery!

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4414e9d0b66b remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2f996950fb2f remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/86421767cd26 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ccf6dfe1ac75 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/442d41779bd8 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b3f8122dd990 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/694f7ac58964

Backed out for various mochitest failures. https://hg.mozilla.org/integration/mozilla-inbound/rev/3c88c70e1e64 https://treeherder.mozilla.org/logviewer.html#?job_id=6356801&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=6360578&repo=mozilla-inbound

And a new hazard. Function '_ZN2jsL21GetFirstSubsumedFrameEP9JSContextPNS_10SavedFrameE|SavedStacks.cpp:js::SavedFrame* js::GetFirstSubsumedFrame(JSContext*, js::SavedFrame*)' has unrooted 'frame' of type 'js::SavedFrame*' live across GC call *subsumes at js/src/vm/SavedStacks.cpp:324 js/src/vm/SavedStacks.cpp:324: Call(10,11, __temp_4 := subsumes*(principals*,__temp_5*)) [[GC call]] js/src/vm/SavedStacks.cpp:324: Assume(11,13, !__temp_4*, false) js/src/vm/SavedStacks.cpp:324: Assign(13,14, __temp_3 := 0) js/src/vm/SavedStacks.cpp:324: Assume(14,15, __temp_3*, false) js/src/vm/SavedStacks.cpp:327: Assign(15,16, return := frame*)

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1c19ad13666

All of the remaining test failures appear to be related to DOMException's stacks.

I think we need to waive xrays or enter the appropriate compartment in the DOMException code, but I'm not familiar with this part of the code base. Any suggestions bz?

The DOMException code at the moment enters the compartment of the StackFrame object before doing anything with it, I believe. Looking at the failures, there's the fact that the exception from testPromiseWithDOMExceptionThrowingPromiseInit is not working right somehow, plus the test timeout from browser/devtools/webconsole/test/browser_webconsole_notifications.js, right? I'm happy to take a look at what happens with the former if you want to throw me a single patch I can apply locally.

(In reply to Boris Zbarsky [:bz] from comment #84) > The DOMException code at the moment enters the compartment of the StackFrame > object before doing anything with it, I believe. > > Looking at the failures, there's the fact that the exception from > testPromiseWithDOMExceptionThrowingPromiseInit is not working right somehow, > plus the test timeout from > browser/devtools/webconsole/test/browser_webconsole_notifications.js, right? > > I'm happy to take a look at what happens with the former if you want to > throw me a single patch I can apply locally. Thanks, very appreciated. I mistakenly believed that the webconsole test was testing an exception line number. Looking more into this failure.

Thanks. Though you might want to change the diff setup to not output those terminal control bits when piping to file. ;) So what happens is that this patch is interacting badly with the workaround we have in place for this bug. In particular, the chrome code throws a content-side DOM exception in this case. This has the stack including chrome stuff, which we didn't want to expose. In particular, the first SavedFrame points to a chrome frame, its parent points to a contnet frame, and there is nothing else on the stack. So we end up calling DOMException::Sanitize to sanitize the stack (that's the workaround). retval->mLocation->CallerSubsumes(aCx) is false (that's the chrome frame), so we do: nsCOMPtr<nsIStackFrame> caller; retval->mLocation->GetCaller(getter_AddRefs(caller)); retval->mLocation.swap(caller); This _used_ to get us the content frame because GetCaller does a property get for .parent on the SavedFrame. But with this patch, doing the .parent sees that we're pointing to a chrome frame we don't subsume, walks up past it to the content frame, then does .parent on that (which is null, because there is no more stack) and returns that. So we get null instead of the thing we were expecting. This workaround will go away in bug 1122238 but we need to fix bug 1031152 first.... So what we need for the moment, I think, is a way to do the "walk up to the first stackframe we subsume but no further" bit, for use in DOMException::Sanitize.

(In reply to Boris Zbarsky [:bz] from comment #87) > This workaround will go away in bug 1122238 but we need to fix bug 1031152 > first.... Which, of course, depends on this bug. Gah! > So what we need for the moment, I think, is a way to do the "walk up to the > first stackframe we subsume but no further" bit, for use in > DOMException::Sanitize. So basically expose GetFirstSubsumedFrame from part 2 via jsfriendapi but with JSObject * instead of SavedFrame *?

Yes, I think that would work.

Comment on attachment 8562430 [details] [diff] [review] Part 8: Expose an API to get the first SavedFrame object in a saved stack whose principals are subsumed by the given JSContext's. Seems OK, though the API should document whether cx and savedFrame need to be same-compartment.

(In reply to Boris Zbarsky [:bz] from comment #91) > Comment on attachment 8562430 [details] [diff] [review] > Part 8: Expose an API to get the first SavedFrame object in a saved stack > whose principals are subsumed by the given JSContext's. > > Seems OK, though the API should document whether cx and savedFrame need to > be same-compartment. So what is our plan for landing these changes? Land the DOMException::Sanitize fixes at the same time as these patches? I've been digging into the console failures, and I'd like to try again with your DOMException::Sanitize changes. We save a stack via the DOMException stack saving API for console messages[0] and while I haven't narrowed it down to this code yet, I have a feeling it is related. I ended my day yesterday with observing that the test failure came down to a console.log call not resulting in notifying console-api-log-event observers. [0] https://dxr.mozilla.org/mozilla-central/source/dom/base/Console.cpp#982

> So what is our plan for landing these changes? Well, you need one more patch, which is to make DOMException::Sanitize use the API part 8 adds, right? Are you writing that, or do you want me to? And then yes, we land it together with this other stuff. Ordering probably doesn't matter too much if we're ok with these tests being orange during a bisect... Testing the console again once we fix DOMException::Sanitize makes some sense, though I doubt that code is hit in the console.

(In reply to Boris Zbarsky [:bz] from comment #93) > > So what is our plan for landing these changes? > > Well, you need one more patch, which is to make DOMException::Sanitize use > the API part 8 adds, right? Are you writing that, or do you want me to? Yes, we need that patch. I was under the impression that you were going to write it, but I can write it if you're busy. > And then yes, we land it together with this other stuff. Ordering probably > doesn't matter too much if we're ok with these tests being orange during a > bisect... We can always squash this into one big patch to preserve bisect-ability.

If you can write it, please do. I can do it if needed, but I have a bunch of other stuff on my plate right now...

Sure thing.

Comment on attachment 8562430 [details] [diff] [review] Part 8: Expose an API to get the first SavedFrame object in a saved stack whose principals are subsumed by the given JSContext's. Review of attachment 8562430 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/SavedStacks.cpp @@ +331,5 @@ > +JS_FRIEND_API(JSObject *) > +GetFirstSubsumedSavedFrame(JSContext *cx, HandleObject savedFrame) > +{ > + MOZ_ASSERT(savedFrame->is<SavedFrame>()); > + RootedSavedFrame frame(cx, &savedFrame->as<SavedFrame>()); Nit: you can remove the MOZ_ASSERT; as<T>() asserts is<T>(). @@ +333,5 @@ > +{ > + MOZ_ASSERT(savedFrame->is<SavedFrame>()); > + RootedSavedFrame frame(cx, &savedFrame->as<SavedFrame>()); > + frame = GetFirstSubsumedFrame(cx, frame); > + return frame; Nit: return GetFirstSubsumedFrame(cx, frame);

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe1e91d45390

Comment on attachment 8563067 [details] [diff] [review] Part 9: Remove sanitization principal checks in DOM exceptions that are now handled by SavedFrame No, we need to keep sanitizing until the caching bits go away. What you want t replace is the loop and GetCaller() bit with something that instead just uses the part 8 API.

(In reply to Boris Zbarsky [:bz] from comment #101) > Comment on attachment 8563067 [details] [diff] [review] > Part 9: Remove sanitization principal checks in DOM exceptions that are now > handled by SavedFrame > > No, we need to keep sanitizing until the caching bits go away. What you > want t replace is the loop and GetCaller() bit with something that instead > just uses the part 8 API. How can I get the SavedFrame object in JSStackFrame::mStack from an nsIStackFrame?

I think you just want to add a new, probably noscript, API on nsIStackFrame, like .caller but maybe called .sanitized. JSStackFrame will do what JSStackFrame::GetCaller does except with the passed-in context and using the new API you added instead of getting the "parent" property. StackFrame will just return null.

(In reply to Nick Fitzgerald [:fitzgen] from comment #102) > How can I get the SavedFrame object in JSStackFrame::mStack from an nsIStackFrame? Not sure if relevant to what you need here, but I tentatively added a nativeSavedFrame getter in attachment 8551839 [details] [diff] [review] for bug 1083359, in order to support Components.utils.callFunctionWithStack. Boris, Nick, I encourage you to look at the patch queue in that bug at the architectural level to see if there is anything that can or should be changed, based on what you need in this bug. Jim should be getting to the review soon but apparently couldn't find the time yet.

Another try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd12dc9bf087

Comment on attachment 8563648 [details] [diff] [review] Part 9: Add nsIStackFrame.sanitized getter and use it in DOMException::Sanitize >- if (!JS_GetProperty(cx, stack, "source", &filenameVal) || >- !filenameVal.isString()) { >+ if (!JS_GetProperty(cx, stack, "source", &filenameVal)) { Are we guaranteed that "source" is never null or anything like that? Seems like this change is not actually related to .sanitized, so I'd prefer it happened in a separate bug. >- if (!JS_GetProperty(cx, stack, "line", &lineVal) || And this one too (though here you _are_ checking for lineVal not being isNumber())? >- if (!JS_GetProperty(cx, stack, "column", &colVal) || And this. >+NS_IMETHODIMP StackFrame::GetSanitized(nsIStackFrame **aSanitized) >+ *aSanitized = nullptr; I think this should be: NS_ADDREF(*aSanitized = this); to keep the old behavior. >+ savedFrame = js::GetFirstSubsumedSavedFrame(cx, savedFrame); Please add a comment about how it's extremely important to not enter the compartment of the savedFrame here? Because afaict it is. That said, I'd also really prefer we passed aCx in explicitly here instead of using ThreadsafeAutoJSContext. You should be able to do that by marking the method as [implicit_jscontext] in the IDL. >+ if (!savedFrame) { >+ *aSanitized = nullptr; NS_ADDREF(*aSanitized = new StackFrame()); >+ if (!stackFrame) { >+ return NS_ERROR_OUT_OF_MEMORY; >+ } The default operator new in Gecko is infallible, so this whole block should go away. So actually, maybe like this: nsCOMPtr<nsIStackFrame> stackFrame; if (savedFrame) { stackFrame = new JSStackFrame(savedFrame); } else { stackFrame = new StackFrame(); } stackFrame.forget(aSanitized); r=me modulo the above

(In reply to Boris Zbarsky [:bz] from comment #107) > Comment on attachment 8563648 [details] [diff] [review] > Part 9: Add nsIStackFrame.sanitized getter and use it in > DOMException::Sanitize > > >- if (!JS_GetProperty(cx, stack, "source", &filenameVal) || > >- !filenameVal.isString()) { > >+ if (!JS_GetProperty(cx, stack, "source", &filenameVal)) { > > Are we guaranteed that "source" is never null or anything like that? Seems > like this change is not actually related to .sanitized, so I'd prefer it > happened in a separate bug. Without these changes, the console test fails because the GetFilename etc calls in Console.cpp:875.. fail if we don't make JSStackFrame understand that these properties can all be null if there are no content frames in the stack. It is safe not to check the value type in this case because nsAutoJSString will call ToString on the value.

> It is safe not to check the value type in this case because nsAutoJSString will call > ToString on the value. Hmm. But ToString(null) is "null". Don't we want "" instead in this case?

Also, can we add MOZ_ASSERTS about lineVal/colVal being isNull() when not isNumber()?

(In reply to Boris Zbarsky [:bz] from comment #109) > > It is safe not to check the value type in this case because nsAutoJSString will call > > ToString on the value. > > Hmm. But ToString(null) is "null". Don't we want "" instead in this case? I can change the behavior to match this.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fcf76430064

Comment on attachment 8563741 [details] [diff] [review] Part 9: Add nsIStackFrame.sanitized getter and use it in DOMException::Sanitize >- if (!str.init(cx, filenameVal.toString())) { >+ if (!str.init(cx, filenameVal)) { Can keep the toString() here, since we know it's isString(). Looks great, thank you!

And https://hg.mozilla.org/integration/mozilla-inbound/rev/fbf882d57a13 to rev the nsIStackFrame IID and fix the dep build issues that were happening.

https://hg.mozilla.org/mozilla-central/rev/c5b1c517ad63 https://hg.mozilla.org/mozilla-central/rev/fbf882d57a13