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)
Tracking
()
VERIFIED
FIXED
mozilla61
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)
12.01 KB,
patch
|
tcampbell
:
review+
|
Details | Diff | Splinter Review |
183.98 KB,
patch
|
tcampbell
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 2•6 years ago
|
||
See bug 1454398 where the asserts got disabled: https://hg.mozilla.org/integration/mozilla-inbound/rev/42e037e0b8d1
Comment 3•6 years ago
|
||
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.
Comment 5•6 years ago
|
||
JSBugMon: Bisection requested, failed due to error (try manually).
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Comment 6•6 years ago
|
||
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 4af4ae0aee55).
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(jdemooij)
Updated•6 years ago
|
Assignee: nobody → jdemooij
Priority: -- → P1
Updated•6 years ago
|
tracking-firefox61:
--- → +
Assignee | ||
Comment 7•6 years ago
|
||
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)
Assignee | ||
Updated•6 years ago
|
Attachment #8968936 -
Flags: feedback?(tcampbell)
Assignee | ||
Comment 8•6 years ago
|
||
Simpler patch.
Attachment #8968936 -
Attachment is obsolete: true
Attachment #8968950 -
Flags: feedback?(tcampbell)
Updated•6 years ago
|
Keywords: sec-other
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:update,ignore][invalid assertions]
Assignee | ||
Comment 11•6 years ago
|
||
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)
Assignee | ||
Updated•6 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 12•6 years ago
|
||
Attachment #8968950 -
Attachment is obsolete: true
Attachment #8970128 -
Flags: review?(tcampbell)
Comment 13•6 years ago
|
||
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+
Assignee | ||
Comment 14•6 years ago
|
||
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 15•6 years ago
|
||
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+
Assignee | ||
Comment 16•6 years ago
|
||
Thanks for the feedback. This is green on Try btw.
Attachment #8970498 -
Attachment is obsolete: true
Attachment #8971941 -
Flags: review?(tcampbell)
Comment 17•6 years ago
|
||
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+
Assignee | ||
Comment 18•6 years ago
|
||
Pushed with comments addressed. https://hg.mozilla.org/integration/mozilla-inbound/rev/f07ea68c0fef2cb271b08358edd2c0cf0fd0aa37 https://hg.mozilla.org/integration/mozilla-inbound/rev/deeb18d57ad217978ff3c6a38481b5d100665279
Updated•6 years ago
|
status-firefox59:
--- → wontfix
status-firefox60:
--- → wontfix
status-firefox-esr52:
--- → wontfix
status-firefox-esr60:
--- → wontfix
Comment 19•6 years ago
|
||
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
Updated•6 years ago
|
Status: RESOLVED → VERIFIED
Comment 20•6 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Updated•6 years ago
|
Whiteboard: [jsbugmon:update,ignore][invalid assertions] → [jsbugmon:update,ignore][invalid assertions][adv-main61-]
Updated•4 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•