Closed Bug 924706 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: billm, Assigned: billm)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch reset-fix (obsolete) — Splinter Review
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.
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!
Blocks: 905382
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.
Attached patch reset-fix v2Splinter Review
Try looks good.
Attachment #814666 - Attachment is obsolete: true
Attachment #821330 - Flags: review?(jcoppeard)
Hooray, thanks Bill!
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.
Attachment #821330 - Flags: review?(jcoppeard) → review+
https://hg.mozilla.org/mozilla-central/rev/9ba57b6051db
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.