Closed Bug 886459 Opened 10 years ago Closed 8 years ago

Remove nsIJSRuntimeService


(Core :: XPConnect, defect)

Not set



Tracking Status
firefox41 --- fixed


(Reporter: mccr8, Assigned: mccr8)



(4 files)

This is a weird little interface that could probably be replaced in many or all places with something more direct.
This thing is never used from script AFAICT. Let's just merge it into nsIXPConnect?
Assignee: nobody → continuation
Summary: consider deCOMtaminating or eliminating nsIJSRuntimeService → Remove nsIJSRuntimeService
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:
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.
No particular hurry on reviewing these.
Attachment #8625743 - Flags: review?(bobbyholley)
This patch also removes a number of places that get a runtime but don't actually use it.
Attachment #8625745 - Flags: review?(bobbyholley)
I'll put this up for review once the other parts are okayed. I'll get a plugin reviewer for the nsJSNPRuntime changes.
Attachment #8625743 - Flags: review?(bobbyholley) → review+
Attachment #8625744 - Flags: review?(bobbyholley) → review+
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?
Attachment #8625745 - Flags: review?(bobbyholley) → review+
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.
Attachment #8625746 - Flags: review?(bobbyholley)
Attachment #8625746 - Flags: review?(aklotz)
Attachment #8625746 - Flags: review?(bobbyholley) → review+
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.
Attachment #8625746 - Flags: review?(aklotz) → review+
You need to log in before you can comment on or make changes to this bug.