Last Comment Bug 667047 - ArrayBuffer: __proto__ inconsistent behaviour
: ArrayBuffer: __proto__ inconsistent behaviour
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
:
Mentors:
Depends on:
Blocks: 665355
  Show dependency treegraph
 
Reported: 2011-06-24 13:56 PDT by Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
Modified: 2011-08-05 08:52 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Identical behaviour to native JS objects. (3.29 KB, patch)
2011-06-27 10:24 PDT, Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
no flags Details | Diff | Review
Identical behaviour to native JS objects. (3.94 KB, patch)
2011-06-29 18:51 PDT, Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
mrbkap: review+
Details | Diff | Review

Description Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) 2011-06-24 13:56:32 PDT
User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:5.0) Gecko/20100101 Firefox/5.0
Build Identifier: 

fixes for bug 665355 cause inconsistent behaviour of __proto__ on ArrayBuffers

Other JS objects treat __proto__ as a normal property once it has been set to null (the prototype chain is destroyed).

test case:
var d = new Date();
d.__proto__ = null
d.__proto__ = {a:2}
assertEq(d.a, undefined) // PASS

var x = new ArrayBuffer();
x.__proto__ = null
x.__proto__ = {a:2}
assertEq(x.a, undefined) // FAIL

Reproducible: Always



Expected Results:  
x.a should be undefined.
Comment 1 Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) 2011-06-27 10:24:27 PDT
Created attachment 542195 [details] [diff] [review]
Identical behaviour to native JS objects.
Comment 2 Blake Kaplan (:mrbkap) (please use needinfo!) 2011-06-29 12:26:09 PDT
Comment on attachment 542195 [details] [diff] [review]
Identical behaviour to native JS objects.

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

::: js/src/jstypedarray.cpp
@@ +279,5 @@
>  
>      if (JSID_IS_ATOM(id, cx->runtime->atomState.protoAtom)) {
> +        JSObject *proto = obj->getProto();
> +        if (proto) {
> +            if (!SetProto(cx, obj, vp->toObjectOrNull(), true))

This is close, but it isn't quite sufficient. For example consider:

var proto = Object.create(null);
var normal = {};
var ab = new ArrayBuffer([]);
ab.__proto__ = proto;
ab.__proto__ = { a: 42 };

normal.__proto__ = proto;
normal.__proto__ = { a: 42 };

is(ab.a, normal.a);

So the mere existence of a prototype doesn't mean that you have a __proto__ property.

How about always unconditionally calling js_SetProperty on the delegate and then detecting whether or not its proto changes and, if it does, changing the arraybuffer's proto?

@@ +292,2 @@
>          }
> +        else {

Uber-nit: the else should be on the same line as the closing brace for the if.
Comment 3 Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) 2011-06-29 18:51:59 PDT
Created attachment 543031 [details] [diff] [review]
Identical behaviour to native JS objects.

Alternative fix for bug 667047 to deal with cases 
where the prototype is set to null in the JS sense 
but not in the C++ sense                                  
eg.
var x = Object.create(null)

x is still null in the JS sense but in C++ the JSObject pointer is not NULL, but a valid JSObject.

I hope this explanation is valid.
Comment 4 Blake Kaplan (:mrbkap) (please use needinfo!) 2011-07-05 17:43:16 PDT
(In reply to comment #3) 
> eg.
> var x = Object.create(null)
> 
> x is still null in the JS sense but in C++ the JSObject pointer is not NULL,
> but a valid JSObject.

I don't understand what this means. x isn't null; it's an object that you can stick properties on and then look those properties up later. It just happens to have a null prototype.
Comment 5 Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) 2011-07-05 18:02:19 PDT
that's what I meant, that for truthiness purposes x is null, but its not really null
Comment 6 Michael Wu [:mwu] 2011-08-01 23:12:50 PDT
Backed out while investigating a webgl permaorange on mozilla-inbound.
Comment 7 Marco Bonardo [::mak] 2011-08-05 08:52:26 PDT
http://hg.mozilla.org/mozilla-central/rev/96f52c040d20

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