1082662 - Assertion failure: *thingp, at gc/Marking.cpp:166

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision 54217864bae9 (run with --no-threads --fuzzing-safe):


function test() {
Object.defineProperty(Object.prototype, "__proto__", { set: function() {}});
Object.defineProperty(Object.prototype, "__proto__", { set: undefined });
} test();

Comment 1

[Christian Holler (:decoder)]

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

Comment 2

[Christian Holler (:decoder)]

Marked s-s because it's GC-related.

Comment 3

[Jeff Walden [:Waldo]]

Better testcase:

  Object.defineProperty(Object.prototype, "x",
                        { get() { return 2; },
                          set(v) { },
                          configurable: true,
                          enumerable: false });
  function test()
  {
    Object.defineProperty(Object.prototype, "x", { set: function() {}});
    Object.defineProperty(Object.prototype, "x", { set: undefined });
  }
  test();

__proto__ has no involvement in this issue at all, so best not to mention it.

On the other hand, Object.prototype has *some* involvement.  Switching to a new object and defining on that doesn't have issues.  Using a new object, that's made into a delegate (by creating another object using it as [[Prototype]], also doesn't have issues.  I imagine whatever it is that makes Object.prototype special will show up in the course of debugging.

I'm quite curious why this issue was never caught by the exhaustive Object.defineProperty tests we have, that everyone loves to hate.  :-)  Perhaps because three steps of definition are required, and those tests only (as I recall) go to two?

Comment 4

[Jeff Walden [:Waldo]]

This looks, by my hazy understanding of the crash spot, like a barrier issue.  I don't know enough about GC and barriers.  Time to rectify.  Taking.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Don't crash trying to mark a dictionary accessor shape whose getter/setter field has previously been mutated from a callable to |undefined|. NOT REVIEWED YET

This bug was, I think, introduced between attachment 8500531 [details] [diff] [review] and attachment 8501596 [details] [diff] [review].  In the initial state, GetterSetterWriteBarrierPost used putRelocatableCell.  The corresponding buffer entry will return if the edge contains null (CellPtrEdge::mark).  The post-state uses putGeneric with ShapeGetterSetterRef, but ShapeGetterSetterRef::mark has no such early return if the edge contains null.  Add it here to fix things.

I...think this is how barriers are supposed to be used, but honestly I've never written a barrier patch before or really taken time to grok barriers, so this could be a wrong tack, or not entirely the right one.  :-)  I could also imagine something like if the new setter/getter is null, removing the store buffer entry entirely.  But then you can potentially have thrashing if someone mutates back and forth between undefined and callable for [[Get]] or [[Set]], so might as well not bother.  Or so I am hazily claiming based on my weak barrier-fu.

This being introduced in bug 1073700, it'll require uplift.  If I continue reading through MarkInternal it looks like we'll just null-deref at a fixed offset when computing |thing->zone()|, so I think this is not a security issue.

Comment 6

[Jan de Mooij [:jandem]]

My bad, thanks for fixing this Waldo!

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> I'm quite curious why this issue was never caught by the exhaustive
> Object.defineProperty tests we have, that everyone loves to hate.  :-) 

(I also used to hate those tests because they used to (?) timeout and are not easy to debug when they fail, but when I worked on bug 1073700 they found a few issues that no other test hit, so I appreciate them now ;)

Comment 8

[Terrence Cole [:terrence]]

Comment on attachment 8505034 [details] [diff] [review]
Don't crash trying to mark a dictionary accessor shape whose getter/setter field has previously been mutated from a callable to |undefined|.  NOT REVIEWED YET

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

Yes, this is the right solution.

Comment 9

[Jeff Walden [:Waldo]]

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

Unhiding as just a null-deref as far as I can tell...flagging for aurora approval momentarily.

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 8505034 [details] [diff] [review]
Don't crash trying to mark a dictionary accessor shape whose getter/setter field has previously been mutated from a callable to |undefined|.  NOT REVIEWED YET

Approval Request Comment
[Feature/regressing bug #]: bug 1073700
[User impact if declined]: crashes with certain manipulations of properties backed by getter/setter functions; semi-unlikely to be too common in the real world where getters/setters aren't changed much after creation, but someone probably does it, and it's early in cycle and a simple, clear fix
[Describe test coverage new/current, TBPL]: landed with test, narrowed down to the exact failing scenario pretty clearly
[Risks and why]: a null-check with early return is about as little risk as you can get, plus it's early
[String/UUID change made/needed]: N/A

Comment 11

[Jeff Walden [:Waldo]]

(In reply to Jan de Mooij [:jandem] from comment #7)
> (I also used to hate those tests because they used to (?) timeout and are
> not easy to debug when they fail, but when I worked on bug 1073700 they
> found a few issues that no other test hit, so I appreciate them now ;)

I am going to frame this and place it on a wall.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/95b84005cd63