Alexandre Poirot [:ochameau] Reporter
	Description • 12 years ago

Here is a scratchpad snippet that describe this issue:

  var Cu = Components.utils;
  // `content` is a reference to the current tab window object
  var s = Cu.Sandbox(content, {
    sandboxPrototype: content
  });
  Cu.reportError( s.location + " - "+ content.location );
  Cu.reportError( s.navigator + " - "+ content.navigator );

If you open a priviledged document `s.location` and `s.navigator` will be undefined whereas same attributes on `content` are correctly set.
This issue only occurs on priviledged document so that it will fail with undefined value if you open about:addons for example. But it is still working fine with content documents like http://...

It is still working fine on FF 12 but not on Aurora, nor on nightly.

I looked at these attributes definition without being able to find any potential issue:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.cpp#7238
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.cpp#7050

	Gabor Krizsanits (INACTIVE)
	Comment 1 • 12 years ago

This is most likely a regression from Bug 726949, I'm currently trying to validate this assumption.

	Gabor Krizsanits (INACTIVE)
	Comment 2 • 12 years ago

So in the chrome case where it fails the difference is that in xpc::SandboxProxyHandler::getPropertyDescriptor, the desc->getter for the location is XPC_WN_Helper_GetProperty, instead of xpc::holder_get (which is in the regular non-chrome case that works).

I guess this is because the different wrapper that is applied? (no xray wrapper in the failing case only a transparent CrossCompartmentWrapper)

bz: Can it be the reason that in this case the getter should not be overwritten with that bound function?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 3 • 12 years ago

Hmm.  I guess "location" and "navigator" end up defined directly on the window by the window resolve hook instead of being gotten through normal XPConnect getters.  And XPC_WN_Helper_GetProperty does nothing with them, so relies on the same slot behavior holder_get and holder_set rely on.  Probably same for XPC_WN_Helper_SetProperty.

So yeah, we should treat these like holder_get and holder_set.  Do you want to write the patch, or should I?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 4 • 12 years ago

And thanks for hunting this down!

	Gabor Krizsanits (INACTIVE)
	Comment 5 • 12 years ago

I'm a bit flooded right now so if you could write the patch that would be great... if you are flooded non the less, and won't have time for it in the next few days I can implement it as well with your help.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 6 • 12 years ago

Nah, I can do this.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 8 • 12 years ago

We should probably track this regression for Firefox 14.

	Bobby Holley (:bholley)
	Comment 11 • 12 years ago

Comment on attachment 620544 [details] [diff] [review]
Skip binding for the XPConnect-default property ops as well when resolving properties on a sandbox prototype.

>+                "Should have an own 'Cu' property");
>+          is(desc.value, Cu, "Should have the right value");
>+          var loc = Cu.evalInSandbox('location', s);
>+          is(loc.href, location.href, "Should have the right location");
>       } catch (e) {
>           ok(false, "Should not get an exception: " + e);
>       }
>   ]]>
>   </script>
> </window>

This test fails without your patch, right? I just want to make sure that we're not doing Xray here (in which case this would be agnostic to your patch).

Looks good. r=bholley assuming the above.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 12 • 12 years ago

> This test fails without your patch, right?

Yes, of course.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 13 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8eed84d2c54

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 14 • 12 years ago

Comment on attachment 620544 [details] [diff] [review]
Skip binding for the XPConnect-default property ops as well when resolving properties on a sandbox prototype.

[Approval Request Comment]
Regression caused by (bug #): bug 726949 
User impact if declined: Some things won't work correctly in the web console and
   in other sandboxes (return undefined instead of the real object).  This may
   well break some extensions; it certainly breaks some of the jetpack unit
   tests, apparently.
Testing completed (on m-c, etc.): Passes attached testcase and manual testing
Risk to taking this patch (and alternatives if risky): Risk to the patch itself
   is pretty low, I think.  The main risk is that there are still other
   getters/setters we should whitelist here that I didn't think of, but those
   are broken whether we take this patch or not.  The only alternative I can
   think of is backing out the bug that caused this, backing out the bug that
   caused _that_, and turning off the new DOM bindings for XHR for another
   release.
String changes made by this patch: None.

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 12 years ago

http://hg.mozilla.org/mozilla-central/rev/a8eed84d2c54

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 18 • 12 years ago

http://hg.mozilla.org/releases/mozilla-aurora/rev/69e772363914