Bob Owen (:bobowen) Assignee
	Description • 10 years ago

Another batch of patches moving nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI etc.
See bug 951991 for details.

	Bob Owen (:bobowen) Assignee
	Comment 1 • 10 years ago

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

	Bob Owen (:bobowen) Assignee
	Comment 2 • 10 years ago

I think all this stuff only really cares about the compartment (which it enters already) not the JSContext.
Although it does call CreateSandboxObject at some point, but this seems pretty similar to the stuff that runs when we're creating globals in nsGlobalWindow::SetNewDocument.

	Bob Owen (:bobowen) Assignee
	Comment 3 • 10 years ago

It seemed to make sense to tackle these all together as they're all related.
I think we need the error reporting, at least for the compilation.

	Bobby Holley (:bholley)
	Comment 4 • 10 years ago

Comment on attachment 8455511 [details] [diff] [review]
Part 2: Replace AutoPushJSContexts et al. in nsXBLPrototypeHandler v1

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

::: dom/xbl/nsXBLPrototypeHandler.cpp
@@ +335,5 @@
>    return NS_OK;
>  }
>  
>  nsresult
>  nsXBLPrototypeHandler::EnsureEventHandler(nsIScriptGlobalObject* aGlobal,

I was thinking that in these situations, we should start passing an AutoJSAPI& as a way of indicating that the callee inherits the caller's JSAPI state. We could then use JS::CurrentGlobalOrNull to grab the global, and GetNativeForGlobal (or WindowGlobalOrNull) to get the corresponding nsIGlobalObject when we need it.

Feel free to do that if you're in the mood, or skip it. If you do skip it though, please at least convert the nsIScriptGlobalObject param to an nsIGlobalObject, since the former is deprecated.

	Bob Owen (:bobowen) Assignee
	Comment 5 • 10 years ago

(In reply to Bobby Holley (:bholley) from comment #4)

> I was thinking that in these situations, we should start passing an
> AutoJSAPI& as a way of indicating that the callee inherits the caller's
> JSAPI state. We could then use JS::CurrentGlobalOrNull to grab the global,
> and GetNativeForGlobal (or WindowGlobalOrNull) to get the corresponding
> nsIGlobalObject when we need it.

Thought it best to r? again to make sure I've understood what you meant.

As we've started passing these around, I've also added a check to assert if anyone tries (on main thread) to grab a JSContext* from an AutoJSAPI that isn't stack top.
I can't think of any good reason why someone would want to do this and, although fairly unlikely, this should guard against someone passing the wrong AutoJSAPI.

	Bob Owen (:bobowen) Assignee
	Comment 6 • 10 years ago

Given bz's comment in nsObjectLoadingContent::SetupProtoChainRunner::Run, I'm guessing that the potential errors in JS_SetPrototype should never happen in this case, so seemed safe to go for a plain Init() for both of these. 
SetupProtoChain already enters the necessary compartment.

	Bob Owen (:bobowen) Assignee
	Comment 7 • 10 years ago

I did have another part 4 from nsDOMClassInfo BaseStubConstructor, but I realised, while I was checking before uploading, that it should probably have an AutoEntryScript, so I've skipped for the moment.

This is a bit of a strange one.
I think it is only called from nsGlobalWindow::DefineArgumentsProperty now.
This change should give the same results, I was tempted to use the SafeJSContext, but it seemed a bit of an odd thing to do from within an nsIScriptContext.

	Bob Owen (:bobowen) Assignee
	Comment 8 • 10 years ago

Hmm, looks like Linux debug doesn't like how I'm instantiating that nsCOMPtr<nsPIDOMWindow> from Part 2:
https://tbpl.mozilla.org/?tree=Try&rev=f0b4fe3fd4a3

I've built this locally on Linux64.

	Bobby Holley (:bholley)
	Comment 9 • 10 years ago

Comment on attachment 8456067 [details] [diff] [review]
Part 2: Replace AutoPushJSContexts et al. in nsXBLPrototypeHandler v2

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

r=bholley modulo those fixes

::: dom/base/ScriptSettings.h
@@ +16,5 @@
>  
>  #include "mozilla/Maybe.h"
>  
> +#ifdef DEBUG
> +#include "nsContentUtils.h"

Hm, this doesn't really solve the problem of including nsContentUtils.h in more places, because the problems with that (slower builds, and slower rebuilds after touching nsContentUtils.h) only really arise during development, which happens with debug builds.

@@ +172,5 @@
>  
>    JSContext* cx() const {
>      MOZ_ASSERT(mCx, "Must call Init before using an AutoJSAPI");
> +    MOZ_ASSERT_IF(NS_IsMainThread(),
> +                  mCx == nsContentUtils::GetCurrentJSContext());

How about dropping the #include and doing MOZ_ASSERT_IF(NS_IsMainThread(), CxPusherIsStackTop());?

::: dom/xbl/nsXBLPrototypeHandler.h
@@ +165,5 @@
>    bool ModifiersMatchMask(nsIDOMUIEvent* aEvent,
>                              bool aIgnoreShiftKey = false);
>    nsresult DispatchXBLCommand(mozilla::dom::EventTarget* aTarget, nsIDOMEvent* aEvent);
>    nsresult DispatchXULKeyCommand(nsIDOMEvent* aEvent);
> +  nsresult EnsureEventHandler(mozilla::dom::AutoJSAPI& aJsapi, nsIAtom* aName,

I think we should drop the 'a' prefixing for the jsapi cases and just pass them as 'jsapi'.

	Bobby Holley (:bholley)
	Comment 10 • 10 years ago

Comment on attachment 8456091 [details] [diff] [review]
Part 4: Replace nsCxPusher in nsJSContext::SetProperty v1

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

::: dom/base/nsJSEnvironment.cpp
@@ +937,5 @@
>  nsresult
>  nsJSContext::SetProperty(JS::Handle<JSObject*> aTarget, const char* aPropName, nsISupports* aArgs)
>  {
> +  AutoJSAPI jsapi;
> +  if (NS_WARN_IF(!jsapi.Init(GetGlobalObject(), mContext))) {

I don't think you need to pass mContext here. You should be able to just assert jsapi.cx() == mContext->GetNativeContext() after creating the jsapi with GetGlobalObject().

	Bob Owen (:bobowen) Assignee
	Comment 11 • 10 years ago

I had to chase the const on AutoJSAPI::cx through the code for a couple of functions, in order to use CxPusherIsStackTop.
r?ing to make sure you're OK with that.

(In reply to Bobby Holley (:bholley) from comment #9)

> How about dropping the #include and doing MOZ_ASSERT_IF(NS_IsMainThread(),
> CxPusherIsStackTop());?

What, you mean that really useful looking and obvious function just four lines down ... doh!

> I think we should drop the 'a' prefixing for the jsapi cases and just pass
> them as 'jsapi'.

Done.

	Bob Owen (:bobowen) Assignee
	Comment 12 • 10 years ago

r=bholley - from comment 10

(In reply to Bobby Holley (:bholley) from comment #10)

> > +  AutoJSAPI jsapi;
> > +  if (NS_WARN_IF(!jsapi.Init(GetGlobalObject(), mContext))) {
> 
> I don't think you need to pass mContext here. You should be able to just
> assert jsapi.cx() == mContext->GetNativeContext() after creating the jsapi
> with GetGlobalObject().

Changed to InitWithLegacyErrorReporting and added assertion that we get the correct context.

	Bob Owen (:bobowen) Assignee
	Comment 13 • 10 years ago

Try push, I gave up re-trying some of the Windows 7 infrastructure issues:
https://tbpl.mozilla.org/?tree=Try&rev=0036cfff43a8

Landing in part number order, thanks.

	Carsten Book [:Tomcat]
	Comment 14 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ce58071a332
https://hg.mozilla.org/integration/mozilla-inbound/rev/27ca85defc8d
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e6f6f7d7753
https://hg.mozilla.org/integration/mozilla-inbound/rev/7035cc030e4e

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 15 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/0ce58071a332
https://hg.mozilla.org/mozilla-central/rev/27ca85defc8d
https://hg.mozilla.org/mozilla-central/rev/4e6f6f7d7753
https://hg.mozilla.org/mozilla-central/rev/7035cc030e4e