See https://www.w3.org/Bugs/Public/show_bug.cgi?id=20701. This is a moving target at the moment, but I want to have a bug on file for my tentative efforts to implement the new behavior.

For those following and being lazy about reading all https://www.w3.org/Bugs/Public/show_bug.cgi?id=20701 , the most recent state of affairs is this etherpad with prose: https://etherpad.mozilla.org/html5-cross-origin-objects Tentative test suite (as a patch to W3C's web-platform-tests) https://www.w3.org/Bugs/Public/attachment.cgi?id=1433 Changing URL to start at comment 128 as it's probably more relevant (and maybe less scary)

Here's my WIP: https://github.com/bholley/gecko-dev/tree/xospecalign I've fixed all the stuff I can that doesn't require moving these objects to WebIDL. Currently at 11 passes and 4 failures.

We're in good shape here, but need WebIDL Window and Location to fully pass the test suite. Blocking on those.

Looks like WebIDL Window has landed. Now we're just waiting on WebIDL Location.

The deps here are now done. I'm working on this now.

OWs only allow access to Window and Location, both of which are on WebIDL now.

This is necessary because subsequent patches cause us to assert when invoking getPropertyDescriptor on a FilteringWrapper for which |Policy| denies both GET and SET. This stuff is really a mess. I'm looking forward to it going away.

This causes garbage from a previous lookup to propagate into subsequent lookups, and creates confusing situations (like having both a value and a getter).

This allows us to test document.domain without hoisting the whole test into an example.com iframe.

Just flagging Ms2ger to make sure I did the test import right. The full review for these tests is happening on github/critic for web-platform-tests.

Comment on attachment 8462278 [details] [diff] [review] Part 7 - Throw for [[Delete]] and [[DefineOwnProperty]]. v1 Review of attachment 8462278 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/wrappers/FilteringWrapper.cpp @@ +238,5 @@ > + // through here. > + if (XrayUtils::IsXrayResolving(cx, wrapper, id)) > + return SecurityXrayDOM::defineProperty(cx, wrapper, id, desc); > + > + JS_ReportError(cx, "Permission denied to define property on cross-origin object"); I would expect a SecurityError DOMException

Comment on attachment 8462286 [details] [diff] [review] Part 14 - Add subdomains for mochi.test. v1 Review of attachment 8462286 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/pgo/server-locations.txt @@ +60,5 @@ > http://127.0.0.1:8888 privileged > http://test:80 privileged > http://mochi.test:8888 privileged > +http://test1.mochi.test:8888 privileged > +http://test2.mochi.test:8888 privileged I'm pretty sure the 'privileged' option hasn't done anything since you ripped out per-site caps. Might as well leave it out.

(In reply to :Ms2ger from comment #25) > I would expect a SecurityError DOMException Filed bug 1044083.

Comment on attachment 8462274 [details] [diff] [review] Part 3 - All properties on cross-origin DOM objects should be |own|. v1 Review of attachment 8462274 [details] [diff] [review]: ----------------------------------------------------------------- Can you please add to the etherpad that all white-listed properties should be own props?

Comment on attachment 8462275 [details] [diff] [review] Part 4 - All properties from cross-origin objects are "configurable", non-enumerable, and non-writable. v1 Review of attachment 8462275 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/wrappers/FilteringWrapper.cpp @@ +190,5 @@ > // All properties on cross-origin DOM objects are |own|. > desc.object().set(wrapper); > + > + // All properties on cross-origin DOM objects are non-enumerable and > + // "configurable". Any value attributes are read-only. I don't see how can these properties be configurable when we throw on delete attempts. Nor do I think we handle it here... Am I missing something?

Comment on attachment 8462277 [details] [diff] [review] Part 6 - Implement proper behavior for [[Enumerate]] And [[OwnPropertyKeys]]. v1 Review of attachment 8462277 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/wrappers/FilteringWrapper.cpp @@ +225,5 @@ > +{ > + // All properties on cross-origin objects are supposed |own|, despite what > + // the underlying native object may report. Override the inherited trap to > + // avoid passing JSITER_OWNONLY as a flag. > + return SecurityXrayDOM::enumerate(cx, wrapper, JSITER_HIDDEN, props); Do we really need hidden here? A simple JSITER_ENUMERATE would not cover all what we need? ::: js/xpconnect/wrappers/XrayWrapper.cpp @@ +1912,5 @@ > return false; > } > > + // Go through the properties we found on the underlying object and see if > + // they appear on the XrayWrapper. If if throws (which may happen if the Nit: "If if"

Comment on attachment 8462279 [details] [diff] [review] Part 8 - Don't use a FilteringWrapper to get an unfiltered view in ChromeObjectWrapper. v1 Review of attachment 8462279 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/wrappers/ChromeObjectWrapper.cpp @@ +59,5 @@ > MOZ_ASSERT(js::Wrapper::wrapperHandler(wrapper) == > &ChromeObjectWrapper::singleton); > Rooted<JSPropertyDescriptor> desc(cx); > const ChromeObjectWrapper *handler = &ChromeObjectWrapper::singleton; > + if (!handler->CrossCompartmentSecurityWrapper::getPropertyDescriptor(cx, wrapper, id, You might want to update the comment as well above this function.

> I don't see how can these properties be configurable when we throw on delete attempts. There is no spec requirement that configurable properties be deletable. The only requirement is that non-configurable properties not be deletable.

(In reply to Boris Zbarsky [:bz] from comment #32) > There is no spec requirement that configurable properties be deletable. The > only requirement is that non-configurable properties not be deletable. Right, I should have rely on the spec instead of our docs it seems...

Comment on attachment 8462275 [details] [diff] [review] Part 4 - All properties from cross-origin objects are "configurable", non-enumerable, and non-writable. v1 Review of attachment 8462275 [details] [diff] [review]: ----------------------------------------------------------------- Ignore my previous comment on this patch.

Comment on attachment 8462282 [details] [diff] [review] Part 11 - Switch policies for get{,Own}PropertyDescriptor. v1 Review of attachment 8462282 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/wrappers/AccessCheck.h @@ +78,5 @@ > struct ExposedPropertiesOnly : public Policy { > static bool check(JSContext *cx, JSObject *wrapper, jsid id, js::Wrapper::Action act); > > static bool deny(js::Wrapper::Action act, JS::HandleId id) { > + // Fail silently for GET ENUMERATE, and GET_PROPERTY_DESCRIPTOR Nit: missing .

Thanks for doing this marathon of reviews so quickly! Especially since it involved a fair amount of background reading on this spec issue that's been dragging on for over a year. ;-) (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #28) > Can you please add to the etherpad that all white-listed properties should > be own props? Wow, good catch. I guess the prose there is pretty vague at present. Fixed. (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #30) > Comment on attachment 8462277 [details] [diff] [review] > Part 6 - Implement proper behavior for [[Enumerate]] And > ::: js/xpconnect/wrappers/FilteringWrapper.cpp > > Do we really need hidden here? A simple JSITER_ENUMERATE would not cover all > what we need? I seem to remember needing it when I wrote this patch, though it might have been back in January when this stuff was still on XPCWNs. Regardless though, it seems better to avoid depending on enumerability (given that it's a kinda-sorta dead concept in ES), and just enumerate all the properties (the way Object.getOwnPropertyNames would), since we subsequently filter the properties and override the attributes.

Hm, I could have sworn I had a green all-platform try push for this, but all I can find is the (green) linux64 one in comment 24. This is a rather large change, so hopefully nothing breaks on another platform.

Looks like there were rooting hazards. I'll back out and fix in the morning: https://hg.mozilla.org/integration/mozilla-inbound/rev/042fa33c3f5c

(In reply to Bobby Holley (:bholley) from comment #36) > Thanks for doing this marathon of reviews so quickly! Especially since it > involved a fair amount of background reading on this spec issue that's been > dragging on for over a year. ;-) > Well... the real hard part of the job is to validate if the _what_ to implement part is all correct. But luckily that part was sort of already done by lot more knowledgeable people than me :)

https://hg.mozilla.org/mozilla-central/rev/e76ece9c756b https://hg.mozilla.org/mozilla-central/rev/88a562ab485b https://hg.mozilla.org/mozilla-central/rev/9e85835de239 https://hg.mozilla.org/mozilla-central/rev/f440504714b9 https://hg.mozilla.org/mozilla-central/rev/103615c82485 https://hg.mozilla.org/mozilla-central/rev/cf2bc60412d6 https://hg.mozilla.org/mozilla-central/rev/4209175a1f0a https://hg.mozilla.org/mozilla-central/rev/92d1c61c3cdf https://hg.mozilla.org/mozilla-central/rev/2a6260b2ae9c https://hg.mozilla.org/mozilla-central/rev/60dcc2593586 https://hg.mozilla.org/mozilla-central/rev/a5b95c1ec252 https://hg.mozilla.org/mozilla-central/rev/d099909ba007 https://hg.mozilla.org/mozilla-central/rev/7185b7e3b96b https://hg.mozilla.org/mozilla-central/rev/00cb0d79e405 https://hg.mozilla.org/mozilla-central/rev/79ac7fa6e649 https://hg.mozilla.org/mozilla-central/rev/22e1b7b69877

Jean-Yves, can you explain why this needs dev-doc?

Hi. I think we should explain what cross-origin objects are and some of their behavior. At first sight, I would say it may be worth to document the cross-origin behaviors of Window and Location. Does it make sense?

(In reply to Jean-Yves Perrier [:teoli] from comment #46) > Hi. I think we should explain what cross-origin objects are and some of > their behavior. At first sight, I would say it may be worth to document the > cross-origin behaviors of Window and Location. Does it make sense? Updating the documentation on the same-origin policy [1] definitely makes sense. In particular, it would be really helpful to include a list of the properties on Window and Location that are actually available cross-origin. The changes in this patch specifically are probably too subtle and technical to be worth documenting outside of the spec. [1] https://developer.mozilla.org/en-US/docs/Web/Security/Same-origin_policy

OK, I've had a go at this: https://developer.mozilla.org/en-US/docs/Web/Security/Same-origin_policy#Cross-origin_script_API_access. The only difference I could see between Firefox 36 and the spec (http://www.whatwg.org/specs/web-apps/current-work/multipage/browsers.html#security-window) is that Firefox 36 allows access to window.length.

(In reply to Will Bamberg [:wbamberg] from comment #48) > OK, I've had a go at this: > https://developer.mozilla.org/en-US/docs/Web/Security/Same- > origin_policy#Cross-origin_script_API_access. The only difference I could > see between Firefox 36 and the spec > (http://www.whatwg.org/specs/web-apps/current-work/multipage/browsers. > html#security-window) is that Firefox 36 allows access to window.length. window.length is a "dynamic nested browsing context properties", which is accessible per-spec. Looks great!