837714 - Assertion failure: allocated(), at ../../gc/Heap.h:472 or Opt-Crash [@ js::GCMarker::drainMarkStack]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

Attachment: Testcase for shell

The attached testcase asserts on mozilla-central revision 847e28c7ba67 (run with --ion-eager).

Comment 1

[Christian Holler (:decoder)]

Debug backtrace of assertion:


Program received signal SIGSEGV, Segmentation fault.
0x0804d5df in js::gc::ArenaHeader::getAllocKind (this=0xf742d000) at ../../gc/Heap.h:472
472             JS_ASSERT(allocated());
(gdb) bt
#0  0x0804d5df in js::gc::ArenaHeader::getAllocKind (this=0xf742d000) at ../../gc/Heap.h:472
#1  0x08071615 in js::gc::Cell::getAllocKind (this=0xf742d010) at ../gc/Heap.h:960
#2  0x08116d6f in js::gc::GetGCThingTraceKind (thing=0xf742d010) at ../jsgcinlines.h:59
#3  0x083ab6e2 in js::gc::MarkKind (trc=0xffffb328, thingp=0xffffb21c, kind=JSTRACE_OBJECT) at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:353
#4  0x083abd38 in MarkValueInternal (trc=0xffffb328, v=0xf7434028) at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:473
#5  0x083ac525 in js::gc::MarkObjectSlots (trc=0xffffb328, obj=(JSObject *) 0xf7434010 [object Object], start=0, nslots=2) at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:620
#6  0x082e24ae in js::ObjectImpl::markChildren (this=0xf7434010, trc=0xffffb328) at /srv/repos/mozilla-central/js/src/vm/ObjectImpl.cpp:308
#7  0x083ad679 in js::gc::MarkChildren (trc=0xffffb328, obj=(JSObject *) 0xf7434010 [object Object]) at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:943
#8  0x083aefe6 in js::TraceChildren (trc=0xffffb328, thing=0xf7434010, kind=JSTRACE_OBJECT) at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:1454
#9  0x08090364 in JS_TraceChildren (trc=0xffffb328, thing=0xf7434010, kind=JSTRACE_OBJECT) at /srv/repos/mozilla-central/js/src/jsapi.cpp:2442
#10 0x0812e4c7 in CheckForCompartmentMismatches (rt=0x891fd30) at /srv/repos/mozilla-central/js/src/jsgc.cpp:2653
#11 0x0812e54a in BeginMarkPhase (rt=0x891fd30) at /srv/repos/mozilla-central/js/src/jsgc.cpp:2666
#12 0x08132d01 in IncrementalCollectSlice (rt=0x891fd30, budget=-2, reason=JS::gcreason::DEBUG_GC, gckind=js::GC_NORMAL) at /srv/repos/mozilla-central/js/src/jsgc.cpp:4152
#13 0x08133455 in GCCycle (rt=0x891fd30, incremental=true, budget=-2, gckind=js::GC_NORMAL, reason=JS::gcreason::DEBUG_GC) at /srv/repos/mozilla-central/js/src/jsgc.cpp:4339
#14 0x08133906 in Collect (rt=0x891fd30, incremental=true, budget=-2, gckind=js::GC_NORMAL, reason=JS::gcreason::DEBUG_GC) at /srv/repos/mozilla-central/js/src/jsgc.cpp:4464
#15 0x08134026 in js::gc::RunDebugGC (cx=0x8946e18) at /srv/repos/mozilla-central/js/src/jsgc.cpp:4638
#16 0x080c430f in js::gc::NewGCThing<JSObject, (js::AllowGC)1> (cx=0x8946e18, kind=js::gc::FINALIZE_OBJECT4_BACKGROUND, thingSize=48) at ../jsgcinlines.h:501
#17 0x0858cdb4 in js::ion::NewGCThing (cx=0x8946e18, allocKind=js::gc::FINALIZE_OBJECT4_BACKGROUND, thingSize=48) at /srv/repos/mozilla-central/js/src/ion/VMFunctions.cpp:119
#18 0xf7fcad37 in ?? ()


Opt-crash trace:

==29044== Invalid read of size 4
==29044==    at 0x82673C0: js::GCMarker::drainMarkStack(js::SliceBudget&) (Heap.h:671)
==29044==    by 0x80CECF4: IncrementalCollectSlice(JSRuntime*, long long, JS::gcreason::Reason, js::JSGCInvocationKind) (jsgc.cpp:3717)
==29044==    by 0x80D0BB4: GCCycle(JSRuntime*, bool, long long, js::JSGCInvocationKind, JS::gcreason::Reason) (jsgc.cpp:4339)
==29044==    by 0x80D0FB9: _ZL7CollectP9JSRuntimebxN2js18JSGCInvocationKindEN2JS8gcreason6ReasonE.part.284 (jsgc.cpp:4464)
==29044==    by 0x80D1C31: js::gc::RunDebugGC(JSContext*) (jsgc.cpp:4383)
==29044==    by 0x8327E49: js::ion::IonCode::New(JSContext*, unsigned char*, unsigned int, JSC::ExecutablePool*) (jsgcinlines.h:501)
==29044==    by 0x835C8CB: _ZN2js3ion6Linker7newCodeEP9JSContextPNS0_14IonCompartmentE.constprop.408 (IonLinker.h:52)
==29044==    by 0x835D03D: js::ion::IonCacheGetProperty::attachReadSlot(JSContext*, js::ion::IonScript*, JSObject*, JSObject*, JS::Handle<js::Shape*>) (IonLinker.h:70)
==29044==    by 0x8363DAE: js::ion::GetPropertyCache(JSContext*, unsigned int, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>) (IonCaches.cpp:925)
==29044==    by 0x5549E1F: ???
==29044==  Address 0xfc0b0 is not stack'd, malloc'd or (recently) free'd


Marking s-s based on opt-crash and GC involved.

Comment 2

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
Due to skipped revisions, the first bad revision could be any of:
changeset:   106581:23a84dbb258f
user:        Alex Crichton
date:        Wed Jul 18 23:55:55 2012 -0700
summary:     Bug 775782 - Instrument pro/epilogue of functions for the SPS profiler in ionmonkey. r=pierron,dvander

changeset:   106582:b82fb4d04f60
parent:      106581:23a84dbb258f
parent:      100107:d78729026fb9
user:        David Anderson
date:        Mon Jul 23 12:37:49 2012 -0700
summary:     Merge from mozilla-central.

changeset:   106583:50e28df7ff8f
parent:      106582:b82fb4d04f60
parent:      100276:5598b8c4f271
user:        David Anderson
date:        Tue Jul 24 16:32:08 2012 -0700
summary:     Merge from mozilla-central.

changeset:   106584:7f0f1fdfa5e2
user:        Nicolas B. Pierron
date:        Tue Jul 24 17:24:08 2012 -0700
summary:     Bug 767349 - Simulate hidden instructions when the target is hidden. r=luke

changeset:   106585:2af804d84437
user:        Nicolas Pierron
date:        Tue Jul 24 17:24:07 2012 -0700
summary:     Bug 767349 - Track bad resume points when snapshots are encoded. r=dvander

changeset:   106586:eef915d5a18f
user:        Nicolas B. Pierron
date:        Tue Jul 24 17:48:47 2012 -0700
summary:     Bug 776748 - Do not invalidate ionScript when JM is invalidated. r=dvander

changeset:   106587:41f66d0e46b3
user:        David Anderson
date:        Wed Jul 25 02:08:41 2012 -0700
summary:     Backed out changeset eef915d5a18f

changeset:   106588:d80fbd8493f1
parent:      106587:41f66d0e46b3
parent:      100401:d03aed049b7b
user:        David Anderson
date:        Wed Jul 25 14:30:08 2012 -0700
summary:     Merge from mozilla-central.

changeset:   106589:81146d7c9f51
user:        Sean Stangl
date:        Wed Jul 25 17:10:20 2012 -0700
summary:     Bug 777570 - visitMathFunctionD() should be isCall(). r=dvander

changeset:   106590:02f44534f7f5
user:        Nicolas B. Pierron
date:        Thu Jul 26 11:17:31 2012 -0700
summary:     Bug 776748 - Do not invalidate ionScript when JM is invalidated. r=dvander

changeset:   106591:31f9c38e4cb9
parent:      106590:02f44534f7f5
parent:      100585:f528e021ceb1
user:        David Anderson
date:        Thu Jul 26 18:19:02 2012 -0700
summary:     Merge from mozilla-central.

changeset:   106592:1274d6819bae
user:        Jan de Mooij
date:        Fri Jul 27 13:08:24 2012 -0700
summary:     Fix hasLazyType assertion (bug 777647, r=dvander).

changeset:   106593:ee40f69169e9
user:        Jan de Mooij
date:        Fri Jul 27 13:12:30 2012 -0700
summary:     Don't go through GetPcScript to monitor AddValue edge cases (bug 776022, r=dvander).

changeset:   106594:ba811ef4de1c
user:        David Anderson
date:        Fri Jul 27 14:57:07 2012 -0700
summary:     Fix typo in recent merge.

changeset:   106595:a21e8bf3531f
user:        David Anderson
date:        Fri Jul 27 16:13:02 2012 -0700
summary:     Include loop entry types when determining OSR types (bug 774644, r=jandem).

changeset:   106596:a9addbf7e526
user:        Jan de Mooij
date:        Tue Jul 24 16:39:17 2012 +0200
summary:     [mq]: heur

changeset:   106597:db83474903a5
user:        David Anderson
date:        Fri Jul 27 17:16:35 2012 -0700
summary:     Backed out changeset a9addbf7e526

changeset:   106598:ae339e63d268
user:        David Anderson
date:        Fri Jul 27 17:17:26 2012 -0700
summary:     Backout due to orange.

changeset:   106599:83c83b185199
user:        Jan de Mooij
date:        Tue Jul 24 16:39:17 2012 +0200
summary:     Implement JSOP_MOD for doubles (bug 716694, r=dvander).

changeset:   106600:7a13838698ed
user:        Shu-yu Guo
date:        Sun Jul 29 11:52:45 2012 -0700
summary:     Refactor |Compile| to be templated and not use fp (bug 773339, r=dvander).

changeset:   106601:75f02a17f7cd
user:        Jan de Mooij
date:        Mon Jul 30 20:37:14 2012 +0200
summary:     Bug 776880 - Fix dropArguments call in CallConstructor to include |this|. r=dvander

changeset:   106602:54f9ee5403f0
user:        Jan de Mooij
date:        Mon Jul 30 20:43:44 2012 +0200
summary:     No bug - Add Compile to js::ion namespace to fix Clang build. r=dvander

changeset:   106603:08187a7ea897
parent:      106602:54f9ee5403f0
parent:      100846:4ca1d7d1d2da
user:        David Anderson
date:        Mon Jul 30 13:15:39 2012 -0700
summary:     Merge from mozilla-central.

This iteration took 83.998 seconds to run.

Comment 3

[Christian Holler (:decoder)]

Ccing IonMonkey/GC devs to figure out what's going on here :)

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

This was not a failure I would have expected, so CC'ing the baseline compiler folks in case anything similar might affect them.

 (1) Build a SetProp IC, needsBarrier() is false, so no write barriers are generated.
 (2) -> Call Linker::newCode(cx) for the IC. GC turns needsBarrier=true.
 (3)   -> Attached ICs are purged.
 (4) Unattached IC from (1) is then attached, violating GC invariants.

So we have to watch out for cases where allocating a code object changes barrier state. It looks like sstangl had to fix this in CodeGenerator already.

Brian suggests just suppressing GC when allocating the IC code object and that seems reasonable, patch soon.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

For anyone debugging horrible GC stuff in the future, this was sort of the workflow:

 (1) Find the address that is faulting. In this case obj1 in |obj2.obj1| was finalized, but still accessible.
 (2) Log when |obj1| gets allocated or finalized in jsgc.
 (3) Log when MarkRuntime() gets called. 
 (*) obj1 was getting finalized after a MarkRuntime() for incremental GC.
     -> guessing missing write barrier
 (4) Turn off GC validator which confused debugging
 (5) Add hw watchpoint for &obj2.obj1
 (*) obj2.obj1 is set in C++, and write barrier is triggered.
     -> likely the missing barrier is elsewhere and kills a whole graph of objects
 (6) Use find/w <heap>, obj and add write barriers all over the place
 (7) Repeat (6) for each containing object until something useful comes up
 (*) land on JIT code that is clearly missing a barrier
     -> hw watchpoint on JIT code to see when it's created

Comment 7

[Brian Hackett [Laid off!]]

Comment on attachment 710471 [details] [diff] [review]
fix

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

I think it would be simpler if the AutoSuppressGC was in newCode() and newIC() was removed.  When we allocate the IonCode for a finished script compilation we are in an AutoEnterAnalysis, and as of bug 772820 (landed for Fx 21) AutoEnterAnalysis will also suppress GC.  So there is no difference between newIC and newCode, at least on Nightly.

Comment 8

[Alex Keybl [:akeybl]]

wontfixing for FF19 given where we are in the cycle, let's make sure to get sec-approval here before landing on m-c.

Comment 9

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 84cb7aa96a52).

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also status-b2g18-v1.0.1: affected

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix v2

Patch w/ suggestion

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Lukas Blakk [:lsblakk] from comment #10)
> Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of
> v1.0.1 are now also status-b2g18-v1.0.1: affected

If IonMonkey is still disabled on b2g18, it should be unaffected.

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 715709 [details] [diff] [review]
fix v2

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

Which older supported branches are affected by this flaw?

All.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

The oldest branch might not have AutoSuppressGC, if that's the case we'll need an alternate fix. I haven't given that much thought yet but there are low-risk solutions.

How likely is this patch to cause regressions; how much testing does it need?

Very unlikely, just perf testing from awfy.

Comment 14

[David Anderson [:dvander] - inactive, e-mail if emergency]

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

Comment 15

[David Anderson [:dvander] - inactive, e-mail if emergency]

backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/b831500ca4be

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

err, https://hg.mozilla.org/integration/mozilla-inbound/rev/6c126d076b0d

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

David: do you have a new patch to try out here?

Comment 18

[David Anderson [:dvander] - inactive, e-mail if emergency]

Whoops, sorry, forgot about this. It bounced for build bustage, will land a proper patch shortly.

Comment 19

[David Anderson [:dvander] - inactive, e-mail if emergency]

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

Comment 20

[Ed Morley [:emorley]]

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

Comment 21

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 22

[Alex Keybl [:akeybl]]

Please prepare patches for all branches, including the ESR, now that this has stuck on m-c and is verified. Thanks!

Comment 23

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 715709 [details] [diff] [review]
fix v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 24

[David Anderson [:dvander] - inactive, e-mail if emergency]

Whoops, missed the questionnaire

Bug caused by: IonMonkey
User impact if declined: Possible sg:crit
Testing completed: Yes
Risk to taking this patch: Very little
String or UUID changes made by this patch: None

Comment 25

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 715709 [details] [diff] [review]
fix v2

Actually IonMonkey is 18 so this doesn't need ESR17 landing.

Comment 26

[bhavana bajaj [:bajaj]]

Comment on attachment 715709 [details] [diff] [review]
fix v2

Please make sure to land before tomorrow(3/5) to make this into FX20 beta3 going to build tomorrow.

Comment 27

[David Anderson [:dvander] - inactive, e-mail if emergency]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c0633e54e6ff

Comment 28

[David Anderson [:dvander] - inactive, e-mail if emergency]

https://hg.mozilla.org/releases/mozilla-beta/rev/f2321dba96fd

Comment 29

[Ioana (away)]

Verified as fixed on Firefox 20 beta (03/06 mozilla-beta repo).

Comment 30

[Alex Keybl [:akeybl]]

(In reply to David Anderson [:dvander] from comment #25)
> Comment on attachment 715709 [details] [diff] [review]
> fix v2
> 
> Actually IonMonkey is 18 so this doesn't need ESR17 landing.

Given that, marking ESR17's status accordingly.