Last Comment Bug 751077 - navigator and location are undefined when using sandboxPrototype=chromeWindow
: navigator and location are undefined when using sandboxPrototype=chromeWindow
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
:
Mentors:
: 751699 751919 752404 753258 (view as bug list)
Depends on:
Blocks: 726949 751069
  Show dependency treegraph
 
Reported: 2012-05-02 02:10 PDT by Alexandre Poirot [:ochameau]
Modified: 2013-04-22 07:44 PDT (History)
15 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
fixed


Attachments
Skip binding for the XPConnect-default property ops as well when resolving properties on a sandbox prototype. (4.67 KB, patch)
2012-05-02 18:11 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
bobbyholley: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Review

Description Alexandre Poirot [:ochameau] 2012-05-02 02:10:37 PDT
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
Comment 1 Gabor Krizsanits [:krizsa :gabor] 2012-05-02 04:35:46 PDT
This is most likely a regression from Bug 726949, I'm currently trying to validate this assumption.
Comment 2 Gabor Krizsanits [:krizsa :gabor] 2012-05-02 09:43:19 PDT
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?
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-02 09:58:40 PDT
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?
Comment 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-02 09:58:52 PDT
And thanks for hunting this down!
Comment 5 Gabor Krizsanits [:krizsa :gabor] 2012-05-02 10:45:58 PDT
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.
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-02 11:11:38 PDT
Nah, I can do this.
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-02 18:11:48 PDT
Created attachment 620544 [details] [diff] [review]
Skip binding for the XPConnect-default property ops as well when resolving properties on a sandbox prototype.
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-02 18:12:55 PDT
We should probably track this regression for Firefox 14.
Comment 9 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-03 14:06:37 PDT
*** Bug 751699 has been marked as a duplicate of this bug. ***
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-07 09:14:55 PDT
*** Bug 751919 has been marked as a duplicate of this bug. ***
Comment 11 Bobby Holley (busy) 2012-05-07 10:16:45 PDT
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.
Comment 12 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-07 10:19:26 PDT
> This test fails without your patch, right?

Yes, of course.
Comment 13 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-07 11:15:21 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8eed84d2c54
Comment 14 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-07 11:21:37 PDT
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.
Comment 15 Ryan VanderMeulen [:RyanVM] 2012-05-07 18:10:50 PDT
http://hg.mozilla.org/mozilla-central/rev/a8eed84d2c54
Comment 16 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-08 09:18:21 PDT
*** Bug 752404 has been marked as a duplicate of this bug. ***
Comment 17 Panos Astithas [:past] 2012-05-09 00:50:22 PDT
*** Bug 753258 has been marked as a duplicate of this bug. ***
Comment 18 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-09 18:34:32 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/69e772363914

Note You need to log in before you can comment on or make changes to this bug.