886459 - Remove nsIJSRuntimeService

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andrew McCreight [:mccr8]]

This is a weird little interface that could probably be replaced in many or all places with something more direct.

Comment 1

[Bobby Holley (:bholley)]

This thing is never used from script AFAICT. Let's just merge it into nsIXPConnect?

Comment 2

[Andrew McCreight [:mccr8]]

The primary use of this service is to get a JS runtime from outside of XPConnect. To address this, I add a new static method in xpcpublic.h that just returns the current JSRuntime.  nsJSNPRuntime also registers a GC callback, so I added a similar method to register and unregister that.  The context callbacks are never used, so I remove those entirely. Finally, there are a bunch of unused includes that can be removed.

Hopefully this won't break anything:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=574902839ba9

Comment 3

[Andrew McCreight [:mccr8]]

The most worrisome parts of this are the changes to nsJSEnviroment.cpp and nsJSNPRuntime.cpp because they remove some strong references to XPConnect, so in theory we could start crashing. The changes to the XPCShell-ish stuff might also be bad because I don't know how exactly that all works. But I guess if we previously had services and XPConnect going by the point we called things, it should be okay. It is possible there are additional null checks that will be needed.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Remove unused includes of nsIJSRuntimeService.h.

No particular hurry on reviewing these.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Remove context callbacks from XPCJSRuntime.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Remove simple uses of nsIJSRuntimeService to get the JSRuntime.

This patch also removes a number of places that get a runtime but don't actually use it.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: part 4 - Remove nsIJSRuntimeService.

I'll put this up for review once the other parts are okayed. I'll get a plugin reviewer for the nsJSNPRuntime changes.

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8625745 [details] [diff] [review]
part 3 - Remove simple uses of nsIJSRuntimeService to get the JSRuntime.

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

::: toolkit/xre/nsXREDirProvider.cpp
@@ +893,5 @@
>        // resources which are about to go away in "profile-before-change" are destroyed first.
>  
> +      JSRuntime *rt = xpc::GetJSRuntime();
> +      if (rt) {
> +        ::JS_GC(rt);

Remove the :: while you're here?

Comment 9

[Andrew McCreight [:mccr8]]

Comment on attachment 8625746 [details] [diff] [review]
part 4 - Remove nsIJSRuntimeService.

r? aklotz for the nsJSNPRuntime changes. The main thing I am concerned about here is that this change makes it so that we no longer keep alive XPConnect via the sCallbackRuntime global variable. I'd hope that something else would keep it alive, but plugins are magic. (L32 debug try run was green.) The rest of the changes to that file are just less boilerplate-y ways to interact with XPConnect.

r? bholley for the rest. The main interesting thing there is adding little AddGCCallback()/RemoveGCCallback() functions for the use of plugins.

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8625746 [details] [diff] [review]
part 4 - Remove nsIJSRuntimeService.

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

The code looks LGTM. Here's to hoping that there aren't any lifetime issues.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bafedff401e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb6ee24607d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4fd3357b32b
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc99bdc69795

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bafedff401e1
https://hg.mozilla.org/mozilla-central/rev/cb6ee24607d7
https://hg.mozilla.org/mozilla-central/rev/c4fd3357b32b
https://hg.mozilla.org/mozilla-central/rev/cc99bdc69795