Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Declared global variables treated as undefined #11

Merged
merged 2 commits into from

2 participants

@assaf

In JavaScript, this results in an ReferenceError: y is not defined:

x = y

OTOH a variable declared with no value is set to undefined, and so this sets x to undefined:

var y
x = y

This pull requests provides two tests, one with a sandbox property (x) set to undefined and one with a global variable (z) declared as undefined.

This seems to fix the first test, by returning the sandbox property if defined, regardless of value:

Local<Value> rv = sandbox->Get(property);
if (sandbox->HasRealNamedProperty(property))
    return scope.Close(rv);

// Next, check the global object (things like Object, Array, etc).
// This needs to call GetRealNamedProperty or else we'll get stuck in
// an infinite loop here.
rv = info->global->GetRealNamedProperty(property);

I am not sure if this is correct, or how to go about fixing the second test.

@brianmcd brianmcd referenced this pull request from a commit
@brianmcd Updated test for #11. c6e9785
@brianmcd brianmcd referenced this pull request from a commit
@brianmcd Fix for #11 - Declared global variables treated as undefined
2 parts to this fix:
    1. Needed a proper NamedPropertyQuery interceptor, which must return
       an empty handle if the queried property is not intercepted.
    2. In Getter, when checking sandbox, we must call
       HasRealNamedProperty to make sure we don't miss properties that
       have been declared but not defined.
d497b61
@brianmcd brianmcd referenced this pull request from a commit
@brianmcd Follow up fix for #11.
HasRealNamedProperty doesn't check the prototype chain, so we have to
traverse it manually when looking for sandbox properties.
6f1d52c
@brianmcd brianmcd referenced this pull request from a commit
@brianmcd Follow up fix for #11
Actually, we can just use GetRealNamedProperty, which does check the
prototype chain.  It returns an empty handle if the property wasn't
found.
4abc7b3
@brianmcd brianmcd merged commit ca986f9 into brianmcd:master
@brianmcd
Owner

Thanks for the tests and the detailed report. I merged the tests in and I think I fixed the issue. If it looks good to you, I'll push a new release to npm. I tried running the Zombie (with the pending test activated) and JSDOM test suites with these fixes, and everything seems normal.

Thanks again for digging into this.

@assaf

Sweet. Works like a charm!

@brianmcd
Owner

I just pushed version 0.0.7 to npm. Thanks for your help on this!

@assaf
@brianmcd
Owner

Glad to help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 21 additions and 0 deletions.
  1. +21 −0 test/contextify.js
View
21 test/contextify.js
@@ -46,6 +46,27 @@ exports['basic tests'] = {
test.done();
},
+ // Make sure properties with value "undefined" are there.
+ 'test for "undefined" properties' : function (test) {
+ var sandbox = { x: undefined };
+ Contextify(sandbox);
+ sandbox.run("_x = x");
+ test.equal(sandbox._x, undefined);
+ test.done();
+ },
+
+ 'test for "undefined" variables' : function (test) {
+ var sandbox = { };
+ Contextify(sandbox);
+ // In JavaScript a declared variable is set to 'undefined'.
+ sandbox.run("var y; (function() { var _y ; y = _y })()");
+ test.equal(sandbox._y, undefined);
+ // This should apply to top-level variables (global properties).
+ sandbox.run("var z; _z = z");
+ test.equal(sandbox._z, undefined);
+ test.done();
+ },
+
// Make sure run can be called with a filename parameter.
'test run with filename' : function (test) {
var sandbox = Contextify();
Something went wrong with that request. Please try again.