"Assertion failure: getter" redefining a non-lastProperty partial accessor property

RESOLVED FIXED in mozilla1.9.3a5

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: Waldo)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

Trunk
mozilla1.9.3a5
assertion, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 attachments)

(Reporter)

Description

8 years ago
var o = {}
Object.defineProperty(o, "x", {get: undefined, set: function() { Object.defineProperty(o, "x", {set: function() { }}); }, configurable: true});
o.a = 0; 
o.x = 0;

Assertion failure: getter, at ../jsscope.cpp:870
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
autoBisect shows this is probably related to bug 550402:

The first bad revision is:
changeset:   40452:53e381fe0f8b
user:        Jeff Walden
date:        Thu Apr 01 18:11:14 2010 -0700
summary:     Bug 550402 - Property readonly bit interferes with setter functions in properties.  r=jorendorff
Blocks: 550402
Keywords: regression
Simpler:

var o = {};
Object.defineProperty(o, "x", {get: undefined, set: function(){}, configurable: true });
o.a = 0;
o.x = 0;
Object.defineProperty(o, "x", { set: function(){} });
The proper description is:

"Assertion failure: getter" redefining property that's not the most recent property in an object whose scope is not in dictionary mode, when the original property was a half-accessor with an explicitly undefined absent half, and the new property doesn't include the explicitly undefined portion and is non-empty, with a different accessor function.

...but that's getting long enough either Bugzilla will complain or email clients won't handle gracefully.  Patch in a second, tests to follow either later tonight or tomorrow morning -- gotta go for a bit soon...
Summary: "Assertion failure: getter" with setter that calls defineProperty → "Assertion failure: getter" redefining a non-lastProperty partial accessor property
Created attachment 440400 [details] [diff] [review]
Patch

We pass in an already-normalized getter, which eventually triggers the assertion.  As for why it's not hit other ways, it's because JSScopeProperty::putProperty is called only for the one particular case in JSScopeProperty::changeProperty described in the previous comment, and the mega-tests don't hit this case.  Yet.  I'll add some more fun to them in the tests patch to come soonish.
Attachment #440400 - Flags: review?(dmandelin)
What is the purpose of the change to jsobj.cpp? It seems to work fine if just the assertions are removed.
The purpose is following the traditional contract that you use JS_PropertyStub for getters or setters if you just want the default one.  NormalizeGetterAndSetter will convert those to NULL for you at the proper time; passing in NULL too early, in some cases, will get you into trouble (for example, I think for JS_DefineProperty, NULL really means obj->getClass()->getProperty or obj->getClass()->setProperty).  Here I don't think it actually does, but this isn't the place to start deviating, and it's not especially obvious to me that this deviation doesn't introduce its own problems.
Comment on attachment 440400 [details] [diff] [review]
Patch

(In reply to comment #6)
> The purpose is following the traditional contract that you use JS_PropertyStub
> for getters or setters if you just want the default one. 
> NormalizeGetterAndSetter will convert those to NULL for you at the proper time;
> passing in NULL too early, in some cases, will get you into trouble (for
> example, I think for JS_DefineProperty, NULL really means
> obj->getClass()->getProperty or obj->getClass()->setProperty).  Here I don't
> think it actually does, but this isn't the place to start deviating, and it's
> not especially obvious to me that this deviation doesn't introduce its own
> problems.

OK. The docs for JS_DefineProperty say that it's fine to pass NULL. ISTR that in the past passing null instead of JSPropertyStub was hurting perf because the tracer thought there was a getter, or something like that. Hopefully we can clarify what these different arguments mean and get it cleaned up.
Attachment #440400 - Flags: review?(dmandelin) → review+
(In reply to comment #7)
> (From update of attachment 440400 [details] [diff] [review])
> (In reply to comment #6)
> > The purpose is following the traditional contract that you use JS_PropertyStub
> > for getters or setters if you just want the default one. 
> > NormalizeGetterAndSetter will convert those to NULL for you at the proper time;
> > passing in NULL too early, in some cases, will get you into trouble (for
> > example, I think for JS_DefineProperty, NULL really means
> > obj->getClass()->getProperty or obj->getClass()->setProperty).  Here I don't
> > think it actually does, but this isn't the place to start deviating, and it's
> > not especially obvious to me that this deviation doesn't introduce its own
> > problems.
> 
> OK. The docs for JS_DefineProperty say that it's fine to pass NULL. ISTR that
> in the past passing null instead of JSPropertyStub was hurting perf because the
> tracer thought there was a getter, or something like that.

That's what Waldo is talking about in comment 6:

"... for JS_DefineProperty, NULL really means obj->getClass()->getProperty or obj->getClass()->setProperty ..."

The class getter and setter can be expensive, and they're unnecessary on many specific properties (they may handle N>1 other built-in property cases). Old API botch.

It is confusing that to the public API, NULL means "class getter/setter, please" while lower in the engine, at the sprop layer, null getter and setter mean "no-op stub, JS_PropertyStub optimized away". But it's tighter than having to keep and call or compare the JS_PropertyStub pointer.

Really, we want to fix the API, or deprecate it and provide better API.

/be
http://hg.mozilla.org/tracemonkey/rev/5edc07b095c8

Still need to adjust the Object.defineProperty mega-tests to handle this...
Flags: in-testsuite?
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: fixed-in-tracemonkey
Target Milestone: --- → mozilla1.9.3a5
Created attachment 440933 [details] [diff] [review]
Tests for the non-lastProperty case (and also for the scope-is-dictionary case, for completeness)

The middle tests are the ones that test this bug -- and they fail without the patch -- and the dictionary ones are ensuring path-coverage completeness.
Attachment #440933 - Flags: review?(jorendorff)

Comment 11

8 years ago
http://hg.mozilla.org/mozilla-central/rev/5edc07b095c8
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Comment on attachment 440933 [details] [diff] [review]
Tests for the non-lastProperty case (and also for the scope-is-dictionary case, for completeness)

This is all good stuff, as always.

Filed bug 562684 to make these run in the browser.

Otherwise, just a few nits regarding duplicated code:

>diff --git a/js/src/tests/ecma_5/Object/15.2.3.6-dictionary-redefinition-1-of-4.js b/js/src/tests/ecma_5/Object/15.2.3.6-dictionary-redefinition-1-of-4.js
>new file mode 100644
>--- /dev/null
>+++ b/js/src/tests/ecma_5/Object/15.2.3.6-dictionary-redefinition-1-of-4.js
>@@ -0,0 +1,39 @@
>+// Any copyright is dedicated to the Public Domain.
>+// http://creativecommons.org/licenses/publicdomain/
>+
>+var PART = 1, PARTS = 4;
>+
>+var gTestfile = '15.2.3.6-dictionary-redefinition-1-of-4.js';
>+//-----------------------------------------------------------------------------
>+var BUGNUMBER = 560566;
>+var summary =
>+  'ES5 Object.defineProperty(O, P, Attributes): dictionary redefinition ' +
>+  PART + ' of ' + PARTS;
>+
>+print(BUGNUMBER + ": " + summary);
>+
>+load("ecma_5/Object/defineProperty-setup.js");
>+
>+/**************
>+ * BEGIN TEST *
>+ **************/
>+
>+try
>+{
>+  new TestRunner().runDictionaryPropertyPresentTestsFraction(PART, PARTS);
>+}
>+catch (e)
>+{
>+  throw "Error thrown during testing: " + e +
>+          " at line " + e.lineNumber + "\n" +
>+        (e.stack
>+          ? "Stack: " + e.stack.split("\n").slice(2).join("\n") + "\n"
>+          : "");
>+}
>+
>+/******************************************************************************/
>+
>+if (typeof reportCompare === "function")
>+  reportCompare(true, true);
>+
>+print("Tests complete!");
>diff --git a/js/src/tests/ecma_5/Object/15.2.3.6-dictionary-redefinition-2-of-4.js b/js/src/tests/ecma_5/Object/15.2.3.6-dictionary-redefinition-2-of-4.js
>new file mode 100644
>--- /dev/null
>+++ b/js/src/tests/ecma_5/Object/15.2.3.6-dictionary-redefinition-2-of-4.js
>@@ -0,0 +1,39 @@
>+// Any copyright is dedicated to the Public Domain.
>+// http://creativecommons.org/licenses/publicdomain/
>+
>+var PART = 2, PARTS = 4;
>+
>+var gTestfile = '15.2.3.6-dictionary-redefinition-2-of-4.js';
>+//-----------------------------------------------------------------------------
>+var BUGNUMBER = 560566;
>+var summary =
>+  'ES5 Object.defineProperty(O, P, Attributes): dictionary redefinition ' +
>+  PART + ' of ' + PARTS;
>+
>+print(BUGNUMBER + ": " + summary);
>+
>+load("ecma_5/Object/defineProperty-setup.js");
>+
>+/**************
>+ * BEGIN TEST *
>+ **************/
>+
>+try
>+{
>+  new TestRunner().runDictionaryPropertyPresentTestsFraction(PART, PARTS);
>+}
>+catch (e)
>+{
>+  throw "Error thrown during testing: " + e +
>+          " at line " + e.lineNumber + "\n" +
>+        (e.stack
>+          ? "Stack: " + e.stack.split("\n").slice(2).join("\n") + "\n"
>+          : "");
>+}
>+
>+/******************************************************************************/
>+
>+if (typeof reportCompare === "function")
>+  reportCompare(true, true);
>+
>+print("Tests complete!");
>diff --git a/js/src/tests/ecma_5/Object/15.2.3.6-dictionary-redefinition-3-of-4.js b/js/src/tests/ecma_5/Object/15.2.3.6-dictionary-redefinition-3-of-4.js
>new file mode 100644
>--- /dev/null
>+++ b/js/src/tests/ecma_5/Object/15.2.3.6-dictionary-redefinition-3-of-4.js
>@@ -0,0 +1,39 @@
>+// Any copyright is dedicated to the Public Domain.
>+// http://creativecommons.org/licenses/publicdomain/
>+
>+var PART = 3, PARTS = 4;
>+
>+var gTestfile = '15.2.3.6-dictionary-redefinition-3-of-4.js';
>+//-----------------------------------------------------------------------------
>+var BUGNUMBER = 560566;
>+var summary =
>+  'ES5 Object.defineProperty(O, P, Attributes): dictionary redefinition ' +
>+  PART + ' of ' + PARTS;
>+
>+print(BUGNUMBER + ": " + summary);
>+
>+load("ecma_5/Object/defineProperty-setup.js");
>+
>+/**************
>+ * BEGIN TEST *
>+ **************/
>+
>+try
>+{
>+  new TestRunner().runDictionaryPropertyPresentTestsFraction(PART, PARTS);
>+}
>+catch (e)
>+{
>+  throw "Error thrown during testing: " + e +
>+          " at line " + e.lineNumber + "\n" +
>+        (e.stack
>+          ? "Stack: " + e.stack.split("\n").slice(2).join("\n") + "\n"
>+          : "");
>+}
>+
>+/******************************************************************************/
>+
>+if (typeof reportCompare === "function")
>+  reportCompare(true, true);
>+
>+print("Tests complete!");
>diff --git a/js/src/tests/ecma_5/Object/15.2.3.6-dictionary-redefinition-4-of-4.js b/js/src/tests/ecma_5/Object/15.2.3.6-dictionary-redefinition-4-of-4.js
>new file mode 100644
>--- /dev/null
>+++ b/js/src/tests/ecma_5/Object/15.2.3.6-dictionary-redefinition-4-of-4.js
>@@ -0,0 +1,39 @@
>+// Any copyright is dedicated to the Public Domain.
>+// http://creativecommons.org/licenses/publicdomain/
>+
>+var PART = 4, PARTS = 4;
>+
>+var gTestfile = '15.2.3.6-dictionary-redefinition-4-of-4.js';
>+//-----------------------------------------------------------------------------
>+var BUGNUMBER = 560566;
>+var summary =
>+  'ES5 Object.defineProperty(O, P, Attributes): dictionary redefinition ' +
>+  PART + ' of ' + PARTS;
>+
>+print(BUGNUMBER + ": " + summary);
>+
>+load("ecma_5/Object/defineProperty-setup.js");
>+
>+/**************
>+ * BEGIN TEST *
>+ **************/
>+
>+try
>+{
>+  new TestRunner().runDictionaryPropertyPresentTestsFraction(PART, PARTS);
>+}
>+catch (e)
>+{
>+  throw "Error thrown during testing: " + e +
>+          " at line " + e.lineNumber + "\n" +
>+        (e.stack
>+          ? "Stack: " + e.stack.split("\n").slice(2).join("\n") + "\n"
>+          : "");
>+}
>+
>+/******************************************************************************/
>+
>+if (typeof reportCompare === "function")
>+  reportCompare(true, true);
>+
>+print("Tests complete!");
>diff --git a/js/src/tests/ecma_5/Object/15.2.3.6-middle-redefinition-1-of-4.js b/js/src/tests/ecma_5/Object/15.2.3.6-middle-redefinition-1-of-4.js
>new file mode 100644
>--- /dev/null
>+++ b/js/src/tests/ecma_5/Object/15.2.3.6-middle-redefinition-1-of-4.js
>@@ -0,0 +1,39 @@
>+// Any copyright is dedicated to the Public Domain.
>+// http://creativecommons.org/licenses/publicdomain/
>+
>+var PART = 1, PARTS = 4;
>+
>+var gTestfile = '15.2.3.6-middle-redefinition-1-of-4.js';
>+//-----------------------------------------------------------------------------
>+var BUGNUMBER = 560566;
>+var summary =
>+  'ES5 Object.defineProperty(O, P, Attributes): middle redefinition ' +
>+  PART + ' of ' + PARTS;
>+
>+print(BUGNUMBER + ": " + summary);
>+
>+load("ecma_5/Object/defineProperty-setup.js");
>+
>+/**************
>+ * BEGIN TEST *
>+ **************/
>+
>+try
>+{
>+  new TestRunner().runNonTerminalPropertyPresentTestsFraction(PART, PARTS);
>+}
>+catch (e)
>+{
>+  throw "Error thrown during testing: " + e +
>+          " at line " + e.lineNumber + "\n" +
>+        (e.stack
>+          ? "Stack: " + e.stack.split("\n").slice(2).join("\n") + "\n"
>+          : "");
>+}
>+
>+/******************************************************************************/
>+
>+if (typeof reportCompare === "function")
>+  reportCompare(true, true);
>+
>+print("Tests complete!");
>diff --git a/js/src/tests/ecma_5/Object/15.2.3.6-middle-redefinition-2-of-4.js b/js/src/tests/ecma_5/Object/15.2.3.6-middle-redefinition-2-of-4.js
>new file mode 100644
>--- /dev/null
>+++ b/js/src/tests/ecma_5/Object/15.2.3.6-middle-redefinition-2-of-4.js
>@@ -0,0 +1,39 @@
>+// Any copyright is dedicated to the Public Domain.
>+// http://creativecommons.org/licenses/publicdomain/
>+
>+var PART = 2, PARTS = 4;
>+
>+var gTestfile = '15.2.3.6-middle-redefinition-2-of-4.js';
>+//-----------------------------------------------------------------------------
>+var BUGNUMBER = 560566;
>+var summary =
>+  'ES5 Object.defineProperty(O, P, Attributes): middle redefinition ' +
>+  PART + ' of ' + PARTS;
>+
>+print(BUGNUMBER + ": " + summary);
>+
>+load("ecma_5/Object/defineProperty-setup.js");
>+
>+/**************
>+ * BEGIN TEST *
>+ **************/
>+
>+try
>+{
>+  new TestRunner().runNonTerminalPropertyPresentTestsFraction(PART, PARTS);
>+}
>+catch (e)
>+{
>+  throw "Error thrown during testing: " + e +
>+          " at line " + e.lineNumber + "\n" +
>+        (e.stack
>+          ? "Stack: " + e.stack.split("\n").slice(2).join("\n") + "\n"
>+          : "");
>+}
>+
>+/******************************************************************************/
>+
>+if (typeof reportCompare === "function")
>+  reportCompare(true, true);
>+
>+print("Tests complete!");
>diff --git a/js/src/tests/ecma_5/Object/15.2.3.6-middle-redefinition-3-of-4.js b/js/src/tests/ecma_5/Object/15.2.3.6-middle-redefinition-3-of-4.js
>new file mode 100644
>--- /dev/null
>+++ b/js/src/tests/ecma_5/Object/15.2.3.6-middle-redefinition-3-of-4.js
>@@ -0,0 +1,39 @@
>+// Any copyright is dedicated to the Public Domain.
>+// http://creativecommons.org/licenses/publicdomain/
>+
>+var PART = 3, PARTS = 4;
>+
>+var gTestfile = '15.2.3.6-middle-redefinition-3-of-4.js';
>+//-----------------------------------------------------------------------------
>+var BUGNUMBER = 560566;
>+var summary =
>+  'ES5 Object.defineProperty(O, P, Attributes): middle redefinition ' +
>+  PART + ' of ' + PARTS;
>+
>+print(BUGNUMBER + ": " + summary);
>+
>+load("ecma_5/Object/defineProperty-setup.js");
>+
>+/**************
>+ * BEGIN TEST *
>+ **************/
>+
>+try
>+{
>+  new TestRunner().runNonTerminalPropertyPresentTestsFraction(PART, PARTS);
>+}
>+catch (e)
>+{
>+  throw "Error thrown during testing: " + e +
>+          " at line " + e.lineNumber + "\n" +
>+        (e.stack
>+          ? "Stack: " + e.stack.split("\n").slice(2).join("\n") + "\n"
>+          : "");
>+}
>+
>+/******************************************************************************/
>+
>+if (typeof reportCompare === "function")
>+  reportCompare(true, true);
>+
>+print("Tests complete!");
>diff --git a/js/src/tests/ecma_5/Object/15.2.3.6-middle-redefinition-4-of-4.js b/js/src/tests/ecma_5/Object/15.2.3.6-middle-redefinition-4-of-4.js
>new file mode 100644
>--- /dev/null
>+++ b/js/src/tests/ecma_5/Object/15.2.3.6-middle-redefinition-4-of-4.js
>@@ -0,0 +1,39 @@
>+// Any copyright is dedicated to the Public Domain.
>+// http://creativecommons.org/licenses/publicdomain/
>+
>+var PART = 4, PARTS = 4;
>+
>+var gTestfile = '15.2.3.6-middle-redefinition-4-of-4.js';
>+//-----------------------------------------------------------------------------
>+var BUGNUMBER = 560566;
>+var summary =
>+  'ES5 Object.defineProperty(O, P, Attributes): middle redefinition ' +
>+  PART + ' of ' + PARTS;
>+
>+print(BUGNUMBER + ": " + summary);
>+
>+load("ecma_5/Object/defineProperty-setup.js");
>+
>+/**************
>+ * BEGIN TEST *
>+ **************/
>+
>+try
>+{
>+  new TestRunner().runNonTerminalPropertyPresentTestsFraction(PART, PARTS);
>+}
>+catch (e)
>+{
>+  throw "Error thrown during testing: " + e +
>+          " at line " + e.lineNumber + "\n" +
>+        (e.stack
>+          ? "Stack: " + e.stack.split("\n").slice(2).join("\n") + "\n"
>+          : "");
>+}
>+
>+/******************************************************************************/
>+
>+if (typeof reportCompare === "function")
>+  reportCompare(true, true);
>+
>+print("Tests complete!");

Each of these 8 files could be this short:

  // Any copyright is dedicated to the Public Domain.
  // http://creativecommons.org/licenses/publicdomain/

  load("ecma_5/Object/defineProperty-setup.js");
  runDictionaryRedefinitionTests(1, 4);

with the common code in defineProperty-setup.js. I claim BUGNUMBER and summary
aren't needed; I could be wrong.

>+      function dictionaryPropertyPresentTests()
>+      {
>+        print("Running dictionary already-present tests now...");
>+
>+        var total = VALID_DESCRIPTORS.length;
>+        var start = Math.floor((part - 1) / parts * total);
>+        var end = Math.floor(part / parts * total);
>+
>+        for (var i = start; i < end; i++)
>+        {
>+          var old = VALID_DESCRIPTORS[i];
>+          print("Starting test with old descriptor " + old.toSource() + "...");
>+
>+          for (var j = 0, sz2 = VALID_DESCRIPTORS.length; j < sz2; j++)
>+          {
>+            self._runSinglePropertyPresentTest(old, VALID_DESCRIPTORS[j],
>+                                               middleDefines);
>+          }
>+        }
>+      }

This is duplicated too. DRY!

r=me either way but I'd be a lot happier if you could squeeze 300 lines or so out of it.
Attachment #440933 - Flags: review?(jorendorff) → review+
Test landing:

http://hg.mozilla.org/tracemonkey/rev/dabf99dbfcff

I chomped down the size of the repeated tests (and expanded them into eight files rather than four, because I was occasionally timing out on four :-\ ), but I didn't make the other repetition change -- a little more pain than I was eager to make at the time, and the duplication is somewhat less obvious between the two.  So halfway adjusted (the easy parts), but not all the way; I should keep to slightly more productive things. :-)

Comment 14

8 years ago
http://hg.mozilla.org/mozilla-central/rev/dabf99dbfcff
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.