Note: There are a few cases of duplicates in user autocompletion which are being worked on.

navigator and location are undefined when using sandboxPrototype=chromeWindow

RESOLVED FIXED in Firefox 14

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: ochameau, Assigned: bz)

Tracking

({regression})

unspecified
mozilla15
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox13 unaffected, firefox14+ fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 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
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?
(Assignee)

Comment 3

5 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?
Blocks: 726949
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 6

5 years ago
Nah, I can do this.
Assignee: nobody → bzbarsky
(Assignee)

Comment 7

5 years ago
Created attachment 620544 [details] [diff] [review]
Skip binding for the XPConnect-default property ops as well when resolving properties on a sandbox prototype.
Attachment #620544 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 8

5 years ago
We should probably track this regression for Firefox 14.
status-firefox14: --- → affected
tracking-firefox14: --- → ?
Keywords: regression
Whiteboard: [need review]
tracking-firefox14: ? → +
(Assignee)

Updated

5 years ago
Duplicate of this bug: 751699
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 12

5 years ago
> This test fails without your patch, right?

Yes, of course.
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8eed84d2c54
status-firefox13: --- → unaffected
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla15
(Assignee)

Comment 14

5 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.
Attachment #620544 - Flags: approval-mozilla-aurora?
http://hg.mozilla.org/mozilla-central/rev/a8eed84d2c54
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Duplicate of this bug: 752404
Duplicate of this bug: 753258
Attachment #620544 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 18

5 years ago
http://hg.mozilla.org/releases/mozilla-aurora/rev/69e772363914
status-firefox14: affected → fixed
You need to log in before you can comment on or make changes to this bug.