924706 - Make sure JSGC_BEGIN callback runs again if we reset an incremental GC

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: reset-fix

Andrew wants to use the JSGC_BEGIN callback to purge some data stored weakly in a cache. We do this in a few places in the JS engine. The problem is that the following can happen:

begin incremental GC, clearing the cache
store X something in the cache
reset the GC
sweep X
try to access X via the cache

I think it makes the most sense to re-run the JSGC_BEGIN callback when we reset. Here's a patch to do that, although it's untested. I've pushed it to try here:

https://tbpl.mozilla.org/?tree=Try&rev=32ca202303fb

Andrew, would you mind testing this to see if it makes your problem go away? I'll ask jonco to review if everything seems to work.

Comment 1

[Andrew McCreight [:mccr8]]

It has been running bc for 26 minutes without a crash, and without those other changes we made, so it looks like it works, thanks!

Comment 2

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

The try results don't look too good. I'll take a look today or tomorrow. I might have messed up the scheduling of the next GC.

Comment 3

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

https://tbpl.mozilla.org/?tree=Try&rev=c5b440e3b259

Comment 4

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

Attachment: reset-fix v2

Try looks good.

Comment 5

[Andrew McCreight [:mccr8]]

Hooray, thanks Bill!

Comment 6

[Jon Coppeard (:jonco)]

Comment on attachment 821330 [details] [diff] [review]
reset-fix v2

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

Great, not calling the begin callback when we restart the GC sounds like a really bad idea.

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ba57b6051db

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9ba57b6051db