865729 - Remove nsIJSContextStack

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=df273f38055a

Comment 2

[Bobby Holley (:bholley)]

This was green except for a few JSD failures, which I've fixed. Uploading patches and flagging gabor for review.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1 - Remove nsIJSContextStack from content/foo. v1

We leave it in nsContentUtils.cpp, which we'll handle in a separate patch.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 2 - Remove nsIJSContextStack from dom/foo. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 3 - Remove nsIJSContextStack from xpconnect/loader. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 4 - Remove nsIJSContextStack from other miscellaneous parts of gecko. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: 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.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 6 - Make nsContentUtils munge the cx stack with sXPConnect directly. v1

Comment 9

[Bobby Holley (:bholley)]

Attachment: 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.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 8 - Use XPConnect directly in C++ unit tests. v1

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 9 - Remove Push/Pop from public API. v1

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 10 - Remove Context stack iterators. v1

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 11 - Remove nsIJSContextStack. v1

\o/

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 15

[Masatoshi Kimura [:emk]]

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?

Comment 16

[Gabor Krizsanits (INACTIVE)]

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

Comment 17

[Gabor Krizsanits (INACTIVE)]

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?

Comment 18

[Gabor Krizsanits (INACTIVE)]

(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.

Comment 19

[Gabor Krizsanits (INACTIVE)]

(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 20

[Gabor Krizsanits (INACTIVE)]

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?

Comment 21

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 742597 [details] [diff] [review]
Part 11 - Remove nsIJSContextStack. v1

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

\o/

Comment 22

[Gabor Krizsanits (INACTIVE)]

(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 23

[Gabor Krizsanits (INACTIVE)]

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.

Comment 24

[Bobby Holley (:bholley)]

(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.

Comment 25

[Gabor Krizsanits (INACTIVE)]

(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 26

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=a751e69be9f1

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e2f30291a3fc
https://hg.mozilla.org/mozilla-central/rev/aa4e20e89f70
https://hg.mozilla.org/mozilla-central/rev/dcf7a7bc0e1d
https://hg.mozilla.org/mozilla-central/rev/6f75f0fd2352
https://hg.mozilla.org/mozilla-central/rev/af0b982f182d
https://hg.mozilla.org/mozilla-central/rev/dab57d93467c
https://hg.mozilla.org/mozilla-central/rev/858e4b47cd82
https://hg.mozilla.org/mozilla-central/rev/a6fa01763d9d
https://hg.mozilla.org/mozilla-central/rev/7247e828e9f5
https://hg.mozilla.org/mozilla-central/rev/88dff387b118
https://hg.mozilla.org/mozilla-central/rev/a751e69be9f1

Comment 28

[Brian Slesinsky]

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

Comment 29

[Bobby Holley (:bholley)]

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.