Closed
Bug 886459
Opened 12 years ago
Closed 10 years ago
Remove nsIJSRuntimeService
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: mccr8, Assigned: mccr8)
Details
Attachments
(4 files)
6.43 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
6.05 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
18.79 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
13.70 KB,
patch
|
bugzilla
:
review+
bholley
:
review+
|
Details | Diff | Splinter Review |
This is a weird little interface that could probably be replaced in many or all places with something more direct.
Comment 1•12 years ago
|
||
This thing is never used from script AFAICT. Let's just merge it into nsIXPConnect?
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → continuation
Summary: consider deCOMtaminating or eliminating nsIJSRuntimeService → Remove nsIJSRuntimeService
Assignee | ||
Comment 2•10 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•10 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•10 years ago
|
||
No particular hurry on reviewing these.
Attachment #8625743 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8625744 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 6•10 years ago
|
||
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•10 years ago
|
||
I'll put this up for review once the other parts are okayed. I'll get a plugin reviewer for the nsJSNPRuntime changes.
Updated•10 years ago
|
Attachment #8625743 -
Flags: review?(bobbyholley) → review+
Updated•10 years ago
|
Attachment #8625744 -
Flags: review?(bobbyholley) → review+
Comment 8•10 years ago
|
||
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•10 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)
Updated•10 years ago
|
Attachment #8625746 -
Flags: review?(bobbyholley) → review+
Comment 10•10 years ago
|
||
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+
Comment 11•10 years ago
|
||
Comment 12•10 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•