After bug 760109 landing, content still can access properties of a standard prototype from a chrome scope if __exposedProps__ doesn't exist. 67 ChromeObjectWrapper::get(JSContext *cx, JSObject *wrapper, JSObject *receiver, 68 jsid id, js::Value *vp) 69 { 70 // Try the lookup on the base wrapper. 71 if (!ChromeObjectWrapperBase::get(cx, wrapper, receiver, id, vp)) 72 return false; 73 74 // If we found something or have no proto, we're done. 75 JSObject *wrapperProto = JS_GetPrototype(wrapper); 76 if (!vp->isUndefined() || !wrapperProto) 77 return true; ChromeObjectWrapperBase::get lookups the chrome object's prototype chain and can find something on a standard prototype from the chrome scope.

After bug 760109 landing: o.s1: chrome o.s2: content p.s1: content p.s2: content Before bug 760109 landing: o.s1: chrome o.s2: undefined p.s1: chrome p.s2: undefined

This does a similar thing to what the testcase in bug 768101 comment 0 does. This works on fx14-17 but does not work on fx10.

Ah, crap. The code isn't broken per se, but the issue that moz_bug_r_a4 pointed out means that I was mistaken to suggest it as a solution to bug 768101. It will do the right thing once __exposedProps__ starts being mandatory, but that's only going to happen this cycle. In order to get the behavior I was imagining, we'll need bug 778085, which offloads the duties of ChromeObjectWrapper into the JS engine and lets us remove ChromeObjectWrapper.{cpp,h} entirely. Those patches will be landing on m-i tomorrow probably once Eddy and I work out a few details.

Tracking this bug for 15, though we are going to final beta now, but if there is a chemspill we can look at taking this along. Also tracking ? for ESR until this lands and we can approve for the appropriate ESR at the time. This is the rest of the work started/landed in bug 760109.

I have patches for this. Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=3d029a5f6c1a

These are green except for one bug, which I've fixed and verified it fixes the oranges in that try push. Uploading patches and flagging blake for review.

Not sure what I was thinking before. We should always be in the compartment of the wrapper here.

Comment on attachment 654780 [details] [diff] [review] Part 1 - Clarify the compartment situation in ChromeObjectWrapper. v1 Review of attachment 654780 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/wrappers/ChromeObjectWrapper.cpp @@ +57,1 @@ > { Nit: the braces go away here.

Comment on attachment 654781 [details] [diff] [review] Part 2 - Remap objects from standard prototypes even if they're explicitly exposed. v2 Review of attachment 654781 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/tests/unit/test_bug780370.js @@ +12,5 @@ > +function run_test() > +{ > + var sb = Cu.Sandbox("http://www.example.com"); > + sb.obj = { foo: 42, __exposedProps__: { hasOwnProperty: 'r' } }; > + do_check_eq(Cu.evalInSandbox('typeof obj.foo', sb), 'undefined', "COW works as expected"); Is it worth adding an Object.prototype.bar in the sandbox and ensuring that the Object.prototype being used is the sandbox's? ::: js/xpconnect/wrappers/ChromeObjectWrapper.cpp @@ +69,5 @@ > > // Try the prototype if that failed. > JS_ASSERT(js::IsObjectInContextCompartment(wrapper, cx)); > JSPropertyDescriptor desc; > + memset(&desc, 0, sizeof(desc)); Why are these memsets needed (here and below). @@ +84,5 @@ > jsid id, js::Value *vp) > { > + // Start with a call to getPropertyDescriptor. We unfortunately need to do > + // this because the call signature of ::get doesn't give us any way to > + // determine the object upon which the property was found. Yeah, this is pretty ugly. One way to fix it would be to expose the guts of BaseProxyHandler::get as a protected function that takes a JSPropertyDescriptor and gets the value using it. For now, I think this is fine, since the double protochain walk isn't detectable (and should be fast enough anyway). If we end up allowing __exposedProps__ on objects on the proto chain though, it might be worth it to clean this up.

(In reply to Blake Kaplan (:mrbkap) from comment #10) > Is it worth adding an Object.prototype.bar in the sandbox and ensuring that > the Object.prototype being used is the sandbox's? Added. > > Why are these memsets needed (here and below). Discussed IRL. > Yeah, this is pretty ugly. One way to fix it would be to expose the guts of > BaseProxyHandler::get as a protected function that takes a > JSPropertyDescriptor and gets the value using it. For now, I think this is > fine, since the double protochain walk isn't detectable (and should be fast > enough anyway). If we end up allowing __exposedProps__ on objects on the > proto chain though, it might be worth it to clean this up. I'll file a bug when I get back from vacation.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/20a3b0eeb776 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c766aa050f0f

Updating tracking flags, we're look at 16 for this nom for beta/ESR when ready.

(In reply to Bobby Holley (:bholley) (on vacation Aug 24 - Sept 3) from comment #12) > remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/20a3b0eeb776 > remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c766aa050f0f https://hg.mozilla.org/mozilla-central/rev/20a3b0eeb776 https://hg.mozilla.org/mozilla-central/rev/c766aa050f0f

Does the patch apply to ESR?

Comment on attachment 654781 [details] [diff] [review] Part 2 - Remap objects from standard prototypes even if they're explicitly exposed. v2 Back from vacation. Looks like this stuck on m-c (which has now also branched to m-a), so it's time to get this onto beta and esr10. This patch isn't super risky. The bulk of the work happened over in bug 760109, which has now landed everywhere. This just tweaks it a bit. No string or IID changes.

Pushed to m-b: remote: https://hg.mozilla.org/releases/mozilla-beta/rev/647876f95fea remote: https://hg.mozilla.org/releases/mozilla-beta/rev/d9a8bb8a5533

Pushed to mozilla-esr10: remote: https://hg.mozilla.org/releases/mozilla-esr10/rev/f61385d44144 remote: https://hg.mozilla.org/releases/mozilla-esr10/rev/b9ee5ec9d172

mass remove verifyme requests greater than 4 months old