Object.observe: prevent observed objects from using fast elements.

This is necessary because polymorphic stores generally
do not perform a map check but only an instance type check,
which misses out on changes in the observation status.
Unfortunately, there currently is no efficient way in V8
to maintain that optimisation in the presence of Object.observe.

R=mstarzinger@chromium.org
BUG=v8:2409

Review URL: https://codereview.chromium.org/11477006

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@13205 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/objects-inl.h

@@ -1426,6 +1426,13 @@ MaybeObject* JSObject::ResetElements() {
   if (!maybe_obj->ToObject(&obj)) return maybe_obj;
   set_map(Map::cast(obj));
   initialize_elements();
+  if (FLAG_harmony_observation && map()->is_observed()) {
+    // Maintain invariant that observed elements are always in dictionary mode.
+    // For this to work on arrays, we have to make sure to reset length first.
+    if (IsJSArray()) JSArray::cast(this)->set_length(Smi::FromInt(0));
+    maybe_obj = NormalizeElements();
+    if (maybe_obj->IsFailure()) return maybe_obj;
+  }
   return this;
 }
 

src/objects.cc

@@ -9424,6 +9424,7 @@ MaybeObject* JSObject::SetFastElementsCapacityAndLength(
   Heap* heap = GetHeap();
   // We should never end in here with a pixel or external array.
   ASSERT(!HasExternalArrayElements());
+  ASSERT(!map()->is_observed());
 
   // Allocate a new fast elements backing store.
   FixedArray* new_elements;
@@ -9488,6 +9489,7 @@ MaybeObject* JSObject::SetFastDoubleElementsCapacityAndLength(
   Heap* heap = GetHeap();
   // We should never end in here with a pixel or external array.
   ASSERT(!HasExternalArrayElements());
+  ASSERT(!map()->is_observed());
 
   FixedArrayBase* elems;
   { MaybeObject* maybe_obj =
@@ -10631,6 +10633,7 @@ Handle<Object> JSObject::TransitionElementsKind(Handle<JSObject> object,
 
 
 MaybeObject* JSObject::TransitionElementsKind(ElementsKind to_kind) {
+  ASSERT(!map()->is_observed());
   ElementsKind from_kind = map()->elements_kind();
 
   if (IsFastHoleyElementsKind(from_kind)) {
@@ -10887,6 +10890,9 @@ bool JSObject::ShouldConvertToFastElements() {
   // An object requiring access checks is never allowed to have fast
   // elements.  If it had fast elements we would skip security checks.
   if (IsAccessCheckNeeded()) return false;
+  // Observed objects may not go to fast mode because they rely on map checks,
+  // and for fast element accesses we sometimes check element kinds only.
+  if (FLAG_harmony_observation && map()->is_observed()) return false;
 
   FixedArray* elements = FixedArray::cast(this->elements());
   SeededNumberDictionary* dictionary = NULL;

src/runtime.cc

@@ -13493,6 +13493,13 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_SetIsObserved) {
     if (!maybe->To(&map)) return maybe;
     map->set_is_observed(is_observed);
     obj->set_map(map);
+    if (is_observed && obj->IsJSObject() &&
+        !JSObject::cast(obj)->HasExternalArrayElements()) {
+      // Go to dictionary mode, so that we don't skip map checks.
+      maybe = JSObject::cast(obj)->NormalizeElements();
+      if (maybe->IsFailure()) return maybe;
+      ASSERT(!JSObject::cast(obj)->HasFastElements());
+    }
   }
   return isolate->heap()->undefined_value();
 }

test/mjsunit/harmony/object-observe.js

@@ -26,6 +26,7 @@
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 // Flags: --harmony-observation --harmony-proxies --harmony-collections
+// Flags: --allow-natives-syntax
 
 var allObservers = [];
 function reset() {
@@ -871,3 +872,80 @@ Object.observe(obj, observer.callback);
 obj.prototype = 7;
 Object.deliverChangeRecords(observer.callback);
 observer.assertNotCalled();
+
+
+// Check that changes in observation status are detected in all IC states and
+// in optimized code, especially in cases usually using fast elements.
+function TestFastElements(prepopulate, polymorphic, optimize) {
+  var setElement = eval(
+    "(function setElement(a, i, v) { a[i] = v " +
+    "/* " + prepopulate + " " + polymorphic + " " + optimize + " */" +
+    "})"
+  );
+  print("TestFastElements:", setElement);
+
+  var arr = prepopulate ? [1, 2, 3, 4, 5] : [0];
+  setElement(arr, 1, 210);
+  setElement(arr, 1, 211);
+  if (polymorphic) setElement(["M", "i", "l", "n", "e", "r"], 0, "m");
+  if (optimize) %OptimizeFunctionOnNextCall(setElement);
+  setElement(arr, 1, 212);
+
+  reset();
+  Object.observe(arr, observer.callback);
+  setElement(arr, 1, 989898);
+  Object.deliverChangeRecords(observer.callback);
+  observer.assertCallbackRecords([
+    { object: arr, name: '1', type: 'updated', oldValue: 212 }
+  ]);
+}
+
+for (var b1 = 0; b1 < 2; ++b1)
+  for (var b2 = 0; b2 < 2; ++b2)
+    for (var b3 = 0; b3 < 2; ++b3)
+      TestFastElements(b1 != 0, b2 != 0, b3 != 0);
+
+
+function TestFastElementsLength(polymorphic, optimize, oldSize, newSize) {
+  var setLength = eval(
+    "(function setLength(a, n) { a.length = n " +
+    "/* " + polymorphic + " " + optimize + " " + oldSize + " " + newSize + " */"
+    + "})"
+  );
+  print("TestFastElementsLength:", setLength);
+
+  function array(n) {
+    var arr = new Array(n);
+    for (var i = 0; i < n; ++i) arr[i] = i;
+    return arr;
+  }
+
+  setLength(array(oldSize), newSize);
+  setLength(array(oldSize), newSize);
+  if (polymorphic) setLength(array(oldSize).map(isNaN), newSize);
+  if (optimize) %OptimizeFunctionOnNextCall(setLength);
+  setLength(array(oldSize), newSize);
+
+  reset();
+  var arr = array(oldSize);
+  Object.observe(arr, observer.callback);
+  setLength(arr, newSize);
+  Object.deliverChangeRecords(observer.callback);
+  if (oldSize === newSize) {
+    observer.assertNotCalled();
+  } else {
+    var count = oldSize > newSize ? oldSize - newSize : 0;
+    observer.assertRecordCount(count + 1);
+    var lengthRecord = observer.records[count];
+    assertSame(arr, lengthRecord.object);
+    assertEquals('length', lengthRecord.name);
+    assertEquals('updated', lengthRecord.type);
+    assertSame(oldSize, lengthRecord.oldValue);
+  }
+}
+
+for (var b1 = 0; b1 < 2; ++b1)
+  for (var b2 = 0; b2 < 2; ++b2)
+    for (var n1 = 0; n1 < 3; ++n1)
+      for (var n2 = 0; n2 < 3; ++n2)
+        TestFastElementsLength(b1 != 0, b2 != 0, 20*n1, 20*n2);

test/mjsunit/regress/regress-observe-empty-double-array.js

@@ -32,6 +32,7 @@
 arr = [1.1];
 Object.observe(arr, function(){});
 arr.length = 0;
-assertTrue(%HasFastDoubleElements(arr));
+// TODO(observe): we currently disallow fast elements for observed object.
+// assertTrue(%HasFastDoubleElements(arr));
 // Should not crash
 arr.push(1.1);