435345 - Cannot accurately watch the 'length' property of arrays

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Cyrus Omar]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9) Gecko/2008051202 Firefox/3.0
Build Identifier: 

In trying to update a library I wrote for observable arrays (i.e. some callback is called when items are added or removed from the array) for FF3, I've encountered a regression. I add a watcher to the length property (among other things). This now doesn't work due to the latest Array optimizations initially:

var arr = [ ]
arr.watch('length', function() { /* do something */})
// TypeError: can't watch non-native objects of class Array

I know Brendan mentioned the optimization should be invisible to users, so this might be a bug in itself. However, this can be worked around by forcing the array to lose whatever these optimizations are if we want to observe it:

arr.__defineSetter__('dummy', function(){})
arr.dummy = 5
arr.watch('length', function(_,b,a) { return a; }) // called, but incorrectly

In prior versions of FF, this pseudo-worked (it would be double called but this could be worked around.) Now its not double called, but two problems:

1. b is always undefined (it should be the previous length). Another thing that can be worked around by keeping the previous length in some other variable.
2. This watcher seems to be accurately triggered most of the time:

arr.push(5)  // ok
arr.pop()    // ok
arr.length++ // ok

However, if you directly set an out of bounds index, it isn't called (and it was in Firefox 2):

arr[arr.length] = 5  // no callback; note that this is a common optimization people use

The only real workaround is to keep a watch on arr[arr.length] at all times and hope that nothing does strange things like arr[arr.length + 3] = 50, but I don't want to be forced to hope other peoples code doesn't do strange things.

This is currently being used in a production library for some intranet applications that would possibly prevent an upgrade to FF3 for users.

Reproducible: Always

Comment 1

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Sigh, watch.

I guess we need to fault to slow when a watch is set, for now -- complicating the SETELEM cases with watchpoint checks is not my idea of a good time.

That still leaves the setting of length, which I suspect is a separate but also straightforward fix.

Crowder, you up for these?  Bounce back to me if not.

Comment 2

[Brian Crowder]

Attachment: fallback to slow array on watch

Some suckiness about this patch:
* All objects pay for the IS_DENSE check.  S'ok, hopefully, since presumably obj_watch won't happen all that often.
* unwatch doesn't restore denseness.
* Means that the use of watch will result in performance penalty for JS programmers.  (is this a doc-needed thing?  evangelism?)
* Made MakeArraySlow no longer static to jsarray.cpp

Comment 3

[Brian Crowder]

Comment on attachment 324368 [details] [diff] [review]
fallback to slow array on watch

jsobj.cpp changes are the interesting part; the rest is s/MakeArraySlow>/js_MakeArraySlow/

Comment 4

[Brian Crowder]

Review-ping.

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 324368 [details] [diff] [review]
fallback to slow array on watch

r=shaver. sort of icky coupling, but we were already including jsarray.h in jsobj.cpp, so whatever!

Comment 6

[Brian Crowder]

changeset:   15602:9443787a6ee6

Comment 7

[Brian Crowder]

Attachment: applied to 1.9 branch

Comment 8

[Samuel Sidler (old account; do not CC)]

Can we get a test for this before landing in 1.9.0?

Comment 9

[Bob Clary [:bc] (inactive)]

(In reply to comment #8)
> Can we get a test for this before landing in 1.9.0?
> 

sure, coming right up.

Comment 10

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/extensions/regress-435345-01.js

passes 1.8.1 on mac, but fails with oldval=undefined on 1.9.1/mozilla-central

Comment 11

[Brian Crowder]

Comment on attachment 330508 [details]
js1_5/extensions/regress-435345-01.js

Looks great, thanks bc!

Comment 12

[Bob Clary [:bc] (inactive)]

crowder, the double call to the watcher in ": 2" on the 1.8.1 branch is expected? Reopen for the trunk for the oldval undefined errors or file a new bug?

Comment 13

[Brian Crowder]

1.8.1 does not have shavarrays, so bugs there are different.  Lemme check out the other errors before we proceed.

Comment 14

[Brian Crowder]

It may be that when we fault to a slow array, we need to reify the length property on the new slow array, so that the later "length" get doesn't fail.  Did that ever work for slow arrays?

Comment 15

[Samuel Sidler (old account; do not CC)]

Comment on attachment 329520 [details] [diff] [review]
applied to 1.9 branch

Approved for 1.9.0.2 on the condition that the test is landed where it needs to go as well. a=ss

Comment 16

[Bob Clary [:bc] (inactive)]

crowder, is this really fixed on mozilla-central given the failures on getting oldval? Is the patch really ready for 1.9.0 as is?

Comment 17

[Brian Crowder]

bclary:  Yeah, we can open those as another bug.  Does that happen on 1.8?

Comment 18

[Bob Clary [:bc] (inactive)]

crowder: 1.8.1 passes the existing test case as is including the double call to the watcher in ": 2" and does not exhibit the oldval=undefined problem. Before I check the test in and enshrine the double call as expected behavior, can I consider that a bug in 1.8.1 and modify test accordingly? I'll file a new bug on m-c for the oldval=undefined problem and block this.

Comment 19

[Bob Clary [:bc] (inactive)]

crowder, on mozilla-central this test case gives the following valgrind error:

==28877== Conditional jump or move depends on uninitialised value(s)
==28877==    at 0x47BA71: obj_watch(JSContext*, unsigned, long*) (jsobj.cpp:1437)
==28877==    by 0x4FFD67: js_Interpret (jsinterp.cpp:4928)
==28877==    by 0x466F5E: js_Execute (jsinterp.cpp:1553)
==28877==    by 0x40EDA8: JS_ExecuteScript (jsapi.cpp:4926)
==28877==    by 0x40A590: Process(JSContext*, JSObject*, char*, int) (js.cpp:272)
==28877==    by 0x40AD32: ProcessArgs(JSContext*, JSObject*, char**, int) (js.cpp:500)
==28877==    by 0x40B0AC: main (js.cpp:3967)

Is it meaningful?

Comment 20

[Bob Clary [:bc] (inactive)]

filed Bug 449152

Comment 21

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-435345-01.js,v  <--  regress-435345-01.js
initial revision: 1.1

mozilla-central: changeset:   16472:7b75a88e0d51

Comment 22

[Brian Crowder]

Attachment: Makes watch try to report the old value more correctly, but fails

As bc correctly reports, we still aren't reporting the "old" watched value correctly for arrays.  Faulting to slow-arrays doesn't solve that problem, because even for slow arrays, we often set "length" by accessing the slot directly rather than calling js_SetLengthProperty.  This is good for efficiency.  However, the penalty we pay for this is that, by the time js_SetLengthProperty /is/ called, the "old value" is usually the same as the new.  With the patch attached here:

js> a = [ ]; a.watch('length', function (x, a, b) { print(Array.prototype.slice.call(arguments)); return b });           
js> a.push(0)
length,1,1
1
js> a.push(1) 
length,2,2
2
js> a.push(2) 
length,3,3
3
js> a.pop()
length,3,2
2
js> a.pop()
length,2,1
1

Notice that pop() works correctly, because DeleteArrayElement does not muck with the length notion maintained internal to the array.  SetArrayElement eventually calls slowarray_addProperty, however, which /does/ increment the internal length -- all well before the watch is ever invoked.

I think we should punt on making this perfect, and leave things as they are.  Thoughts?

Comment 23

[Brian Crowder]

No one (shaver or Brendan) ever replied to comment #22?  I'd like to hear those thoughts before I push the 1.9.0.2 patch.

Comment 24

[Brendan Eich [:brendan]]

Please a followup bug on what's left in comment 22. What was fixed here, even if incomplete, is progress that we should ship.

/be

Comment 25

[Brian Crowder]

Landed attachment 329520 [details] [diff] [review] in cvs:

Checking in jsarray.c;
/cvsroot/mozilla/js/src/jsarray.c,v  <--  jsarray.c
new revision: 3.178; previous revision: 3.177
done
Checking in jsarray.h;
/cvsroot/mozilla/js/src/jsarray.h,v  <--  jsarray.h
new revision: 3.26; previous revision: 3.25
done
Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.476; previous revision: 3.475
done

Comment 26

[Brian Crowder]

446634 is the follow-up bug, fwiw.

Comment 27

[Bob Clary [:bc] (inactive)]

verified 1.9.0 modulo errors in bug 446634.