Last Comment Bug 895223 - Can't JSON stringify a proxy to an array
: Can't JSON stringify a proxy to an array
Status: RESOLVED FIXED
: dev-doc-complete, regression, site-compat
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla40
Assigned To: Tom Schuster [:evilpie]
:
Mentors:
: 876114 1111604 (view as bug list)
Depends on: 1147005
Blocks: 893276 978228 es6internalmethods 827490
  Show dependency treegraph
 
Reported: 2013-07-17 18:28 PDT by Dave Townsend [:mossop]
Modified: 2015-05-26 06:13 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
affected
affected
affected
affected
affected
affected
affected
affected
affected
affected
affected
affected
fixed


Attachments
Make the length property of arrays behave properly when behind a proxy (1.75 KB, patch)
2013-07-18 13:20 PDT, Eddy Bruel [:ejpbruel]
bobbyholley: review-
Details | Diff | Review
Pass obj, not receiver, to PropertyOp getters for data properties (1.78 KB, patch)
2013-08-15 09:06 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Review
Part 1 - Change jsperf.cpp to use JSNative getters rather than PropertyOps, v1 (6.50 KB, patch)
2013-08-15 12:55 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Review
Part 1 - Change jsperf.cpp to use JSNative getters rather than PropertyOps, v2 (6.56 KB, patch)
2013-08-15 14:02 PDT, Jason Orendorff [:jorendorff]
jdemooij: review+
Details | Diff | Review
Part 2 - Pass holder to PropertyOp getters, v2 (5.92 KB, patch)
2013-08-15 14:03 PDT, Jason Orendorff [:jorendorff]
jdemooij: review+
Details | Diff | Review
Hacky way to pass the holder (4.25 KB, patch)
2015-01-30 16:36 PST, Tom Schuster [:evilpie]
no flags Details | Diff | Review
Pass holder an receiver to JSGetterOP (20.86 KB, patch)
2015-03-27 14:33 PDT, Tom Schuster [:evilpie]
jdemooij: review+
Details | Diff | Review
Use JSNative instead of JSGetterOp for ctypes FieldGetter/Setter (6.45 KB, patch)
2015-03-30 13:52 PDT, Tom Schuster [:evilpie]
jorendorff: review+
Details | Diff | Review
Always pass holder to JSGetterOps (8.84 KB, patch)
2015-03-30 13:54 PDT, Tom Schuster [:evilpie]
no flags Details | Diff | Review
Test for the now correctly behaving properties (2.53 KB, patch)
2015-03-30 13:55 PDT, Tom Schuster [:evilpie]
jorendorff: review+
Details | Diff | Review
Always pass holder to JSGetterOps (9.31 KB, patch)
2015-04-02 15:53 PDT, Tom Schuster [:evilpie]
jorendorff: review+
jdemooij: review+
Details | Diff | Review

Description Dave Townsend [:mossop] 2013-07-17 18:28:34 PDT
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] 2013-07-17 18:34:25 PDT
Eddy, is this expected?
Comment 2 Dave Townsend [:mossop] 2013-07-18 10:52:49 PDT
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] 2013-07-18 13:20:15 PDT
Created attachment 778023 [details] [diff] [review]
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 (PTO through June 13) 2013-07-18 21:41:31 PDT
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] 2013-08-15 08:25:20 PDT
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] 2013-08-15 09:06:26 PDT
Created attachment 790808 [details] [diff] [review]
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 (PTO through June 13) 2013-08-15 09:34:50 PDT
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] 2013-08-15 10:35:45 PDT
(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] 2013-08-15 11:34:09 PDT
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] 2013-08-15 12:55:07 PDT
Created attachment 790915 [details] [diff] [review]
Part 1 - Change jsperf.cpp to use JSNative getters rather than PropertyOps, v1

A prerequisite.
Comment 11 Jason Orendorff [:jorendorff] 2013-08-15 14:02:41 PDT
Created attachment 790947 [details] [diff] [review]
Part 1 - Change jsperf.cpp to use JSNative getters rather than PropertyOps, v2
Comment 12 Jason Orendorff [:jorendorff] 2013-08-15 14:03:40 PDT
Created attachment 790948 [details] [diff] [review]
Part 2 - Pass holder to PropertyOp getters, v2
Comment 13 Jan de Mooij [:jandem] 2013-08-17 02:08:32 PDT
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] 2013-08-17 02:20:26 PDT
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] 2013-08-19 14:07:40 PDT
(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] 2013-08-20 00:41:41 PDT
(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 [:bz] 2013-08-20 21:30:44 PDT
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 (PTO through June 13) 2013-08-20 21:44:13 PDT
(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 [:bz] 2013-08-20 23:31:33 PDT
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] 2013-08-22 16:00:18 PDT
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 21 Jason Orendorff [:jorendorff] 2013-08-28 04:44:06 PDT
*** Bug 876114 has been marked as a duplicate of this bug. ***
Comment 22 Kohei Yoshino [:kohei] 2013-08-28 05:01:50 PDT
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] 2013-08-28 05:06:25 PDT
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 2013-08-28 05:06:51 PDT
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] 2013-08-28 05:20:20 PDT
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] 2013-08-29 16:41:12 PDT
Copied flags from Bug 876114.
Comment 27 Jason Orendorff [:jorendorff] 2013-09-06 19:52:56 PDT
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) 2013-09-07 17:28:52 PDT
https://hg.mozilla.org/mozilla-central/rev/5c2a0f1510bc
Comment 29 Jason Orendorff [:jorendorff] 2013-09-09 05:26:04 PDT
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 31 Kohei Yoshino [:kohei] 2013-09-21 21:59:19 PDT
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] 2014-02-28 14:37:55 PST
Why this is still open?
Comment 33 Kohei Yoshino [:kohei] 2014-02-28 17:05:57 PST
Yeah, this is not fixed yet. Updated the doc.
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_21
Comment 34 Tom Schuster [:evilpie] 2014-12-19 05:58:23 PST
*** Bug 1111604 has been marked as a duplicate of this bug. ***
Comment 35 Tom Schuster [:evilpie] 2015-01-28 14:24:44 PST
When this is fixed, please make sure to test the usage of the IsArray trap, especially with proxies.
Comment 36 Tom Schuster [:evilpie] 2015-01-30 16:36:03 PST
Created attachment 8557403 [details] [diff] [review]
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 38 Tom Schuster [:evilpie] 2015-03-25 08:49:16 PDT
Looking good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=34fbbd5627d1
Comment 39 Tom Schuster [:evilpie] 2015-03-27 14:33:06 PDT
Created attachment 8584809 [details] [diff] [review]
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] 2015-03-30 08:58:30 PDT
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 Schuster [:evilpie] 2015-03-30 10:23:03 PDT
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] 2015-03-30 12:03:22 PDT
Thanks, Tom. Ping me when you get the updated patch ready. I can quickly review.
Comment 44 Tom Schuster [:evilpie] 2015-03-30 13:52:31 PDT
Created attachment 8585692 [details] [diff] [review]
Use JSNative instead of JSGetterOp for ctypes FieldGetter/Setter
Comment 45 Tom Schuster [:evilpie] 2015-03-30 13:54:46 PDT
Created attachment 8585694 [details] [diff] [review]
Always pass holder to JSGetterOps
Comment 46 Tom Schuster [:evilpie] 2015-03-30 13:55:46 PDT
Created attachment 8585695 [details] [diff] [review]
Test for the now correctly behaving properties
Comment 47 Tom Schuster [:evilpie] 2015-03-30 15:07:52 PDT
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 Schuster [:evilpie] 2015-03-31 03:02:16 PDT
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 Schuster [:evilpie] 2015-03-31 13:31:18 PDT
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] 2015-03-31 14:32:54 PDT
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] 2015-03-31 14:34:49 PDT
(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] 2015-03-31 14:54:31 PDT
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 Schuster [:evilpie] 2015-04-02 15:53:05 PDT
Created attachment 8587703 [details] [diff] [review]
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] 2015-04-03 08:55:15 PDT
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 Schuster [:evilpie] 2015-04-09 10:32:07 PDT
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] 2015-04-09 19:50:19 PDT
https://hg.mozilla.org/mozilla-central/rev/47e8516e7397
Comment 57 Jason Orendorff [:jorendorff] 2015-04-10 16:23:37 PDT
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.

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