1025476 - Add compulsory Init functions to AutoJSAPI to allow caller to control where errors are reported and the initial compartment entered

Status: RESOLVED FIXED

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

Description

[Bob Owen (:bobowen)]

These changes are a result of using AutoJSAPI and derivatives for a while and a discussion in bug 1023969, which continued on IRC between bholley, smaug and me.

The plan is to add the following Init functions to AutoJSAPI and to make the calling of one of these compulsory. Much, if not all, of the current functionality in the constructors will move to the Init functions.

* void AutoJSAPI::Init() - get the SafeJSContext (or worker equivalent), push the context if on main thread and enter the null compartment via a JSAutoNullCompartment. This should never fail.

* bool AutoJSAPI::Init(nsIGlobalObject* aGlobalObject) - get and push the JSContext for the global (or if unavailable the SafeJSContext) and enter the compartment of its corresponding JS global. This returns true if successful and false otherwise.

This will hopefully make it more straight forward when using AutoJSAPI.
It will also remove the need for AutoJSAPIWithErrorsReportedToWindow, as we should be able to use the latter to replace it.

Comment 1

[Bob Owen (:bobowen)]

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

Need to add/change comments.
I've added a bool parameter to control whether it tries to push global object's JSContext for error reporting purposes.
I know that isn't quite what we talked about, but it seemed to make sense as I was doing it.

It means that these changes should basically give the same functionality as before.
I can still worry less about trying to not pass true here as I go through the rest of them and it'll give an easy way of following up on these if we want to, once bug 951991 is out of the way.
It would also mean that you could use this Init off main thread.

Anyway, it would be quick to rip out if you decide against it.

Comment 2

[Bob Owen (:bobowen)]

Attachment: Part 1: Add compulsory Init functions to AutoJSAPI v1

Hopefully this is a reasonable first stab at this.

In addition to what I'd said in comment 1, I've added (hopefully meaningful) comments.
I've also split the "Legacy" error reporting bit into a separate function instead of using a flag parameter.
This meant that it was neater to do the checking for a null JS global after the context had been grabbed, but before it had been pushed.  Doing it before any of this was triggering a rooting hazard.
Anyway, I generally find to road to hell is paved with bool parameters. :-)

Sorry about doing this all in one big patch, I was finding trying to do it incrementally much more painful.

Here's another try push, with virtually the exact same changes (only comment differences I think):
https://tbpl.mozilla.org/?tree=Try&rev=90044785c4ff

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 8440720 [details] [diff] [review]
Part 1: Add compulsory Init functions to AutoJSAPI v1

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

Just a few things, but I'm probably going to want to look at this again.

::: content/media/webaudio/AudioDestinationNode.cpp
@@ +126,5 @@
>      // which is strongly referenced by the runnable that called
>      // AudioDestinationNode::FireOfflineCompletionEvent.
>  
> +    AutoJSAPI jsapi;
> +    if (NS_WARN_IF(!jsapi.Init(aNode->DOMEventTargetHelper::GetParentObject()))) {

I'm assuming you've audited all of these changes to make sure that they do the same thing as they did before?

Also, why do we need to fully qualify all of them? In general, it seems like we want the most-derived version of GetParentObject().

::: dom/base/ScriptSettings.cpp
@@ +260,4 @@
>  {
> +  MOZ_ASSERT(!mCx, "An AutoJSAPI should only be initialised once");
> +
> +  if (!aGlobalObject) {

MOZ_WARN_IF here. I understand that it's also optimal for the caller's to MOZ_WARN_IF (to get a more useful file/line), but having it twice isn't really a problem, and it at least gives us something if the caller forgets.

@@ +265,5 @@
> +  }
> +
> +  mCx = nsContentUtils::GetDefaultJSContextForThread();
> +
> +  JSObject* global = aGlobalObject->GetGlobalJSObject();

Did you confirm with sfink whether this was a rooting hazard given the mCxPusher pusher construction below?

@@ +266,5 @@
> +
> +  mCx = nsContentUtils::GetDefaultJSContextForThread();
> +
> +  JSObject* global = aGlobalObject->GetGlobalJSObject();
> +  if (!global) {

And here.

@@ +267,5 @@
> +  mCx = nsContentUtils::GetDefaultJSContextForThread();
> +
> +  JSObject* global = aGlobalObject->GetGlobalJSObject();
> +  if (!global) {
> +    mCx = nullptr;

Instead of doing this, just set mCx below this block.

@@ +281,4 @@
>  }
>  
> +bool
> +AutoJSAPI::InitWithLegacyErrorReporting(nsIGlobalObject* aGlobalObject)

I think we should factor out a separate ::Init(nsIglobalObject*, JSContext*) method that is called both here (with the result of FindJSContext) and by the the Init function above (with the result of GetDefaultJSContextForThread).

::: dom/base/ScriptSettings.h
@@ +98,4 @@
>   *
>   * * Grabbing an appropriate JSContext, and, on the main thread, pushing it onto
>   *   the JSContext stack.
> + * * Entering an initial compartment, to ensure that the previously entered

Note I'd say:

Entering an initial (possibly null) compartment.

@@ +160,5 @@
> +  // be used. This constructor initialises the AutoJSAPI, so Init must NOT be
> +  // called on subclasses that use this.
> +  // If aGlobalObject, its associated JS global or aCx are null this will cause
> +  // an assertion, as will setting aIsMainThread incorrectly.
> +  AutoJSAPI(nsIGlobalObject* aGlobalObject, bool aIsMainThread, JSContext* aCx);

I think the subclasses (like AutoEntryScript) should also grow an Init() function, and we should ditch this infallible protected constructor. The same logic applies there, right?

I'm fine with this happening in a followup bug though.

::: dom/events/EventListenerManager.cpp
@@ +777,4 @@
>    // Activate JSAPI, and make sure that exceptions are reported on the right
>    // Window.
> +  AutoJSAPI jsapi;
> +  NS_ENSURE_STATE(jsapi.InitWithLegacyErrorReporting(global));

We should avoid doing actual work inside these macros.

::: dom/workers/WorkerPrivate.cpp
@@ +3128,5 @@
>      size_t actionsIndex = windowActions.LastIndexOf(WindowAction(window));
>  
> +    AutoJSAPI jsapi;
> +    if (NS_WARN_IF(!jsapi.InitWithLegacyErrorReporting(
> +                            sharedWorker->GetParentObject()))) {

This indentation seems weird?

::: js/src/jsapi.h
@@ +1705,5 @@
>      JSContext *cx_;
>      JSCompartment *oldCompartment_;
>    public:
> +    explicit JSAutoNullableCompartment(JSContext *cx, JSObject *target);
> +    explicit JSAutoNullableCompartment(JSContext *cx);

This API basically forces consumers to construct it with a Maybe<> in order to use both modes. I think it makes more sense to have a single constructor whose JSObject* argument is targetOrNull, and dynamically handle it in the constructor implementation.

Comment 4

[Bob Owen (:bobowen)]

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

Thanks, not got much time, off to sleep.
I'll try and fix these tomorrow, I'll just go through them quickly to try and save another round trip.

> > +    AutoJSAPI jsapi;
> > +    if (NS_WARN_IF(!jsapi.Init(aNode->DOMEventTargetHelper::GetParentObject()))) {
> 
> I'm assuming you've audited all of these changes to make sure that they do
> the same thing as they did before?

Yeah, we're getting to the object via slightly different routes sometimes, but apart from that it should be the same.
That's partly why I added the legacy reporting option, so that there was no change, even though it would probably have been going back to the behaviour before I started messing with all this.
 
> Also, why do we need to fully qualify all of them? In general, it seems like
> we want the most-derived version of GetParentObject().

Unfortunately GetParentObject is redefined to return nsPIDOMWindow* on a lot of these objects, even though they already return it for GetOwner().
 
> > +  mCx = nsContentUtils::GetDefaultJSContextForThread();
> > +
> > +  JSObject* global = aGlobalObject->GetGlobalJSObject();
> 
> Did you confirm with sfink whether this was a rooting hazard given the
> mCxPusher pusher construction below?

No, as it wasn't being flagged as one, only when called before the code to find the JSContext.
I can check with him though.

> > +  mCx = nsContentUtils::GetDefaultJSContextForThread();
> > +
> > +  JSObject* global = aGlobalObject->GetGlobalJSObject();
> > +  if (!global) {
> > +    mCx = nullptr;
> 
> Instead of doing this, just set mCx below this block.

So, hold in a local variable first?

> > +bool
> > +AutoJSAPI::InitWithLegacyErrorReporting(nsIGlobalObject* aGlobalObject)
> 
> I think we should factor out a separate ::Init(nsIglobalObject*, JSContext*)
> method that is called both here (with the result of FindJSContext) and by
> the the Init function above (with the result of
> GetDefaultJSContextForThread).

OK, I think it will mean an extra check for the legacy route, but I don't suppose that matters much.

> 
> I think the subclasses (like AutoEntryScript) should also grow an Init()
> function, and we should ditch this infallible protected constructor. The
> same logic applies there, right?
> 
> I'm fine with this happening in a followup bug though.

I was thinking we might want to do that, I'll file a bug.
 
> ::: dom/events/EventListenerManager.cpp
> @@ +777,4 @@
> >    // Activate JSAPI, and make sure that exceptions are reported on the right
> >    // Window.
> > +  AutoJSAPI jsapi;
> > +  NS_ENSURE_STATE(jsapi.InitWithLegacyErrorReporting(global));
> 
> We should avoid doing actual work inside these macros.

OK, does the same go for NS_WARN_IF and NS_FAILED?
 
> ::: dom/workers/WorkerPrivate.cpp
> @@ +3128,5 @@
> >      size_t actionsIndex = windowActions.LastIndexOf(WindowAction(window));
> >  
> > +    AutoJSAPI jsapi;
> > +    if (NS_WARN_IF(!jsapi.InitWithLegacyErrorReporting(
> > +                            sharedWorker->GetParentObject()))) {
> 
> This indentation seems weird?

Yeah, but it goes over 80 chars otherwise and there didn't seem to be a nice place to split it.
 
> ::: js/src/jsapi.h
> @@ +1705,5 @@
> >      JSContext *cx_;
> >      JSCompartment *oldCompartment_;
> >    public:
> > +    explicit JSAutoNullableCompartment(JSContext *cx, JSObject *target);
> > +    explicit JSAutoNullableCompartment(JSContext *cx);
> 
> This API basically forces consumers to construct it with a Maybe<> in order
> to use both modes. I think it makes more sense to have a single constructor
> whose JSObject* argument is targetOrNull, and dynamically handle it in the
> constructor implementation.

Well, I thought it was better in case they accidentally passed null.
But I guess they would find out soon enough.

Comment 5

[Bobby Holley (:bholley)]

(In reply to Bob Owen (:bobowen) from comment #4)
> Unfortunately GetParentObject is redefined to return nsPIDOMWindow* on a lot
> of these objects, even though they already return it for GetOwner().

Let's just add an nsPIDOMWindow* overload to AutoJSAPI that static_casts to nsGlobalWindow* (and implicitly then to nsIGlobalObject*)?

> > > +  mCx = nsContentUtils::GetDefaultJSContextForThread();
> > > +
> > > +  JSObject* global = aGlobalObject->GetGlobalJSObject();
> > > +  if (!global) {
> > > +    mCx = nullptr;
> > 
> > Instead of doing this, just set mCx below this block.
> 
> So, hold in a local variable first?

Why do we need to hold it at all?

> > > +  AutoJSAPI jsapi;
> > > +  NS_ENSURE_STATE(jsapi.InitWithLegacyErrorReporting(global));
> > 
> > We should avoid doing actual work inside these macros.
> 
> OK, does the same go for NS_WARN_IF and NS_FAILED?

I think those are special cases. The rule I'm remembering was mostly applied to NS_ENSURE macros. Basically, extracting the return value and doing the NS_ENSURE As a separate line.

> Well, I thought it was better in case they accidentally passed null.
> But I guess they would find out soon enough.

They already have to explicitly choose JSAutoNullableCompartment, so I'm not worried about that.

Comment 6

[Bob Owen (:bobowen)]

Attachment: Part 1: Add compulsory Init functions to AutoJSAPI

Sorry it's taken so long to get these done.
I got ambushed by the compiler and my C++ knowledge and then my Internet searching skills let me down. :-(

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

I'll go back through your original comments.

(In reply to Bobby Holley (:bholley) from comment #3)
> Comment on attachment 8440720 [details] [diff] [review]
> Part 1: Add compulsory Init functions to AutoJSAPI v1
> 
> Review of attachment 8440720 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just a few things, but I'm probably going to want to look at this again.

I'm pretty sure both of these will still be true. :-)
 
> Also, why do we need to fully qualify all of them? In general, it seems like
> we want the most-derived version of GetParentObject().

When I added an overload to Init for nsPIDOMWindow the compiler started complaining about the call of overload being ambiguous, when calling Init with an nsGlobalWindow (even from within the function itself after the static_cast).

This was the ambush and I now know somewhat more about overload resolution in C++, which is no bad thing.
However, I couldn't find a way to tell the compiler which overload to prefer if the ranking was the same.

I could have used casting again I think, but given the consumer was having to make the choice, I thought it was better to rename the new functions to InitUsingWin etc..
That way it is clear, which is preferred if both types are readily available.

> > +  if (!aGlobalObject) {
> 
> MOZ_WARN_IF here. I understand that it's also optimal for the caller's to
> MOZ_WARN_IF (to get a more useful file/line), but having it twice isn't
> really a problem, and it at least gives us something if the caller forgets.

Added.
 
> > +  JSObject* global = aGlobalObject->GetGlobalJSObject();
> > +  if (!global) {
> 
> And here.

Added.

> > +  JSObject* global = aGlobalObject->GetGlobalJSObject();
> 
> Did you confirm with sfink whether this was a rooting hazard given the
> mCxPusher pusher construction below?

Still need to ask sfink about this.
 
> > +  JSObject* global = aGlobalObject->GetGlobalJSObject();
> > +  if (!global) {
> > +    mCx = nullptr;
> 
> Instead of doing this, just set mCx below this block.

This problem went away with the refactoring.

> > +AutoJSAPI::InitWithLegacyErrorReporting(nsIGlobalObject* aGlobalObject)
> 
> I think we should factor out a separate ::Init(nsIglobalObject*, JSContext*)
> method that is called both here (with the result of FindJSContext) and by
> the the Init function above (with the result of
> GetDefaultJSContextForThread).

Hopefully, what I've done is what you were thinking.

Once I done it (and with the JSAutoNullableCompartment change below) an extra bit of refactoring to have a common InitInternal, seemed to make sense.

> > + * * Entering an initial compartment, to ensure that the previously entered
> 
> Note I'd say:
> 
> Entering an initial (possibly null) compartment.

Added.

> @@ +160,5 @@
> > +  // be used. This constructor initialises the AutoJSAPI, so Init must NOT be
> > +  // called on subclasses that use this.
> > +  // If aGlobalObject, its associated JS global or aCx are null this will cause
> > +  // an assertion, as will setting aIsMainThread incorrectly.
> > +  AutoJSAPI(nsIGlobalObject* aGlobalObject, bool aIsMainThread, JSContext* aCx);
> 
> I think the subclasses (like AutoEntryScript) should also grow an Init()
> function, and we should ditch this infallible protected constructor. The
> same logic applies there, right?
> 
> I'm fine with this happening in a followup bug though.

Still need to file this, I'll do it tomorrow, I'm off out in a second.

> > +  NS_ENSURE_STATE(jsapi.InitWithLegacyErrorReporting(global));
> 
> We should avoid doing actual work inside these macros.

Changed to |NS_WARN_IF|s.

> > +    if (NS_WARN_IF(!jsapi.InitWithLegacyErrorReporting(
> > +                            sharedWorker->GetParentObject()))) {
> 
> This indentation seems weird?

I've removed this, even though it goes over 80, I agree it is probably clearer.

> > +    explicit JSAutoNullableCompartment(JSContext *cx, JSObject *target);
> > +    explicit JSAutoNullableCompartment(JSContext *cx);
> 
> This API basically forces consumers to construct it with a Maybe<> in order
> to use both modes. I think it makes more sense to have a single constructor
> whose JSObject* argument is targetOrNull, and dynamically handle it in the
> constructor implementation.

Changed, which was useful for me as well. :-)

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8442227 [details] [diff] [review]
Part 1: Add compulsory Init functions to AutoJSAPI

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

Looks great! Thanks for doing this.

::: dom/base/ScriptSettings.h
@@ +138,5 @@
> +  // false and use of cx() will cause an assertion.
> +  bool Init(nsIGlobalObject* aGlobalObject);
> +
> +  // Unsurprisingly, this uses aCx and enters the compartment of aGlobalObject.
> +  // If aGlobalObject or its associated JS global are null then it returns,

Nit - remove the trailing commma?

@@ +153,5 @@
> +  // If aGlobalObject or its associated JS global are null then it returns,
> +  // false and use of cx() will cause an assertion.
> +  bool InitWithLegacyErrorReporting(nsIGlobalObject* aGlobalObject);
> +
> +  // Convenience functions to take an nsPIDOMwindow*, when it is more easily

nit: nsPIDOMWindow

Comment 8

[Bob Owen (:bobowen)]

Attachment: Part 1: Add compulsory Init functions to AutoJSAPI v3

r=bholley - from comment 7 with corrections as below.

Fairly full try push, because of the number of changes:
https://tbpl.mozilla.org/?tree=Try&rev=33d778d0c897


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

> Looks great! Thanks for doing this.

No problem.

> > +  // If aGlobalObject or its associated JS global are null then it returns,
> 
> Nit - remove the trailing commma?

Damn, I saw this (and the ones in the other Init comments) on my final check through the patch before uploading, but in my rush I forgot ... thanks.
 
> > +  // Convenience functions to take an nsPIDOMwindow*, when it is more easily
> 
> nit: nsPIDOMWindow

Fixed, thanks.

Comment 9

[Bob Owen (:bobowen)]

Comment on attachment 8442679 [details] [diff] [review]
Part 1: Add compulsory Init functions to AutoJSAPI v3

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

::: dom/base/ScriptSettings.cpp
@@ +234,5 @@
>    if (aIsMainThread) {
>      mCxPusher.construct(mCx);
>    }
>  
> +  mAutoNullableCompartment.construct(mCx, aGlobal);

Hi sfink,
bholley wanted to make sure you're happy that I'm not introducing a rooting hazard here by holding the unrooted JS global (aGlobal) over this mCxPusher construction. The browser rooting analysis doesn't report it as one.
Thanks.

Comment 10

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8442679 [details] [diff] [review]
Part 1: Add compulsory Init functions to AutoJSAPI v3

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

::: dom/base/ScriptSettings.cpp
@@ +234,5 @@
>    if (aIsMainThread) {
>      mCxPusher.construct(mCx);
>    }
>  
> +  mAutoNullableCompartment.construct(mCx, aGlobal);

It's not a hazard because it's not live across the construction -- aGlobal is only passed into AutoCxPusher::AutoCxPusher, it is dead afterward.

It does mean the caller of InitInternal had better not use that JSObject* after the call, either, or the hazard analysis *will* yell. (Well, all of its callers are Init() functions, which trivially do not use the global afterwards. But their callers won't be able to use it afterwards either.)

Comment 11

[Bob Owen (:bobowen)]

Thanks Steve.

Last couple of try pushes from below that cover most things:
https://tbpl.mozilla.org/?tree=Try&rev=a253417b34be
https://tbpl.mozilla.org/?tree=Try&rev=33d778d0c897

Thanks.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d6cff589e8f

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8d6cff589e8f