Closed Bug 865729 Opened 11 years ago Closed 11 years ago

Remove nsIJSContextStack

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(11 files)

20.44 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
14.06 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
7.70 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
4.26 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
2.17 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
12.83 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
4.44 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
3.36 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
13.98 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
13.24 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
12.61 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
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.
This was green except for a few JSD failures, which I've fixed. Uploading patches and flagging gabor for review.
We leave it in nsContentUtils.cpp, which we'll handle in a separate patch.
Attachment #742584 - Flags: review?(gkrizsanits)
Attachment #742585 - Flags: review?(gkrizsanits)
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)
It's tempting to go through nsContentUtils here, but I'm pretty sure caps is
initialized before nsContentUtils.
Attachment #742590 - Flags: review?(gkrizsanits)
Attachment #742594 - Flags: review?(gkrizsanits)
Attachment #742595 - Flags: review?(gkrizsanits)
\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+
(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.
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
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.