vm: fix a variety of bugs, using V8 4.3 APIs #1773

domenic · 2015-05-22T23:43:16Z

See individual commits for details.

domenic · 2015-05-22T23:44:00Z

src/node_contextify.cc

-    if (!in_sandbox || !in_proxy_global) {
+    if (sandbox->HasRealNamedProperty(property)) {
+      PropertyAttribute propAttr =
+        sandbox->GetRealNamedPropertyAttributes(property).FromJust();


The .FromJust() here feels bad. I am not sure what the right way to handle these new maybes is.

Maybe if they're empty I should return None?

domenic · 2015-05-23T00:08:07Z

Added another commit while I was in there; kept it separate.

domenic · 2015-05-23T01:00:58Z

Morphing this into a general "fix all the vm bugs" thread.

domenic · 2015-05-26T14:56:40Z

src/node_contextify.cc

-    if (!in_sandbox || !in_proxy_global) {
-      args.GetReturnValue().Set(None);
+    bool in_sandbox =
+      sandbox->HasRealNamedProperty(ctx->context(), property).FromMaybe(false);


I need to rewrite this to be a little less crazy now that, as of v8/v8@7b24219, GetRealNamedProperty returns Nothing for absent, which allows us to only use GetRealNamedProperty instead of both HasRealNamedProperty and GetRealNamedProperty.

domenic · 2015-06-01T15:12:48Z

Rebased on latest next branch, and with some better Maybe-hygeine in the last commit. Would love to land this.

R=@bnoordhuis, @indutny

indutny · 2015-06-01T15:17:28Z

src/node_contextify.cc

-    if (!in_sandbox || !in_proxy_global) {
-      args.GetReturnValue().Set(None);
+
+    Maybe<PropertyAttribute> maybePropAttr =


May I ask you to use underscores instead of camel case? This way it will be more consistent with the rest of the core and this file in particular ;)

indutny · 2015-06-01T15:23:28Z

Few nits otherwise LGTM! Thank you! Guess we need to land V8 update first? ;)

domenic · 2015-06-01T15:27:41Z

@indutny fixed nits and lint errors; thanks for that. The next branch has a floating V8 backport patch for now so this works on top of the existing next.

bnoordhuis · 2015-06-01T16:10:35Z

src/node_contextify.cc

-    Local<Value> rv = sandbox->GetRealNamedProperty(property);
-    if (rv.IsEmpty()) {
+    MaybeLocal<Value> maybe_rv =
+      sandbox->GetRealNamedProperty(ctx->context(), property);


Femto-nit: can you indent line continuations with four spaces?

EDIT: Here and everywhere.

@smikes

The GlobalPropertyQueryCallback was changed in 2010 to return an integer instead of a boolean: https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU This integer communicates the property descriptors of the property, instead of just its presence or absence. However, the original contextify code was probably written before this change, and it was not updated when porting to Node.js. Credit to @smikes for the test and the original PR of nodejs#885. Fixes nodejs#885; fixes nodejs#864.

No reason to install access checks if they're always going to return true.

By using the new SetHandler API instead of SetNamedPropertyHandler, we can intercept symbols now. This forces us to use Maybes and MaybeLocals more, since this new API does not have a non-maybe variant. Fixes nodejs#884.

By using the new SetHandler API instead of SetNamedPropertyHandler, we can intercept symbols now. This forces us to use Maybes and MaybeLocals more, since this new API does not have a non-maybe variant. Fixes: #884 PR-URL: #1773 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

@smikes

The GlobalPropertyQueryCallback was changed in 2010 to return an integer instead of a boolean: https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU This integer communicates the property descriptors of the property, instead of just its presence or absence. However, the original contextify code was probably written before this change, and it was not updated when porting to Node.js. Credit to @smikes for the test and the original PR of #885. Fixes: #864 Fixes: #885 PR-URL: #1773 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>