951213 - Assertion failure: canRemoveLastProperty(), at jsobjinlines.h:123

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision b980c2dee2e7 (run with --fuzzing-safe --ion-eager):


setObjectMetadataCallback(function(obj) {});
function foo(x, y) {
  this.g = x + y;
}
var a = 0;
var b = { valueOf: function() Object.defineProperty(Object.prototype, 'g', {}) };
var c = new foo(a, b);

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/bec71542c055
user:        Brian Hackett
date:        Sat Dec 14 16:29:43 2013 -0800
summary:     Bug 950118 - Don't allow the object metadata hook to reenter JS, r=jimb.

This iteration took 1.020 seconds to run.

Comment 3

[Brian Hackett [Laid off!]]

Attachment: patch

The logic used when rolling back object properties when TI's definite properties analysis gets invalidated wasn't able to cope with objects that have metadata.  JSObjet::removeProperty is a more generic API that can cope with this case.

Comment 4

[Brian Hackett [Laid off!]]

Attachment: patch

Oops, wrong patch.

Comment 5

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/39778c27c5fd

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/dcad449c5d00 for causing ggc test failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=32296712&tree=Mozilla-Inbound

Comment 7

[Brian Hackett [Laid off!]]

Attachment: updated

GGC builds break on the new testcase even without the patch.  This patch fixes GGC to handle the testcase and avoids expensive enabling/disabling of the nursery every time the metadata callback is invoked.

Note that not rekeying initial shape table entries for nursery metadata things won't lead to any incorrect behavior, though some extra shapes might end up being created.  The same holds for parent, but not proto --- |InitialShapeEntry::proto| should be fixed up, or subtly incorrect behavior (not crashes) could happen.  (If the same |proto| address is used for two different prototype objects then objects with different protos could have the same shape, which will confuse jitcode prototype-traversing caches.)  It would be best if this behaved like TypeObjectWithNewScriptEntry, though again this is a separate issue from this patch.

Comment 8

[Terrence Cole [:terrence]]

Comment on attachment 8351494 [details] [diff] [review]
updated

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

Great! r=me

The InitialShapeSet issue is worrisome. I'll file a bug.

Comment 10

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/057498186852

Comment 11

[Phil Ringnalda (:philor)]

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