714562 - Intermittent Assertion failure: info.prevp, at /builds/slave/m-in-osx64-dbg/build/js/src/jsgc.cpp:779 or crash [@ js::gc::Chunk::releaseArena]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Phil Ringnalda (:philor)]

Yet another thing I've been hoping was also other things, like bug 714545. This is from the push after the fix for it landed, but there have been a few before then, too.

https://tbpl.mozilla.org/php/getParsedLog.php?id=8269807&tree=Mozilla-Inbound
Rev4 MacOSX Snow Leopard 10.6 mozilla-inbound debug test mochitest-other on 2012-01-01 15:57:29 PST for push b76de21ba602

TEST-INFO | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug557956.js | Console message: LOG addons.xpi: Install of http://example.com/browser/toolkit/mozapps/extensions/test/browser/browser_bug557956_8_2.xpi completed.
Assertion failure: info.prevp, at /builds/slave/m-in-osx64-dbg/build/js/src/jsgc.cpp:779
WARNING: shutting down early because of crash!: file /builds/slave/m-in-osx64-dbg/build/dom/plugins/ipc/PluginModuleChild.cpp, line 746
WARNING: plugin process _exit()ing: file /builds/slave/m-in-osx64-dbg/build/dom/plugins/ipc/PluginModuleChild.cpp, line 711
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug557956.js | Exited with code 1 during test run
INFO | automation.py | Application ran for: 0:26:47.399499
INFO | automation.py | Reading PID log: /var/folders/Hs/HsDn6a9SG8idoIya6p9mtE+++TI/-Tmp-/tmp3mzFPvpidlog
PROCESS-CRASH | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug557956.js | application crashed (minidump found)
Crash dump filename: /var/folders/Hs/HsDn6a9SG8idoIya6p9mtE+++TI/-Tmp-/tmpc_U7Dw/minidumps/F328A31B-C076-41E9-8AD9-017419A487BE.dmp
Operating system: Mac OS X
                  10.6.8 10K549
CPU: amd64
     family 6 model 23 stepping 10
     2 CPUs

Crash reason:  EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
Crash address: 0x0

Thread 0 (crashed)
 0  XUL!CrashInJS [jsutil.cpp:b76de21ba602 : 95 + 0x0]
    rbx = 0x261a4000   r12 = 0x049f1400   r13 = 0x051c3508   r14 = 0x006660d0
    r15 = 0x049f1610   rip = 0x0101ae04   rsp = 0x5fbfae40   rbp = 0x5fbfae40
    Found by: given as instruction pointer in context
 1  XUL!js::gc::Chunk::releaseArena [jsgc.cpp:b76de21ba602 : 779 + 0x17]
    rbx = 0x261a4000   r12 = 0x049f1400   r13 = 0x051c3508   r14 = 0x006660d0
    r15 = 0x049f1610   rip = 0x0309e3fd   rsp = 0x5fbfae50   rbp = 0x5fbfaed0
    Found by: call frame info
 2  XUL!js::gc::FinalizeArenas [jsgc.cpp:b76de21ba602 : 362 + 0xe]
    rbx = 0x261a4000   r12 = 0x480f2008   r13 = 0x0000000c   r14 = 0x006660d0
    r15 = 0x049f1610   rip = 0x030a0152   rsp = 0x5fbfaee0   rbp = 0x5fbfaf50
    Found by: call frame info
 3  XUL!SweepPhase [jsgc.cpp:b76de21ba602 : 1516 + 0x16]
    rbx = 0x04caae10   r12 = 0x049f1400   r13 = 0x04caaed0   r14 = 0x04caaed0
    r15 = 0x006660d0   rip = 0x030adffc   rsp = 0x5fbfaf60   rbp = 0x5fbfafc0
    Found by: call frame info
 4  XUL!GCCycle [jsgc.cpp:b76de21ba602 : 2829 + 0x18]
    rbx = 0x022f1f40   r12 = 0x3d456400   r13 = 0x05183000   r14 = 0x00000000
    r15 = 0x00000000   rip = 0x030af152   rsp = 0x5fbfafd0   rbp = 0x5fbfb170
    Found by: call frame info
 5  XUL!js_GC [jsgc.cpp:b76de21ba602 : 3125 + 0xe]
    rbx = 0x05183000   r12 = 0x006660d0   r13 = 0x5fbfb1a0   r14 = 0x00000000
    r15 = 0x1601fd01   rip = 0x030afa0f   rsp = 0x5fbfb180   rbp = 0x5fbfb1f0
    Found by: call frame info
 6  XUL!nsXPConnect::Collect [nsXPConnect.cpp:b76de21ba602 : 412 + 0x7]
    rbx = 0x05d7d000   r12 = 0x006660d0   r13 = 0x5fbfb220   r14 = 0x00000000
    r15 = 0x1601fd53   rip = 0x022bce51   rsp = 0x5fbfb200   rbp = 0x5fbfb320
    Found by: call frame info
 7  XUL!nsXPConnect::GarbageCollect [nsXPConnect.cpp:b76de21ba602 : 420 + 0xc]
    rbx = 0x0085c200   r12 = 0x00000000   r13 = 0x01c62530   r14 = 0x00000000
    r15 = 0x1601fd53   rip = 0x022b8c51   rsp = 0x5fbfb330   rbp = 0x5fbfb330
    Found by: call frame info
 8  XUL!nsJSContext::GarbageCollectNow [nsJSEnvironment.cpp:b76de21ba602 : 3243 + 0xc]
    rbx = 0x0085c200   r12 = 0x00000000   r13 = 0x01c62530   r14 = 0x00000000
    r15 = 0x1601fd53   rip = 0x01c6246d   rsp = 0x5fbfb340   rbp = 0x5fbfb350
    Found by: call frame info
 9  XUL!nsTimerImpl::Fire [nsTimerImpl.cpp:b76de21ba602 : 428 + 0xa]
    rbx = 0x00000002   r12 = 0x4ba76ba0   r13 = 0x01c62530   r14 = 0x00000000
    r15 = 0x1601fd53   rip = 0x02c6769b   rsp = 0x5fbfb360   rbp = 0x5fbfb3e0

Comment 1

[Igor Bukanov]

This is yet another regression from the bug 702251 that is not covered by the fix in the bug 714344. I have missed that during the decommit outside the GC lock the allocation thread may have allocated all the arenas from the chunk and removed it from the available list. In this case the chunk should be added back to the available list.

Comment 2

[Phil Ringnalda (:philor)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=8270832&tree=Mozilla-Inbound

and https://tbpl.mozilla.org/php/getParsedLog.php?id=8270118&tree=Mozilla-Inbound for an opt crash with what I think is the same stack as the assertion.

Comment 3

[Igor Bukanov]

Attachment: v1

This is what I have sent to the try server - the patch effectively replaces that Maybe<AutoUnlock> fix from the 714344 with a code to properly remove/add chunk from the available list. It does not look too complex/fragile as I initially thought it would be in the bug 714344.

Comment 4

[Igor Bukanov]

Attachment: v2

In the new version I made comments clear.

Comment 5

[Phil Ringnalda (:philor)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=8276499&tree=Mozilla-Inbound

Comment 6

[Phil Ringnalda (:philor)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=8275286&tree=Mozilla-Inbound

Comment 7

[Phil Ringnalda (:philor)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=8286935&tree=Mozilla-Inbound

Comment 8

[Phil Ringnalda (:philor)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=8289144&tree=Firefox

Comment 9

[Phil Ringnalda (:philor)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=8294571&tree=Mozilla-Inbound

Comment 10

[Phil Ringnalda (:philor)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=8298833&tree=Build-System

Comment 11

[Igor Bukanov]

Attachment: v3

The new version skips unlocking during the decommit if the main thread already waits in GC for the background thread to finish. This helps to avoids extra system calls resulting from unlock/lock calls on a contested lock and lets the main thread to start the GC faster.

Comment 12

[Phil Ringnalda (:philor)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=8322117&tree=Mozilla-Inbound

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 585685 [details] [diff] [review]
v3

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

This seems correct. Eventually it would be nice if we could refactor the code so that we're not duplicating the logic from allocateArena and releaseArena here.

This list manipulation is getting pretty gross. Do you think it would be easier if the prev pointers pointed to chunks instead of to their next pointers? It might be worth a try.

::: js/src/jsgc.cpp
@@ +2315,5 @@
> +                        insertPoint = availableListHeadp;
> +                }
> +                chunk->insertToAvailableList(insertPoint);
> +            } else {
> +                JS_ASSERT(chunk->info.prevp);

Can you assert !chunk->unused() here? That way this code matches up more clearly with the releaseArena code.

Comment 14

[Phil Ringnalda (:philor)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=8337172&tree=Mozilla-Inbound

Comment 15

[Phil Ringnalda (:philor)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=8338759&tree=Mozilla-Inbound

Comment 16

[Igor Bukanov]

(In reply to Bill McCloskey (:billm) from comment #13)
> This list manipulation is getting pretty gross. Do you think it would be
> easier if the prev pointers pointed to chunks instead of to their next
> pointers?

With that the first chunk can not just point to a field in JSRuntime * requiring more if checks when adding or removing chunks to the list. Another complication is that just from the value of info.prevp it would be impossible to tell if the chunk is on the available list or not. That would have to be replaced with something like chunk->hasAvailableArenas() && !chunk->unused() bringing complications when updating the counters.

In any case, at some point I want to replace the chunks list with a sorted vector as that it seems allows for lesser fragmentation.

Comment 17

[Igor Bukanov]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8062086f8e00

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/8062086f8e00