Closed Bug 1037564 Opened 5 years ago Closed 5 years ago

Change usages of nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI and derivatives for bug 951991 - batch 9

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: bobowen, Assigned: bobowen)

References

Details

Attachments

(4 files, 3 obsolete files)

Another batch of patches moving nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI etc.
See bug 951991 for details.
https://tbpl.mozilla.org/?tree=Try&rev=2bbb4adb8b39
Assignee: nobody → bobowencode
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.
Attachment #8455464 - Flags: review?(bobbyholley)
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.
Attachment #8455511 - Flags: review?(bobbyholley)
Attachment #8455464 - Flags: review?(bobbyholley) → review+
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.
Attachment #8455511 - Flags: review?(bobbyholley) → review+
(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.
Attachment #8455511 - Attachment is obsolete: true
Attachment #8456067 - Flags: review?(bobbyholley)
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.
Attachment #8456085 - Flags: review?(bobbyholley)
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.
Attachment #8456091 - Flags: review?(bobbyholley)
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.
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'.
Attachment #8456067 - Flags: review?(bobbyholley) → review+
Attachment #8456085 - Flags: review?(bobbyholley) → review+
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().
Attachment #8456091 - Flags: review?(bobbyholley) → review+
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.
Attachment #8456067 - Attachment is obsolete: true
Attachment #8458637 - Flags: review?(bobbyholley)
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.
Attachment #8456091 - Attachment is obsolete: true
Attachment #8458638 - Flags: review+
Attachment #8458637 - Flags: review?(bobbyholley) → review+
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.
Keywords: checkin-needed
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.