Closed Bug 751077 Opened 9 years ago Closed 9 years ago

navigator and location are undefined when using sandboxPrototype=chromeWindow


(Core :: DOM: Core & HTML, defect)

Not set



Tracking Status
firefox13 --- unaffected
firefox14 + fixed


(Reporter: ochameau, Assigned: bzbarsky)



(Keywords: regression)


(1 file)

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:
This is most likely a regression from Bug 726949, I'm currently trying to validate this assumption.
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?
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?
Blocks: 726949
And thanks for hunting this down!
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.
Nah, I can do this.
Assignee: nobody → bzbarsky
We should probably track this regression for Firefox 14.
Keywords: regression
Whiteboard: [need review]
Duplicate of this bug: 751699
Duplicate of this bug: 751919
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.
Attachment #620544 - Flags: review?(bobbyholley+bmo) → review+
> This test fails without your patch, right?

Yes, of course.
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla15
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
String changes made by this patch: None.
Attachment #620544 - Flags: approval-mozilla-aurora?
Closed: 9 years ago
Resolution: --- → FIXED
Duplicate of this bug: 752404
Duplicate of this bug: 753258
Attachment #620544 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.