Closed Bug 1157577 Opened 7 years ago Closed 7 years ago

Assertion failure: getObjectFlags() == unowned->getObjectFlags(), at js/src/vm/Shape.cpp

Categories

(Core :: JavaScript Engine, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: gkw, Assigned: terrence)

References

Details

(Keywords: assertion, regression, testcase, Whiteboard: [jsbugmon:update])

Attachments

(2 files)

x = evalcx('');
gcslice(10);
for (var p in x) {}

asserts js debug shell on m-c changeset a5af73b32ac8 with --fuzzing-safe --no-threads --ion-eager at Assertion failure: getObjectFlags() == unowned->getObjectFlags(), at js/src/vm/Shape.cpp.

Configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/fuzzing/js/compileShell.py -b "--enable-debug --enable-more-deterministic --enable-nspr-build" -r a5af73b32ac8

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/6f7ff9108025
user:        Terrence Cole
date:        Tue Apr 14 13:28:39 2015 -0700
summary:     Bug 1154101 - Remove PushMarkStack indirection; r=sfink

Terrence, is bug 1154101 a likely regressor?
Flags: needinfo?(terrence)
Attached file stack
(lldb) bt 5
* thread #1: tid = 0xc2d81, 0x000000010029ebee js-dbg-64-dm-nsprBuild-darwin-a5af73b32ac8`js::BaseShape::assertConsistency(this=<unavailable>) + 158 at Shape.cpp:1239, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x000000010029ebee js-dbg-64-dm-nsprBuild-darwin-a5af73b32ac8`js::BaseShape::assertConsistency(this=<unavailable>) + 158 at Shape.cpp:1239
    frame #1: 0x000000010018c2a1 js-dbg-64-dm-nsprBuild-darwin-a5af73b32ac8`void js::GCMarker::traverse<js::BaseShape>(js::BaseShape*) [inlined] js::GCMarker::eagerlyMarkChildren(base=0x0000000103877218) + 8 at Marking.cpp:984
    frame #2: 0x000000010018c299 js-dbg-64-dm-nsprBuild-darwin-a5af73b32ac8`void js::GCMarker::traverse<js::BaseShape>(js::BaseShape*) + 9 at Marking.cpp:523
    frame #3: 0x000000010018c290 js-dbg-64-dm-nsprBuild-darwin-a5af73b32ac8`void js::GCMarker::traverse<js::BaseShape>(this=0x0000000101e691c0, thing=0x0000000103877218) + 16 at Marking.cpp:527
    frame #4: 0x00000001001aeadc js-dbg-64-dm-nsprBuild-darwin-a5af73b32ac8`js::GCMarker::eagerlyMarkChildren(this=0x0000000101e691c0, shape=<unavailable>) + 44 at Marking.cpp:962
(lldb)
This seems to have existed before my patch and the patch in question does not change timing or the code that runs. The problem does seem like it would be pretty sensitive to the GC timing, so I guess I just got unlucky. 

The problem is that the flags gets out of sync with unowned. The code in question is:
  *this = *other; // BaseShape::operator=
     // Copies flags, but not unowned.
     // State on exit is flagsNew and unownedOld.
  setUnowned(other)
     flags |= SHAPE_OWNED;
       // State here is flagsNew|OWNED and unownedOld.
     unowned_ = other; // before assignment, fires pre-barrier with above state.
       // pre-barrier ends up seeing flagsNew, which, obviously, does not necessarily match unownedOld->flags.

I've restructured the code to do:
  BaseShape::fromUnowned(other);  // replaces operator=
    ... other init ...
    unowned_ = &other; // pre barrier fires before flags are set, so unowned matches flags.
    flags = other.flags | SHAPE_OWNED; // set flags after unowned
Assignee: nobody → terrence
Status: NEW → ASSIGNED
Flags: needinfo?(terrence)
Attachment #8596877 - Flags: review?(bhackett1024)
Comment on attachment 8596877 [details] [diff] [review]
bug-1157577-v0.diff

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

::: js/src/vm/Shape.h
@@ +407,5 @@
>      const Class* clasp() const { return clasp_; }
>  
>      bool isOwned() const { return !!(flags & OWNED_SHAPE); }
>  
> +    static void fromUnowned(BaseShape& dest, UnownedBaseShape& src);

How about 'copyFromUnowned'
Attachment #8596877 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/mozilla-central/rev/47ac094b1917
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.