Remove nsIJSRuntimeService

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(4 attachments)

(Assignee)

Description

5 years ago
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)

Updated

3 years ago
Assignee: nobody → continuation
Summary: consider deCOMtaminating or eliminating nsIJSRuntimeService → Remove nsIJSRuntimeService
(Assignee)

Comment 2

3 years ago
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
(Assignee)

Comment 3

3 years ago
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.
(Assignee)

Comment 4

3 years ago
Created attachment 8625743 [details] [diff] [review]
part 1 - Remove unused includes of nsIJSRuntimeService.h.

No particular hurry on reviewing these.
Attachment #8625743 - Flags: review?(bobbyholley)
(Assignee)

Comment 5

3 years ago
Created attachment 8625744 [details] [diff] [review]
part 2 - Remove context callbacks from XPCJSRuntime.
Attachment #8625744 - Flags: review?(bobbyholley)
(Assignee)

Comment 6

3 years ago
Created attachment 8625745 [details] [diff] [review]
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.
Attachment #8625745 - Flags: review?(bobbyholley)
(Assignee)

Comment 7

3 years ago
Created attachment 8625746 [details] [diff] [review]
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.
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+
(Assignee)

Comment 9

3 years ago
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.