Closed Bug 1454887 Opened 6 years ago Closed 6 years ago

Crash [@ js::ObjectGroup::sweep] or Assertion failure: !zone()->types.assertNoTISweeping, at vm/TypeInference.cpp:4330

Categories

(Core :: JavaScript Engine, defect, P1)

x86_64
Linux
defect

Tracking

()

VERIFIED FIXED
mozilla61
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 + verified

People

(Reporter: decoder, Assigned: jandem)

References

Details

(5 keywords, Whiteboard: [jsbugmon:update,ignore][invalid assertions][adv-main61-])

Crash Data

Attachments

(2 files, 3 obsolete files)

The following testcase crashes on mozilla-central revision f94b64e00202 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --disable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off):

gczeal(17, 2);
function Foo(a, b) {
    this.a = a;
}
function invalidate_foo() {
    var a = [];
    for (var i = 0; i < 50; i++) a.push(new Foo(i, i + 1));
    Object.defineProperty(Foo.prototype, "a", {
        configurable: true,
        set: function() {
            counter++;
        }
    });
}
invalidate_foo();


Backtrace:

received signal SIGSEGV, Segmentation fault.
js::ObjectGroup::sweep (this=this@entry=0x7ffff4c7c040, oom=oom@entry=0x0) at js/src/vm/TypeInference.cpp:4330
#0  js::ObjectGroup::sweep (this=this@entry=0x7ffff4c7c040, oom=oom@entry=0x0) at js/src/vm/TypeInference.cpp:4330
#1  0x0000000000a089d6 in js::ObjectGroup::maybeSweep (this=this@entry=0x7ffff4c7c040, oom=0x0) at js/src/vm/ObjectGroup-inl.h:27
#2  0x0000000000a089f9 in js::ObjectGroup::newScript (this=0x7ffff4c7c040) at js/src/vm/ObjectGroup-inl.h:101
#3  js::ObjectGroup::anyNewScript (this=0x7ffff4c7c040) at js/src/vm/TypeInference.cpp:2939
#4  0x0000000000a11cd2 in js::ObjectGroup::clearNewScript (this=0x7ffff4c7c040, cx=0x7ffff5f14000, replacement=0x0) at js/src/vm/TypeInference.cpp:3001
#5  0x0000000000a13f4f in js::HeapTypeSet::newPropertyState (cx=0x7ffff5f14000, this=0x7ffff49da028) at js/src/vm/TypeInference-inl.h:962
#6  js::HeapTypeSet::setNonDataProperty (cx=0x7ffff5f14000, this=0x7ffff49da028) at js/src/vm/TypeInference-inl.h:979
#7  js::ObjectGroup::markPropertyNonData (this=<optimized out>, cx=cx@entry=0x7ffff5f14000, obj=obj@entry=0x7ffff4cae040, id=...) at js/src/vm/TypeInference.cpp:2829
#8  0x00000000009864bf in js::MarkTypePropertyNonData (cx=cx@entry=0x7ffff5f14000, obj=obj@entry=0x7ffff4cae040, id=..., id@entry=...) at js/src/vm/TypeInference-inl.h:454
#9  0x00000000009685b5 in UpdateShapeTypeAndValue (value=..., id=..., shape=<optimized out>, obj=<optimized out>, cx=<optimized out>) at js/src/vm/NativeObject.cpp:1272
#10 AddOrChangeProperty<(IsAddOrChange)1> (desc=..., id=..., obj=..., cx=<optimized out>) at js/src/vm/NativeObject.cpp:1481
#11 js::NativeDefineProperty (cx=0x7ffff5f14000, obj=..., id=..., desc_=..., result=...) at js/src/vm/NativeObject.cpp:1898
#12 0x000000000096a099 in js::DefineProperty (cx=<optimized out>, obj=..., id=..., desc=..., result=...) at js/src/vm/JSObject.cpp:2790
#13 0x00000000009ab7f1 in intrinsic_DefineProperty (cx=0x7ffff5f14000, argc=<optimized out>, vp=0x7ffff49ca288) at js/src/vm/SelfHosting.cpp:630
#14 0x0000000000523291 in js::CallJSNative (args=..., native=0x9ab5f0 <intrinsic_DefineProperty(JSContext*, unsigned int, JS::Value*)>, cx=0x7ffff5f14000) at js/src/vm/JSContext-inl.h:280
#15 js::InternalCallOrConstruct (cx=0x7ffff5f14000, args=..., construct=<optimized out>) at js/src/vm/Interpreter.cpp:467
#16 0x000000000051e61c in js::CallFromStack (args=..., cx=<optimized out>) at js/src/vm/Interpreter.cpp:522
#17 Interpret (cx=0x7ffff5f14000, state=...) at js/src/vm/Interpreter.cpp:3084
[...]
#35 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:9146
rax	0x1c6b1c0	29798848
rbx	0x7ffff4c7c040	140737300119616
rcx	0xeaecb0	15396016
rdx	0x20000008	536870920
rsi	0x7ffff5f79000	140737320030208
rdi	0x7ffff4c7c040	140737300119616
rbp	0x7ffff5f14000	140737319616512
rsp	0x7fffffffbb30	140737488337712
r8	0x100	256
r9	0x3	3
r10	0x1	1
r11	0xdc	220
r12	0x7ffff5f14000	140737319616512
r13	0x7ffff4c7c000	140737300119552
r14	0x0	0
r15	0x0	0
rip	0xa087f1 <js::ObjectGroup::sweep(js::AutoClearTypeInferenceStateOnOOM*)+1537>
=> 0xa087f1 <js::ObjectGroup::sweep(js::AutoClearTypeInferenceStateOnOOM*)+1537>:	movl   $0x0,0x0
   0xa087fc <js::ObjectGroup::sweep(js::AutoClearTypeInferenceStateOnOOM*)+1548>:	ud2


Marking s-s because this is a TI/GC assertion and test uses gczeal.
This is an instance of the recently added (and now removed) TI sweeping assertion going off.

Jan, is this also a false positive?  I can't immediately tell if this is dangerous or not.
JSBugMon: Bisection requested, failed due to error (try manually).
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 4af4ae0aee55).
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
Flags: needinfo?(jdemooij)
Assignee: nobody → jdemooij
Priority: -- → P1
Attached patch Rework asserts (obsolete) — Splinter Review
The asserts added in bug 1447989 were too strict so I had to disable them.

This patch tries a different approach: it adds AutoAssertDoesNotNeedSweep where we assert the group or script doesn't need TI sweeping, then we pass a reference to AutoAssertDoesNotNeedSweep down to constraintList.

This would have found the original bug (bug 1447989) but should avoid the false positives I think.

Ted, WDYT?
Flags: needinfo?(jdemooij)
Attachment #8968936 - Flags: feedback?(tcampbell)
Attachment #8968936 - Flags: feedback?(tcampbell)
Attached patch Rework asserts (obsolete) — Splinter Review
Simpler patch.
Attachment #8968936 - Attachment is obsolete: true
Attachment #8968950 - Flags: feedback?(tcampbell)
Keywords: sec-other
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:update,ignore][invalid assertions]
Comment on attachment 8968950 [details] [diff] [review]
Rework asserts

OK I'm done with these bugs.

I'm going to rewrite TI to make all the implicit sweeping explicit.
Attachment #8968950 - Flags: feedback?(tcampbell)
Status: NEW → ASSIGNED
Attachment #8968950 - Attachment is obsolete: true
Attachment #8970128 - Flags: review?(tcampbell)
Comment on attachment 8970128 [details] [diff] [review]
Part 1 - Remove invalid asserts

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

Reverting the imprecise asserts seems like the right plan.
Attachment #8970128 - Flags: review?(tcampbell) → review+
Attached patch Part 2 (obsolete) — Splinter Review
This adds AutoSweepObjectGroup and AutoSweepTypScript, and all TI sweeping must use one of these classes.

Methods that used to call maybeSweep now take a reference to one of these classes. It's more verbose, but personally I think it's a bit easier to reason about.

What do you think? If you don't think it's an improvement I don't mind dropping this patch. I also think we might want to block this on unboxed object removal because a lot of the changes here are unboxed-object related.
Attachment #8970498 - Flags: feedback?(tcampbell)
Comment on attachment 8970498 [details] [diff] [review]
Part 2

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

I like this explicit approach. Our current system is a textbook example of a source of Heisenbugs. I could imagine us reverting the explicit style in the future if we have better primitives and static analysis (basically make it owned by GC and using similar level of wrapper types and static checks).

I'd be in favor of landing this before unboxed-object changes to avoid too many moving targets. We already have strong evidence that this TI sweeping is responsible for browser instability and we should consider trying to land this for FF61 (either before merge or via uplift to beta).
Attachment #8970498 - Flags: feedback?(tcampbell) → feedback+
Thanks for the feedback. This is green on Try btw.
Attachment #8970498 - Attachment is obsolete: true
Attachment #8971941 - Flags: review?(tcampbell)
Comment on attachment 8971941 [details] [diff] [review]
Part 2 - Make TI sweeping more explicit

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

Great! TI sweeping had too many subtleties to hide the way we did. It is nice that no longer hide sweeps in asserts as well.

Optionally, cleaning up ObjectGroup::print() would be nice, be feel free to land as is.

Going forward, this exercise has raised the question of should we still incrementally sweep when using accessors that don't strictly need sweeping.

::: js/src/gc/Marking.cpp
@@ +1672,3 @@
>          return true;
>  
> +    HeapTypeSet* typeSet = group->maybeGetPropertyDontCheckGeneration(JSID_VOID);

// This typeset doesn't escape this function so avoid sweeping here.

(or something to that effect)

::: js/src/vm/JSObject.cpp
@@ +1039,5 @@
> +                    // The script was analyzed successfully and may have changed
> +                    // the new type table, so refetch the group.
> +                    group = ObjectGroup::defaultNewGroup(cx, nullptr, TaggedProto(proto),
> +                                                         newTarget);
> +                    AutoSweepObjectGroup sweepNewGroup(group);

Interesting debug/release divergence..

::: js/src/vm/TypeInference.cpp
@@ +3114,5 @@
>              : tagged.isDynamic()
>              ? "(dynamic)"
>              : "(null)");
>  
> +    AutoSweepObjectGroup sweep(this);

Can we pull this out? It is a weird side-effect to leave in a print function. We can add separate override to call from debugger if we need.

::: js/src/vm/TypeInference.h
@@ +736,5 @@
>              ReportMagicWordFailure(magic_, ConstraintTypeSetMagic, uintptr_t(flags), uintptr_t(objectSet));
>  #endif
>      }
>  
> +    // This takes a reference to AutoSweepBase to ensure we swept the owning

Nice.
Attachment #8971941 - Flags: review?(tcampbell) → review+
Blocks: 1457834
https://hg.mozilla.org/mozilla-central/rev/f07ea68c0fef
https://hg.mozilla.org/mozilla-central/rev/deeb18d57ad2
Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Whiteboard: [jsbugmon:update,ignore][invalid assertions] → [jsbugmon:update,ignore][invalid assertions][adv-main61-]
Group: core-security-release
Keywords: bugmon
You need to log in before you can comment on or make changes to this bug.