1185653 - Unboxed arrays change (5dbe1acdee3c, bug 1170372) causes JS exception "TypeError: cyclic object value" on dashboard.heroku.com

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Ed Morley [:emorley]]

STR:
1) Visit https://dashboard.heroku.com/orgs/mozillacorporation/apps
2) As an admin, click on an app that you're not a member of

Expected:
* No JS exception, and able to join the app.

Actual:
* JS exception at step #1.:
* At step #2, "You do not have access to view this app.  You can request access from an admin user."

The exception was:

TypeError: cyclic object value
at https://d1ic07fwm32hlr.cloudfront.net/assets/vendor-2dcfa7693dddcaa0a5f5b066aa01d728.js:32:13680

Viewing using Firefox's JS debugger shows the exception occurring in (thanks to the sourcemap):
bower_components/rest-model/dist/rest-model.js:2162

Specifically the JSON.stringify() of:

exports.set = function(key, value) {
var stringValue = JSON.stringify(value);
return this.storage.setItem(key, stringValue);
};

The json in question appears to be the contents of https://dashboard.heroku.com/api/apps (there's an array with 76 elements, with properties that matches that API response).

The exception (and resultant broken behaviour) does not occur in release Firefox, nor Chrome dev.

Bisecting with mozregression resulted in:

13:43.46 LOG: MainThread Bisector INFO Last good revision: 71b2118a180c
13:43.48 LOG: MainThread Bisector INFO First bad revision: 5dbe1acdee3c
13:43.48 LOG: MainThread Bisector INFO Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=71b2118a180c&tochange=5dbe1acdee3c

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

Comment 1

[Ed Morley [:emorley]]

Jan, Brian is away for the next 11 days, I don't suppose you could take a quick look at this and see if we should back it out in the meantime? I can give access to Heroku to repro, if that helps. Thanks :-)

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Ed Morley [:emorley] from comment #1)
> I can
> give access to Heroku to repro, if that helps. Thanks :-)

Yes that'd be useful. Or a copy of the JSON data that causes the problem, if you can reproduce with just that. I can't load that URL here and it probably also depends on the apps you installed etc.

Comment 3

[Till Schneidereit [:till]]

I'm not sure what I can do to help here.

Comment 4

[Ed Morley [:emorley]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> Yes that'd be useful. Or a copy of the JSON data that causes the problem, if
> you can reproduce with just that. I can't load that URL here and it probably
> also depends on the apps you installed etc.

I wasn't able to repro with just that. I've added you as an admin on the mozilla Heroku account; you should have an invite to your moco email address.

(In reply to Till Schneidereit [:till] from comment #3)
> I'm not sure what I can do to help here.

I needinfoed since you commented on the regressing bug in a way that made me think you might be able to help, but now that Jan is taking a look it's all good :-)

Comment 5

[Jan de Mooij [:jandem]]

Thanks Ed, I can see the problem here.

Unfortunately it landed as a single big patch instead of the smaller patches that were uploaded to Bugzilla, so now I have to track down which part of the patch is responsible :(

The good news is that it also repros with the JITs disabled, so I can hopefully ignore the JIT changes.

Comment 6

[Jan de Mooij [:jandem]]

OK, below is a shell testcase.

JSON.stringify calls the enumerate hook on an UnboxedPlainObject, which then enumerates the properties of its expando, if any. However, UnboxedPlainObject::obj_enumerate enumerates *all* expando properties, even the non-enumerable ones, leading to the "cyclic object value" error...

function f() {
    var arr = [];
    for (var i=0; i<100; i++) {
	var o3 = {foo: i};
	var o2 = {owner: o3};
	arr.push(o2);
    }
    for (var i=0; i<100; i++) {
	var o2 = arr[i];
	var o3 = o2.owner;
	Object.defineProperty(o3, "bla", {value: arr, enumerable: false});
    }
    print(JSON.stringify(arr));
}
f();

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Patch

This adds an enumerableOnly bool to the enumerate hook. If it's `true`, the UnboxedPlainObject hook filters out any non-enumerable properties on the expando object.

Not super excited about it, but it fixes this issue and should be safe enough to backport.

I was also able to remove the special-case for `length` on unboxed arrays.

Comment 8

[Ed Morley [:emorley]]

Thank you for tracking this down! :-)

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Ed Morley [:emorley] from comment #8)
> Thank you for tracking this down! :-)

No problem, thanks for filing and bisecting.

(In reply to Ed Morley [:emorley] from comment #4)
> I've added you as an admin on the
> mozilla Heroku account; you should have an invite to your moco email address.

(I removed myself from the Mozilla account, the less I can mess up the better :)

Comment 10

[Ed Morley [:emorley]]

(In reply to Jan de Mooij [:jandem] from comment #9)
> (I removed myself from the Mozilla account, the less I can mess up the
> better :)

Thank you, whether you were finished with it or not was going to be my next question :-)

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked and adding a regression keyword.

Comment 12

[Jason Orendorff [:jorendorff]]

Comment on attachment 8637477 [details] [diff] [review]
Patch

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

r=me with reservations. This is OK, but it won't fix all the bugs here, right? JSITER_HIDDEN isn't the only flag that can be passed to Snapshot. I think all the other flags must be buggy as well.

For example, Reflect.ownKeys() passes JSITER_SYMBOLS, and Object.getOwnPropertySymbols() passes JSITER_SYMBOLS|JSITER_SYMBOLSONLY. I'm not sure what those will do when applied to a typed or unboxed object; possibly assert, and crash in scary ways in release builds (bad downcasting).

Separately, we want ProxyHandler or ObjectOps to line up better, and this would be a step in the opposite direction. But we can do it for a quick bug fix if needed.

In the end, both ProxyHandler and ObjectOps should have the same two hooks:
    bool enumerate      (cx, obj,        out iteratorObj);
    bool ownPropertyKeys(cx, obj, flags, out idVector);
where `flags` is a pure optimization, to avoid a bunch of follow-up getOwnPropertyDescriptor() hook calls, and the allowed flags are JSITER_{HIDDEN,SYMBOLS,SYMBOLSONLY}.

::: js/public/Class.h
@@ +262,4 @@
>  // for-in loop will iterate over. All of this is nonstandard.
>  //
>  // An object is "enumerated" when it's the target of a for-in loop or JS_Enumerate().
>  // All other property inspection, including Object.keys(obj), goes through [[OwnKeys]].

Please fix the comment. This paragraph is already inaccurate before your changes. I mean, it would be nice if it were true, but it's not.

::: js/src/jsiter.cpp
@@ +285,5 @@
>      RootedObject pobj(cx, pobj_);
>  
>      do {
>          if (JSNewEnumerateOp enumerate = pobj->getOps()->enumerate) {
>              // This hook has the full control over what gets enumerated.

This one-line comment also isn't quite correct and can just be deleted.

::: js/src/vm/UnboxedObject.cpp
@@ +1547,5 @@
>              return false;
>      }
> +
> +    if (!enumerableOnly && !properties.append(NameToId(cx->names().length)))
> +        return false;

These seem worth testing, since enumeration has more evolving to do --- both for ES6 compliance and eventual ObjectOps-ProxyHandler unification.

Comment 13

[Jan de Mooij [:jandem]]

(In reply to Jason Orendorff [:jorendorff] from comment #12)
> r=me with reservations. This is OK, but it won't fix all the bugs here,
> right? JSITER_HIDDEN isn't the only flag that can be passed to Snapshot. I
> think all the other flags must be buggy as well.
> 
> For example, Reflect.ownKeys() passes JSITER_SYMBOLS, and
> Object.getOwnPropertySymbols() passes JSITER_SYMBOLS|JSITER_SYMBOLSONLY.

That's what I thought as well, but it turns out those flags are fine because Enumerate (called to add each property) does the filtering for those.

Comment 14

[Ed Morley [:emorley]]

On Monday this regression will be on the beta channel; any chance this can land before then? :-)

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/462b7f77dc3a

Comment 16

[Jan de Mooij [:jandem]]

(In reply to Ed Morley [:emorley] from comment #14)
> On Monday this regression will be on the beta channel; any chance this can
> land before then? :-)

Sorry, I was on PTO for the past few days. Will get this uplifted asap.

Comment 17

[Jan de Mooij [:jandem]]

Comment on attachment 8637477 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1170372.
[User impact if declined]: Broken websites.
[Describe test coverage new/current, TreeHerder]: Added a test, fixes the website.
[Risks and why]: Low. Not a one-liner but the patch is straight-forward.
[String/UUID change made/needed]: None.

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/462b7f77dc3a

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Ed, could you please verify that the fix works on the latest Nightly? Thanks.

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8637477 [details] [diff] [review]
Patch

Patch has been in m-c for a few days. Seems safe to uplift to Aurora and Beta.

Comment 21

[Ed Morley [:emorley]]

(In reply to Ritu Kothari (:ritu) from comment #19)
> Ed, could you please verify that the fix works on the latest Nightly? Thanks.

Yes this works on latest Nightly now, thank you :-)

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ac952c7ae8cc

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/45349faf26f9