769192 - "Assertion failure: !(*attrsp & (0x10 | 0x20)),"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

Object.watch.call(new Uint8ClampedArray, "length", function() {});

asserts js debug shell on m-c changeset bf8f2961d0cc without any CLI arguments at Assertion failure: !(*attrsp & (0x10 | 0x20)),

s-s because the regressing patch points to typed arrays, a known source of s-s bugs in the past.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   97822:c408dd243e7a
user:        Steve Fink
date:        Mon Jun 25 17:22:37 2012 -0700
summary:     Bug 734215 - Typed array property access should allow proxies. r=luke

Comment 1

[Jeff Walden [:Waldo]]

Did the newly-accessorized properties get removed from the lookup ops for all the relevant classes?  Without having tested, I'm guessing that didn't happen, needs to happen.

Comment 2

[Steve Fink [:sfink] [:s:]]

(In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #1)
> Did the newly-accessorized properties get removed from the lookup ops for
> all the relevant classes?  Without having tested, I'm guessing that didn't
> happen, needs to happen.

Uh, there's something strange going on here. I'm not sure if I landed what I meant to land. My current version definitely removes all of them, but current m-i still has them. Investigating...

Comment 3

[Steve Fink [:sfink] [:s:]]

Oh, never mind. My m-i checkout was stuck at an old version. So that's not it; I'll have to dig in to see what's going on with this bug.

Comment 4

[Steve Fink [:sfink] [:s:]]

Ok, the problem is that I was setting both JSPROP_READONLY and JSPROP_GETTER, not having read that JSPROP_READONLY is valid if JSPROP_GETTER is not used.

So I can fix that trivially by removing JSPROP_READONLY, but I don't understand what JSPROP_* flags I'm supposed to be using for all the places where they show up. I just don't understand their semantics well enough, nor do I understand what eg TypedArray::obj_getPropertyAttributes gets called for. It's total gibberish to me.

Waldo, when you have time, can you educatify me?

Comment 5

[Jeff Walden [:Waldo]]

It is a bug to use JSPROP_READONLY on a property that has JSPROP_GETTER or JSPROP_SETTER defined in it.  So that definitely should be removed.

As for the other point: getPropertyAttributes, getProperty, and so on only deal with own properties, so I think you should remove all mention of the properties you accessorized from those hooks.  The readonly bit is kind of a sideshow to the actual issue: these objects don't have these as own properties at all *and they must consistently answer that they don't have them* to queries of any sort on the matter.

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Assuming sec-critical unless otherwise shown.

Comment 7

[Steve Fink [:sfink] [:s:]]

Attachment: Typed array accessors should not use JSPROP_READONLY

This removes the problematic JSPROP_READONLY flag, removes mention of the accessor properties from the instance classes, and adds dummy properties on the prototypes to allow the named properties to be iterated normally.

The latter does not work. Begging for help on IRC rather than beating my head against it.

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 638042 [details] [diff] [review]
Typed array accessors should not use JSPROP_READONLY

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

::: js/src/jstypedarray.cpp
@@ +2942,5 @@
>          NULL,                /* clear       */                                 \
>      }                                                                          \
>  }
>  
> +static bool defineArrayBufferViewProtoProperties(JSContext *cx, HandleObject proto)

I don't understand why you have this method at all.  ArrayType::defineGetters is what defines these properties, as accessors, on typed array prototypes.  DefineDataViewGetter is what defines these properties, as accessors, on DataView.prototype.  And you've open-coded the getter creation and definition for the byteLength property on ArrayBuffer.prototype.  These places are what should have JSPROP_ENUMERATE and no JSPROP_READONLY and perhaps JSPROP_PERMANENT to be consistent with other ECMA function properties and so on.  This method seems entirely superfluous.

Comment 9

[Steve Fink [:sfink] [:s:]]

Attachment: Typed array accessors should not use JSPROP_READONLY

Now I know why you seemed to be wondering if I was a total idiot. Somehow I thought I had made DefineGetter put the property on the instance.

I notice that this includes the various named properties in for..in on the instance as well as on the prototype; any way to avoid that?

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 638081 [details] [diff] [review]
Typed array accessors should not use JSPROP_READONLY

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

for..in exposes enumerable properties along the entire prototype chain.

Pre-ES5 the only way to get all the enumerable properties directly on an object is to for..in and check each for hasOwnProperty.  Post-ES5 there's at least Object.keys() to avoid excess looping.  All this presents one reason why for..in is arguably broken, and one reason (I think I even remember Brendan saying roughly this, once) why property-based enumeration is a weak concept.

::: js/src/jstypedarray.cpp
@@ +1457,5 @@
>      static bool
>      DefineGetter(JSContext *cx, PropertyName *name, HandleObject proto)
>      {
>          RootedId id(cx, NameToId(name));
> +        unsigned flags = JSPROP_SHARED | JSPROP_PERMANENT | JSPROP_ENUMERATE | JSPROP_GETTER;

JSPROP_SHARED for this use goes hand-in-hand with JSPROP_GETTER, so put the two enums next to each other in this.

@@ +3038,5 @@
>      if (!LinkConstructorAndPrototype(cx, ctor, arrayBufferProto))
>          return NULL;
>  
>      RootedId byteLengthId(cx, NameToId(cx->runtime->atomState.byteLengthAtom));
> +    unsigned flags = JSPROP_SHARED | JSPROP_PERMANENT | JSPROP_GETTER;

Shared next to getter.

@@ +3134,5 @@
>  bool
>  DefineDataViewGetter(JSContext *cx, PropertyName *name, HandleObject proto)
>  {
>      RootedId id(cx, NameToId(name));
> +    unsigned flags = JSPROP_SHARED | JSPROP_PERMANENT | JSPROP_GETTER;

Shared next to getter.

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Assigning to Steve because he has an r+ patch pending some potential modifications prior to landing.

Comment 12

[Steve Fink [:sfink] [:s:]]

Thanks for the ping; I'd forgotten I hadn't landed this.

I just tried, and it bounced from failed tests. I'm not sure if it's the code or the tests yet, but I'll get this landed soon.

Comment 13

[Steve Fink [:sfink] [:s:]]

Doh! Accidentally reset the flags somehow.

Comment 14

[Ed Morley [:emorley]]

Landed:
https://hg.mozilla.org/mozilla-central/rev/d99d8b4d0eeb

But was backed out for M3 failures:
https://hg.mozilla.org/mozilla-central/rev/c4f0151df881

Comment 15

[Steve Fink [:sfink] [:s:]]

Attachment: Typed array accessors should not use JSPROP_READONLY

The previous patch triggered test failures because we have test code that
constructs a string with:

  ([String.fromCharCode(c) for each (c in Uint8Array(aBuffer, 0, 8))].join("")

which honestly seems like a perfectly reasonable thing to do in JS. But this breaks with the patch, because .byteLength and friends are [[Enumerable]] following WebIDL 4.4.6. In talking to Waldo, we agreed that this doesn't really seem JS-ish, so I'll ask for some sort of spec change. (Note that the .length is the same situation, which WebIDL specifically makes non-[[Enumerable]] for Arrays.) But for now, I intend to land without JSPROP_ENUMERATE.

Comment 16

[Steve Fink [:sfink] [:s:]]

Comment on attachment 641995 [details] [diff] [review]
Typed array accessors should not use JSPROP_READONLY

Verbal r+ on the change from Waldo.

Comment 17

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/430e25acf004

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/430e25acf004

Comment 19

[David Mandelin [:dmandelin]]

Looks like a tracking flag got cleared by mistake. Steve, could you nom for Aurora?

Comment 20

[Steve Fink [:sfink] [:s:]]

Comment on attachment 641995 [details] [diff] [review]
Typed array accessors should not use JSPROP_READONLY

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 734215

User impact if declined: assert failures in debug builds. Incorrect API exposed to web content, leading to compatibility headaches, dogs and cats living together, etc.

Testing completed (on m-c, etc.): it's been on m-c for a week, no problems so far. (If I hadn't pushed the wrong thing to try, it would already have been in Aurora.)

Risk to taking this patch (and alternatives if risky): Theoretically, it could expose something else bad. But it mostly removes code, and fixes a case where I was using flags in an illegal combination according to the JSAPI, so it's a clear improvement. It also updates the flag combination to comply with a very recent spec modification.

String or UUID changes made by this patch: none

Comment 21

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Setting checkin-needed for mozilla-aurora.

Comment 22

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/30546191916b

Comment 23

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 24

[u279076]

(In reply to Christian Holler (:decoder) from comment #23)
> JSBugMon: This bug has been automatically verified fixed.

Assuming this means verified against Firefox 17. Adding verifyme to verify against Firefox 16.

Comment 25

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929