929314 - Remove all uses of Handle::repoint

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

As Waldo points out, Handle::repoint is a footgun, as it allows us to create Handles that refer to things that have a *shorter* lifetime than the handle, which we couldn't otherwise:

Handle h1 = something;
{
  Rooted r(cx, blech);
  Handle h2 = r;
  h1.repoint(h2);
}
/* h1 is garbage */

There are only a few uses of Handle::repoint: CompileOptions, C1Spewer, and IonSpewer. All of those can be changed to use different things, perhaps the AutoRooter type proposed in bug 892643.

Comment 1

[Florian Bender]

Is this still valid with the depending bugs fixed?

Comment 2

[Jim Blandy :jimb]

As long as there exist uses of Handle<T>::repoint, then this bug is valid. It looks like C1Spewer and IonSpewer are the ones that still exist.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: 1 - refactor-ion-spewer

I though it would be simple to replace the handles that we call repoint() on with PersistentRooteds, which can simply be assigned to.  However I ended up having to refactor IonSpewer so that it's stored as a pointer in the JitRuntime so that it can be initialised after we're created a JSRuntime.

This has the additional advantage of not using a global to store this state, which presumable didn't work if there were multiple runtimes.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: 2 - remove-repoint

And now we can just kill Handle::repoint()

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8424894 [details] [diff] [review]
1 - refactor-ion-spewer

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

IonSpewer::function is unused, looks like. I just removed it and I can compile a debug shell; can you do that instead?

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Jon Coppeard (:jonco) from comment #3)
> This has the additional advantage of not using a global to store this state,
> which presumable didn't work if there were multiple runtimes.

Good point. The spewer is mostly used in the shell though where it's not a problem, and IIUC even with the spewer-per-runtime we'll still fopen the same file for writing from multiple threads (not sure what will happen there). As the spewer is just a debug-only thing, I think it's fine to leave it as a singleton for now until people actually run into problems with it.

Comment 7

[Terrence Cole [:terrence]]

Comment on attachment 8424896 [details] [diff] [review]
2 - remove-repoint

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

\o/ r=me

Comment 8

[Jon Coppeard (:jonco)]

Attachment: 1 - refector-ion-spewer v2

Thanks, I should have spotted that!  This is much simpler.

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8425345 [details] [diff] [review]
1 - refector-ion-spewer v2

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

Thanks!

Comment 10

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/179d30a83cb5
https://hg.mozilla.org/integration/mozilla-inbound/rev/687938abd5de

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/179d30a83cb5
https://hg.mozilla.org/mozilla-central/rev/687938abd5de