Remove nsIJSContextStack

RESOLVED FIXED in mozilla23

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla23
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments)

20.44 KB, patch
Gabor Krizsanits (INACTIVE)
: review+
Details | Diff | Splinter Review
14.06 KB, patch
Gabor Krizsanits (INACTIVE)
: review+
Details | Diff | Splinter Review
7.70 KB, patch
Gabor Krizsanits (INACTIVE)
: review+
Details | Diff | Splinter Review
4.26 KB, patch
Gabor Krizsanits (INACTIVE)
: review+
Details | Diff | Splinter Review
2.17 KB, patch
Gabor Krizsanits (INACTIVE)
: review+
Details | Diff | Splinter Review
12.83 KB, patch
Gabor Krizsanits (INACTIVE)
: review+
Details | Diff | Splinter Review
4.44 KB, patch
Gabor Krizsanits (INACTIVE)
: review+
Details | Diff | Splinter Review
3.36 KB, patch
Gabor Krizsanits (INACTIVE)
: review+
Details | Diff | Splinter Review
13.98 KB, patch
Gabor Krizsanits (INACTIVE)
: review+
Details | Diff | Splinter Review
13.24 KB, patch
Gabor Krizsanits (INACTIVE)
: review+
Details | Diff | Splinter Review
12.61 KB, patch
Gabor Krizsanits (INACTIVE)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Bug 860438 did a lot of the heavy lifting, but the next step is to make munging of the cx stack entirely private to nsCxPusher, and move the XPConnect-exposed versions of Get{Current,Safe}JSContext onto nsIXPConnect.
(Assignee)

Comment 2

5 years ago
This was green except for a few JSD failures, which I've fixed. Uploading patches and flagging gabor for review.
(Assignee)

Comment 3

5 years ago
Created attachment 742584 [details] [diff] [review]
Part 1 - Remove nsIJSContextStack from content/foo. v1

We leave it in nsContentUtils.cpp, which we'll handle in a separate patch.
Attachment #742584 - Flags: review?(gkrizsanits)
(Assignee)

Comment 4

5 years ago
Created attachment 742585 [details] [diff] [review]
Part 2 - Remove nsIJSContextStack from dom/foo. v1
Attachment #742585 - Flags: review?(gkrizsanits)
(Assignee)

Comment 5

5 years ago
Created attachment 742586 [details] [diff] [review]
Part 3 - Remove nsIJSContextStack from xpconnect/loader. v1
Attachment #742586 - Flags: review?(gkrizsanits)
(Assignee)

Comment 6

5 years ago
Created attachment 742587 [details] [diff] [review]
Part 4 - Remove nsIJSContextStack from other miscellaneous parts of gecko. v1
Attachment #742587 - Flags: review?(gkrizsanits)
(Assignee)

Comment 7

5 years ago
Created attachment 742588 [details] [diff] [review]
Part 5 - Make nsIXPConnect inherit nsIThreadJSContextStack. v1

Right now, the concrete nsXPConnect implements nsIXPConnect and
nsIThreadJSContextStack separately. I want to migrate the API from the latter
interface to the former, but I can't right now because it means we'd end up
with a duplicated method (getSafeJSContext). Since there's only one concrete
class that implements nsXPConnect, let's just use interface inheritance, which
simplifies the migration.
Attachment #742588 - Flags: review?(gkrizsanits)
(Assignee)

Comment 8

5 years ago
Created attachment 742589 [details] [diff] [review]
Part 6 - Make nsContentUtils munge the cx stack with sXPConnect directly. v1
Attachment #742589 - Flags: review?(gkrizsanits)
(Assignee)

Comment 9

5 years ago
Created attachment 742590 [details] [diff] [review]
Part 7 - Use sXPConnect directly in caps. v1

It's tempting to go through nsContentUtils here, but I'm pretty sure caps is
initialized before nsContentUtils.
Attachment #742590 - Flags: review?(gkrizsanits)
(Assignee)

Comment 10

5 years ago
Created attachment 742593 [details] [diff] [review]
Part 8 - Use XPConnect directly in C++ unit tests. v1
Attachment #742593 - Flags: review?(gkrizsanits)
(Assignee)

Comment 11

5 years ago
Created attachment 742594 [details] [diff] [review]
Part 9 - Remove Push/Pop from public API. v1
Attachment #742594 - Flags: review?(gkrizsanits)
(Assignee)

Comment 12

5 years ago
Created attachment 742595 [details] [diff] [review]
Part 10 - Remove Context stack iterators. v1
Attachment #742595 - Flags: review?(gkrizsanits)
(Assignee)

Comment 13

5 years ago
Created attachment 742597 [details] [diff] [review]
Part 11 - Remove nsIJSContextStack. v1

\o/
Attachment #742597 - Flags: review?(gkrizsanits)
Comment on attachment 742594 [details] [diff] [review]
Part 9 - Remove Push/Pop from public API. v1

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

::: js/xpconnect/src/nsXPConnect.cpp
@@ +2077,5 @@
> +NS_EXPORT_(bool)
> +PushJSContext(JSContext *aCx)
> +{
> +  // JSD mumbo jumbo.
> +  nsXPConnect *xpc = nsXPConnect::GetXPConnect();

Weird indentation
Comment on attachment 742585 [details] [diff] [review]
Part 2 - Remove nsIJSContextStack from dom/foo. v1

>--- a/dom/base/Navigator.cpp
>+++ b/dom/base/Navigator.cpp
>@@ -887,21 +886,17 @@ Navigator::MozIsLocallyAvailable(const nsAString &aURI,
>   // Same origin check.
>-  nsCOMPtr<nsIJSContextStack> stack = do_GetService(sJSStackContractID);
>-  NS_ENSURE_TRUE(stack, NS_ERROR_FAILURE);
>-
>-  JSContext* cx = nullptr;
>-  rv = stack->Peek(&cx);
>+  JSContext *cx = nsContentUtils::GetCurrentJSContext();

Is sJSStackContractID still used anywhere?
Attachment #742584 - Flags: review?(gkrizsanits) → review+
Attachment #742585 - Flags: review?(gkrizsanits) → review+
Comment on attachment 742586 [details] [diff] [review]
Part 3 - Remove nsIJSContextStack from xpconnect/loader. v1

-class JSCLContextHelper
+class JSCLContextHelper MOZ_STACK_CLASS

class MOZ_STACK_CLASS JSCLContextHelper
Attachment #742586 - Flags: review?(gkrizsanits) → review+
Attachment #742587 - Flags: review?(gkrizsanits) → review+
Attachment #742588 - Flags: review?(gkrizsanits) → review+
Comment on attachment 742589 [details] [diff] [review]
Part 6 - Make nsContentUtils munge the cx stack with sXPConnect directly. v1

-  nsresult rv = iterator->Reset(aStack);
+  nsresult rv = iterator->Reset(nsContentUtils::XPConnect());

Is there any point of this argument here? I mean, can't we just change this to Reset() by any chance?
Attachment #742589 - Flags: review?(gkrizsanits) → review+
Attachment #742590 - Flags: review?(gkrizsanits) → review+
Attachment #742593 - Flags: review?(gkrizsanits) → review+
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #17)
> Is there any point of this argument here? I mean, can't we just change this
> to Reset() by any chance?

Since it's removed entirely later, never-mind.
(In reply to Bobby Holley (:bholley) from comment #11)
> Created attachment 742594 [details] [diff] [review]
> Part 9 - Remove Push/Pop from public API. v1

-        nsCOMPtr<nsIJSContextStack> cxstack = do_GetService("@mozilla.org/js/xpc/ContextStack;1");
-        if (!cxstack) {
-            printf("failed to get the nsThreadJSContextStack service!\n");
-            return 1;
-        }
-
-        if (NS_FAILED(cxstack->Push(cx))) {
-            printf("failed to push the current JSContext on the nsThreadJSContextStack!\n");
+        if (!xpc::danger::PushJSContext(cx)) {

Why don't we use a CxPusher here? If that would require to move it to xpcpublic, well... it kind of belong to there anyway... Or is there something I'm overlooking here?
Comment on attachment 742595 [details] [diff] [review]
Part 10 - Remove Context stack iterators. v1

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

Just one question, have you checked if we have any crazy add-on using this stack iterator for something?
Attachment #742595 - Flags: review?(gkrizsanits) → review+
Comment on attachment 742597 [details] [diff] [review]
Part 11 - Remove nsIJSContextStack. v1

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

\o/
Attachment #742597 - Flags: review?(gkrizsanits) → review+
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #20)
> Just one question, have you checked if we have any crazy add-on using this
> stack iterator for something?

I haven't find any, so never-mind...
Comment on attachment 742594 [details] [diff] [review]
Part 9 - Remove Push/Pop from public API. v1

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

My earlier question still stands, but regardless of your answer, I don't want to hold you up with landing because of this little thing.
Attachment #742594 - Flags: review?(gkrizsanits) → review+
(Assignee)

Comment 24

5 years ago
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #16) 
> Why don't we use a CxPusher here? If that would require to move it to
> xpcpublic, well... it kind of belong to there anyway... Or is there
> something I'm overlooking here?

It's mostly related to linkage. Currently xpcshell can't include nsContentUtils.h because it pulls in a bunch of stuff that's not available to stuff with external linkage (string APIs and the like). It's true that we could move nsCxPusher to xpcpublic, but that would involve making sure that all of the symbols were properly exported, which could end up snowballing depending on what kind of stuff nsCxPusher ends up pulling in. It would certainly be cleaner, but given that we're currently working full-throttle on removing cx pushing entirely, I felt like we could live with one external call from xpcshell for the time being.
(In reply to Bobby Holley (:bholley) from comment #24)
> certainly be cleaner, but given that we're currently working full-throttle
> on removing cx pushing entirely, I felt like we could live with one external
> call from xpcshell for the time being.

Alright, I'm fine with it.

Comment 28

5 years ago
Hi, I'm upgrading the GWT Firefox plugin to work with Firefox 23. It was using nsIJSContextStack [1], so that needs to be changed, but I don't understand what the new way to do it is. Any guidance?

(I didn't write our plugin and I'm not at all familiar with the Firefox code base; normally I just build new releases.)

[1] see getWindowObject at line 93 of https://gwt.googlesource.com/gwt-plugins/+/refs/heads/master/xpcom/ExternalWrapper.cpp
(Assignee)

Comment 29

5 years ago
Hi Brian,

There are a number of ways to accomplish what this code is currently doing (though most of them require internal linkage, which might be out of the question for you). However, I'm kind of concerned that this function might be predicated on concepts that are (or will soon be) meaningless/underdefined in modern Gecko architecture.

If you just want to get it compiling and move on, I might be able to offer some suggestions. But it probably makes more sense to have me talk to someone who understands the plugin architecture and figure out a solution that will be stable going forward.

Feel free to ping me (bholley) on irc.mozilla.org ( #developers ) or email me.
You need to log in before you can comment on or make changes to this bug.