813683 - IonMonkey: Get rid of the WeakCache used by VMWrapper.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

The WeakCache was introduced to keep the VMWrapper alive as long as the scripts which were calling them were alive.  Now that Bug 786146 has moved them to the Runtime and keep them alive all the time, in IonRuntime::Mark, we no longer need the WeakCache, and a simple Map should do the work.

Comment 1

[Terrence Cole [:terrence]]

Attachment: no_need_for_rekeyable_on_vmfunction-v0.diff

VMFunction is a C++ allocation and cannot move, so there is no need for this to be a RekeyableHashTable. Moreover, it is only valid to have a bare GC pointer (the JitCode*) if it is always rooted. It was not clear to me that this was or was not the case, so I made it a HeapPtr to be on the safe side. If the table is always rooted then the overhead will be very low and if it's not, then it might save us from some long-tail UAFs.

Comment 2

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7439d46c299

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8793048 [details] [diff] [review]
no_need_for_rekeyable_on_vmfunction-v0.diff

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

This patch sounds good to me, any idea what causes the red on try?

Comment 4

[Steve Fink [:sfink] [:s:]]

(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> This patch sounds good to me, any idea what causes the red on try?

It's the other assertion patch in that push, from bug 1291776.

Comment 5

[Terrence Cole [:terrence]]

(In reply to Steve Fink [:sfink] [:s:] (PTO Sep23-28) from comment #4)
> (In reply to Nicolas B. Pierron [:nbp] from comment #3)
> > This patch sounds good to me, any idea what causes the red on try?
> 
> It's the other assertion patch in that push, from bug 1291776.

Actually, no, this is bug 1294563 comment 2. The failures are real. The followup try push appears not to have been posted here for some reason?

Comment 6

[Terrence Cole [:terrence]]

Attachment: no_need_for_rekeyable_on_vmfunction-v1.diff

This is the right patch. We're fine just stripping the GC bits from it since the whole thing has static lifetime.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19ae43f8f7ca
No need for Rekeyable Hashtable for C++ VMFunction* pointers; r=nbp

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/19ae43f8f7ca