895223 - Can't JSON stringify a proxy to an array

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Dave Townsend [:mossop]]

I'd expect this to work:

var a = [1, 2, 3];
var b = new Proxy(a, {});
JSON.stringify(b);

But all it returns is "[]". If a is an object then it gets stringified just fine.

Comment 1

[Dave Townsend [:mossop]]

Eddy, is this expected?

Comment 2

[Dave Townsend [:mossop]]

This works better:

var a = [1, 2, 3];
var b = new Proxy(a, { get: function(target, name) target[name] });
JSON.stringify(b);

Comment 3

[Eddy Bruel [:ejpbruel]]

Attachment: Make the length property of arrays behave properly when behind a proxy

The problem seems to be this:

JSON.stringify tries to access the length property of b. Since b is a proxy, it forwards to a, which in turn forwards to Array.prototype.length. The end result is that the length getter is invoked on Array.prototype, using the proxy as the receiver.

On the C++ side, we end up in array_length_getter, which expects to be called with a receiver that is either a native array object, or an object that has a native array object in its prototype.

The solution seems to be to explicitly check whether the receiver is a proxy, and unwrap it to get to the underlying native array object (provided we are allowed to do so). We have a function for this, CheckedUnwrap. However, this function only works on wrappers (which are really a specialization of proxies). Since proxies can be wrapped, and wrappers can be proxied to, we need to handle both cases separately.

I've added a patch that implements my proposed solution. Since I'm not sure how safe this solution is from a security point of view, I've flagged Bobby for feedback.

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 778023 [details] [diff] [review]
Make the length property of arrays behave properly when behind a proxy

Review of attachment 778023 [details] [diff] [review]:
-----------------------------------------------------------------

This is the same problem as bug 827449, except now we're dealing with direct proxies instead of same-compartment wrappers. The simple unwrapping approach won't work.

Jorendorff, what shall we do here?

Comment 5

[Jason Orendorff [:jorendorff]]

Hmm.

The interesting thing here is that now we're dealing entirely with standard facilities. I'm pretty sure the spec requires this to work. Array length is, per spec, a data property.

I guess that would mean that property ops like array length must be invoked so as to behave more like data properties.

Let me try hacking Shape::get a bit.

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: Pass obj, not receiver, to PropertyOp getters for data properties

This passes tests locally.

Try servering: https://tbpl.mozilla.org/?tree=Try&rev=f7c1838b30a2

Comment 7

[Bobby Holley (:bholley)]

I don't understand this patch. Isn't the issue here that |obj| is Array.prototype and |receiver| is a proxy? How does this help?

Whatever the answer is, it should end up in a comment above this line.

Comment 8

[Jason Orendorff [:jorendorff]]

(In reply to Bobby Holley (:bholley) from comment #7)
> I don't understand this patch. Isn't the issue here that |obj| is
> Array.prototype and |receiver| is a proxy? How does this help?

|receiver| is a proxy and |obj| is the array. (The length property of arrays is not inherited; each array has its own non-configurable .length property.)

I want to go a little further and use |pobj|, which is also the array in this case, and more generally is the object the property is actually found on. Testing it now.

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 790808 [details] [diff] [review]
Pass obj, not receiver, to PropertyOp getters for data properties

Clearing r? for now. Going to post a bigger patch.

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: Part 1 - Change jsperf.cpp to use JSNative getters rather than PropertyOps, v1

A prerequisite.

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: Part 1 - Change jsperf.cpp to use JSNative getters rather than PropertyOps, v2

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: Part 2 - Pass holder to PropertyOp getters, v2

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 790947 [details] [diff] [review]
Part 1 - Change jsperf.cpp to use JSNative getters rather than PropertyOps, v2

Review of attachment 790947 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/perf/jsperf.cpp
@@ +206,4 @@
>  {
> +    if (!value.isObject()) {
> +        JS_ReportErrorNumber(cx, js_GetErrorMessage, 0, JSMSG_NOT_NONNULL_OBJECT);
> +        return 0;

Nit: s/0/NULL twice.

@@ +217,5 @@
>      // JS_GetInstancePrivate only sets an exception if its last argument
>      // is nonzero, so we have to do it by hand.
>      JS_ReportErrorNumber(cx, js_GetErrorMessage, 0, JSMSG_INCOMPATIBLE_PROTO,
>                           pm_class.name, fname, JS_GetClass(obj)->name);
>      return 0;

And here.

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 790948 [details] [diff] [review]
Part 2 - Pass holder to PropertyOp getters, v2

Review of attachment 790948 [details] [diff] [review]:
-----------------------------------------------------------------

Nice. I'm not sure how this affects things on the DOM side though. Please ask bz, r=me if he's okay with it.

::: js/src/jsarray.cpp
@@ +379,2 @@
>  {
> +    vp.setNumber(obj->as<ArrayObject>().length());

Beautiful.

::: js/src/vm/Shape-inl.h
@@ +245,5 @@
>      return true;
>  }
>  
>  inline bool
> +Shape::get(JSContext *cx, HandleObject receiver, HandleObject holder, MutableHandleValue vp)

So pobj and obj were never used? Where are the compiler warnings when you need them :)

Comment 15

[Jason Orendorff [:jorendorff]]

(In reply to Jan de Mooij [:jandem] from comment #13)
> Nit: s/0/NULL twice.

OK, fixed. I was just following the prevailing style in that file.

> I'm not sure how this affects things on the DOM side though.

I did ask about that and the new DOM bindings do not use JSPropertyOp getters at all.

> So pobj and obj were never used? Where are the compiler warnings when you
> need them :)

Yeah. Weird.

Comment 16

[Jan de Mooij [:jandem]]

(In reply to Jason Orendorff [:jorendorff] from comment #15)
> > I'm not sure how this affects things on the DOM side though.
> 
> I did ask about that and the new DOM bindings do not use JSPropertyOp
> getters at all.

Not everything has been ported to the new DOM bindings though; there's still code that uses JSPropertyOps...

Comment 17

[Boris Zbarsky [:bzbarsky]]

Outside of the JS engine itself, I believe only worker code uses JSPropertyOps.  In particular, xpconnect, webidl, and quickstubs all use JSNative getters/setters.

Comment 18

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #17)
> Outside of the JS engine itself, I believe only worker code uses
> JSPropertyOps.  In particular, xpconnect, webidl, and quickstubs all use
> JSNative getters/setters.

Um, XPConnect definitely uses JSPropertyOps - see XPCWrappedNativeJSOps.cpp. Consumers can register arbitrary PropertyOps via nsIXPCScriptable. Or am I misinterpreting what's going on?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Oh, hmm.  I guess for the case when you have an nsIXPCScriptable that has WANT_GETPROPERTY/WANT_SETPROPERTY we set the JSClass getProperty/setProperty to XPC_WN_Helper_Get/SetProperty.

So yes, in those cases we'll end up with JSPropertyOps.  :(  And there are still a few (very few) web-visible consumers of that stuff.

Comment 20

[Jason Orendorff [:jorendorff]]

These patches didn't pass try server. I need to look closer but haven't had time. The first one in particular should trivially pass.

Comment 22

[Kohei Yoshino [:kohei]]

This has regressed in Gecko 21, as per Bug 876114. My code was working before but it doesn't work without the get trap.

Comment 23

[Jason Orendorff [:jorendorff]]

Extra test case thanks to laurent and kohei in bug 876114:

var log = '';

var p = new Proxy(["hello"], { 
    set: function(arr, idx, v) {
        log += idx + ';';
        arr[idx] = v;
        return true;
    }
});

p.push("aa");
p.push("bb");
p.push("cc");
assertEq(log, "0;length;1;length;2;length;");

Comment 24

[Loic]

Does the doc need to be changed after your modifications in comment https://bugzilla.mozilla.org/show_bug.cgi?id=876114#c13 when this bug will land?

Comment 25

[Kohei Yoshino [:kohei]]

Removed the bug from
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_21
as I don't usually put a simple regression on the compatibility docs.

And slightly modified a note on the Proxy doc:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy
Once the bug gets fixed, I'll update the note again.

Comment 26

[Kohei Yoshino [:kohei]]

Copied flags from Bug 876114.

Comment 27

[Jason Orendorff [:jorendorff]]

Landed part 1; part 2 is still busted and I don't have time for it right now.

One way to address it would be to reimplement WANT_GETPROPERTY/SETPROPERTY in terms of the newer ObjectOps. That may not actually be all that bad.

Another way would be to keep the old behavior for JSPropertyOps that match the object's JSClass.getProperty/setProperty; that would be super gruesome but is probably the quickest thing to get done (assuming someone's willing to hold their nose and review it).

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c2a0f1510bc

Comment 28

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5c2a0f1510bc

Comment 29

[Jason Orendorff [:jorendorff]]

Sorry, forgot to keep-open this. Only part 1 of 2 landed.

(In reply to Jason Orendorff [:jorendorff] from comment #27)
> One way to address it would be to reimplement WANT_GETPROPERTY/SETPROPERTY
> in terms of the newer ObjectOps. That may not actually be all that bad.

Actually only WANT_GETPROPERTY. The patch only affects getProperty ops.

So the deal would be, simulate the old getProperty behavior using the newer ops.getGeneric hook.

This might lead to removing JSClass::getProperty. That would be nice.

Comment 30

[Kohei Yoshino [:kohei]]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/25/Site_Compatibility

Comment 31

[Kohei Yoshino [:kohei]]

Moved the regression info to the Firefox 21 compat doc:
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_21

Comment 32

[Kohei Yoshino [:kohei]]

Why this is still open?

Comment 33

[Kohei Yoshino [:kohei]]

Yeah, this is not fixed yet. Updated the doc.
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_21

Comment 35

[Tom S [:evilpie]]

When this is fixed, please make sure to test the usage of the IsArray trap, especially with proxies.

Comment 36

[Tom S [:evilpie]]

Attachment: Hacky way to pass the holder

I made this hack patch to pass the holder in the |vp| argument. This allows us to not change any signatures, and doesn't use more registers in ion.

Comment 37

[Tom S [:evilpie]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=47e2bc22c11e

Comment 38

[Tom S [:evilpie]]

Looking good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=34fbbd5627d1

Comment 39

[Tom S [:evilpie]]

Attachment: Pass holder an receiver to JSGetterOP

We now pass the holder and receiver to JSGetterOps. Interestingly we already passed the receiver in one case, but the holder in another!

Jan, please look at the Ion changes, thanks.

Comment 40

[Jan de Mooij [:jandem]]

Comment on attachment 8584809 [details] [diff] [review]
Pass holder an receiver to JSGetterOP

Review of attachment 8584809 [details] [diff] [review]:
-----------------------------------------------------------------

jit/ changes look good to me. Can you add a jit-test for this?

Comment 41

[Tom S [:evilpie]]

I talked with Jason about this recently, it basically looks like we can make this a bit simpler when we just always pass the holder to PropertyOps. There is one case in ctypes where we actually want the receiver and that can be simply rewritten to a JSNative based getter/setter.

Comment 42

[Jason Orendorff [:jorendorff]]

Thanks, Tom. Ping me when you get the updated patch ready. I can quickly review.

Comment 43

[Tom S [:evilpie]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e48912c1e9d

Comment 44

[Tom S [:evilpie]]

Attachment: Use JSNative instead of JSGetterOp for ctypes FieldGetter/Setter

Comment 45

[Tom S [:evilpie]]

Attachment: Always pass holder to JSGetterOps

Comment 46

[Tom S [:evilpie]]

Attachment: Test for the now correctly behaving properties

Comment 47

[Tom S [:evilpie]]

Comment on attachment 8585694 [details] [diff] [review]
Always pass holder to JSGetterOps

Review of attachment 8585694 [details] [diff] [review]:
-----------------------------------------------------------------

I am trying to figure out a compacting GC test failure that only seems to happen on try.

Comment 48

[Tom S [:evilpie]]

So in test jit-test/tests/basic/splice-check-steps.js for some reason the length property is wrong at some point. For [22,1,2,3,4,5] we get a length of 9. My best guess right now is the Ion IC, because the rest shouldn't go wrong. BaseProxyHandler::get is a bit weird as well, but isn't invoked here.

Comment 49

[Tom S [:evilpie]]

So I found out how to reproduce it:

"../configure --enable-debug --without-intl-api --enable-gczeal"
export JS_GC_ZEAL=14
jit-test/jit_test.py --args="--ion-eager --ion-offthread-compile=off" debug2-obj/dist/bin/js basic/splice-check-steps.js

Comment 50

[Jason Orendorff [:jorendorff]]

Comment on attachment 8585692 [details] [diff] [review]
Use JSNative instead of JSGetterOp for ctypes FieldGetter/Setter

Review of attachment 8585692 [details] [diff] [review]:
-----------------------------------------------------------------

OK. It would be pretty easy to eliminate that call to LookupField in the getter and setter. Maybe Arai will go after it.

Comment 51

[Jason Orendorff [:jorendorff]]

(I meant by storing more specific information in the accessors' reserved slots. But it would be a better use of time, and a much bigger performance win, to steer ctypes in the direction of typed objects, which have JIT optimizations.)

Comment 52

[Jason Orendorff [:jorendorff]]

Comment on attachment 8585695 [details] [diff] [review]
Test for the now correctly behaving properties

Review of attachment 8585695 [details] [diff] [review]:
-----------------------------------------------------------------

Great!

Comment 53

[Tom S [:evilpie]]

Attachment: Always pass holder to JSGetterOps

Thanks to Jan for helping me understand why we need this special code for passing the holder in the Ion IC.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1bf6066c6242

Comment 54

[Jan de Mooij [:jandem]]

Comment on attachment 8587703 [details] [diff] [review]
Always pass holder to JSGetterOps

Review of attachment 8587703 [details] [diff] [review]:
-----------------------------------------------------------------

IonCaches changes LGTM.

::: js/src/jit/IonCaches.cpp
@@ +1061,5 @@
>  
> +        // Push the holder.
> +        if (obj == holder) {
> +            // When the holder is also the current receiver, we just have a shape guard,
> +            // so we might end-up with an random object which is also guaranteed to have

Nit: s/an/a/. Also while you're here, I think "end up" is more common than "end-up".

Comment 55

[Tom S [:evilpie]]

Pushed the first patch for changing FieldGetter to a JSNative. Waiting for jorendorff.
https://hg.mozilla.org/integration/mozilla-inbound/rev/47e8516e7397

Comment 56

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/47e8516e7397

Comment 57

[Jason Orendorff [:jorendorff]]

Comment on attachment 8587703 [details] [diff] [review]
Always pass holder to JSGetterOps

Review of attachment 8587703 [details] [diff] [review]:
-----------------------------------------------------------------

Fantastic. r=me on everything except IonCaches which jandem already reviewed.

Comment 58

[Tom S [:evilpie]]

\o/ Thank you both. Finally fixed!
https://hg.mozilla.org/integration/mozilla-inbound/rev/1108a9629d39
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a6497505eb1

Comment 59

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1108a9629d39
https://hg.mozilla.org/mozilla-central/rev/1a6497505eb1

Comment 60

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/40#JavaScript
https://developer.mozilla.org/en-US/Firefox/Releases/40/Site_Compatibility#Proxyfied_arrays_without_the_get_trap_now_work_properly