1050795 - Remove final use of nsCxPusher in WorkerRunnable and the class itself for bug 951991

Status: RESOLVED FIXED

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

Description

[Bob Owen (:bobowen)]

These patches were moved from bug 1047509, so as to not block the other patches from landing.

Comment 1

[Bob Owen (:bobowen)]

Attachment: Part 1: Replace nsCxPusher in WorkerRunnable::Run v1

Comment 2

[Bob Owen (:bobowen)]

Attachment: Part 2: Remove nsCxPusher v1

Comment 3

[Bob Owen (:bobowen)]

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

Comment 4

[Bob Owen (:bobowen)]

Comment on attachment 8470070 [details] [diff] [review]
Part 1: Replace nsCxPusher in WorkerRunnable::Run v1

Addressing comments from previous patch Part 4 on bug 1047509

I'm on PTO next week, but I should be able to wrap this all up fairly quickly when I get back.
I expect this will need some more work.

(In reply to Bobby Holley (:bholley) from comment #18)
> Comment on attachment 8469440 [details] [diff] [review]
> Part 4: Replace nsCxPusher in WorkerRunnable::Run v1
> 
> Review of attachment 8469440 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/workers/WorkerRunnable.cpp
> @@ +262,5 @@
> >      return NS_OK;
> >    }
> >  
> > +  nsCOMPtr<nsIGlobalObject> globalObject;
> > +  bool isMainThread = false;
> 
> I'd prefer:
> 
> bool isMainThread = !targetIsWorkerThread &&
> !mWorkerPrivate->GetGlobalScope();
> MOZ_ASSERT(isMainThread == NS_IsMainThread());

Done ... I assume you meant !mWorkerPrivate->GetParent()

> @@ +267,1 @@
> >    JSContext* cx;
> 
> Let's move the definition of cx to immediately above the jsapi/aes
> construction.
> 
> @@ +295,5 @@
> > +    if (isMainThread) {
> > +      globalObject = static_cast<nsGlobalWindow*>(mWorkerPrivate->GetWindow());
> > +    } else {
> > +      globalObject = mWorkerPrivate->GetParent()->GlobalScope();
> > +      cx = mWorkerPrivate->GetParent()->GetJSContext();
> 
> Let's remove the cx assignment - see below.

Definition and assignment moved below.
Thanks, I see that the idea is to let the choice of globalObject drive things, whereas before it was JSContext.

> @@ +311,5 @@
> > +    if (isMainThread) {
> > +      aes.construct(globalObject);
> > +    } else {
> > +      aes.construct(globalObject, false, cx);
> > +    }
> 
> I'd prefer to remove the isMainThread conditional and make this all:
> 
> aes.construct(globalObject, isMainThread, isMainThread ? nullptr :
> GetCurrentThreadJSContext());

Done.

> @@ +326,5 @@
> >  
> >    Maybe<JSAutoCompartment> ac;
> >    if (targetCompartmentObject) {
> >      ac.construct(cx, targetCompartmentObject);
> >    }
> 
> All this targetCompartmentObject logic is unnecessary now, right? In any
> case where the object is non-null, we've already constructed an aes, and
> should be in the right compartment right?

I may have misunderstood what's going on here, but I think we still need to enter the compartment of our mWorkerPrivate to reproduce the previous behaviour.
As if we're not on our thread we'll be in our parent's compartment or possibly the null compartment.

Also, in the old code when on the worker thread it used to enter the compartment of CurrentGlobalOrNull, which I think would be the compartment we're already in, so I just removed that.
It does mean in the new code we will definitely be in our compartment, where in the old code it would have been whatever the compartment the context used to be in ... maybe ... I think.

> I think we should nuke it. Then, we can replace the Maybe<> stuff below with
> a conditional construction of aes2 that happens if aes wasn't constructed
> and mWorkerPrivate->GlobalScope() is now non-null.

I've switched round the jsapi and aes and then just constructed the aes, which I think gives the same effect.
I was thinking, would we actually ever want to run script in PostRun?

Comment 5

[Bob Owen (:bobowen)]

Comment on attachment 8470071 [details] [diff] [review]
Part 2: Remove nsCxPusher v1

Thought you might enjoy r+ing this one again. :D

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8470070 [details] [diff] [review]
Part 1: Replace nsCxPusher in WorkerRunnable::Run v1

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

(In reply to Bob Owen (:bobowen) from comment #4)
> I may have misunderstood what's going on here, but I think we still need to
> enter the compartment of our mWorkerPrivate to reproduce the previous
> behaviour.

I'm not the expert on this code either, so I appreciate your skepticism! Let's make sure we reach a consensus that we both agree is correct here. :-)

> As if we're not on our thread we'll be in our parent's compartment

Which is the same compartment we get for WorkerPrivate->GetWrapper(), right? WorkerPrivate's wrapper represents the "Worker" object (I think) in the parent's scope, which is either a DOMWindow or another Worker. 

I'd think you could add:

MOZ_ASSERT_IF(globalObject,
              js::GetGlobalForObjectCrossCompartment(WorkerPrivate->GetWrapper()) == globalObject->GetGlobalJSObject());

Assuming that assert doesn't fire, I think we should be able to remove this logic.

> or possibly the null compartment.

This only happens if we use the AutoJSAPI, which only happens if globalObject is null, which only happens in the targetIsWorkerThread case, and where targetCompartmentObject would end up being null anyway.

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8470070 [details] [diff] [review]
Part 1: Replace nsCxPusher in WorkerRunnable::Run v1

Canceling review while we sort this out. :-)

Comment 8

[Bob Owen (:bobowen)]

(In reply to Bobby Holley (:bholley) from comment #6)
> Comment on attachment 8470070 [details] [diff] [review]
> Part 1: Replace nsCxPusher in WorkerRunnable::Run v1
> 
> Review of attachment 8470070 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Bob Owen (:bobowen) from comment #4)
> > I may have misunderstood what's going on here, but I think we still need to
> > enter the compartment of our mWorkerPrivate to reproduce the previous
> > behaviour.
> 
> I'm not the expert on this code either, so I appreciate your skepticism!
> Let's make sure we reach a consensus that we both agree is correct here. :-)
> 
> > As if we're not on our thread we'll be in our parent's compartment
> 
> Which is the same compartment we get for WorkerPrivate->GetWrapper(), right?
> WorkerPrivate's wrapper represents the "Worker" object (I think) in the
> parent's scope, which is either a DOMWindow or another Worker. 

Ah, so as I half suspected I was indeed misunderstanding things.
In which case, when we have a globalObject then this is superfluous.

I have to admit that I don't really understand how and when a lot of objects get wrapped.
I think this is because it often involves codegened classes.
I should have fired up gdb to look at this though. :-)
 
> > or possibly the null compartment.
> 
> This only happens if we use the AutoJSAPI, which only happens if
> globalObject is null, which only happens in the targetIsWorkerThread case,
> and where targetCompartmentObject would end up being null anyway.

With my current patch, globalObject is sometimes null when on main thread.
This seems to be when our parent is a BackStagePass (from looking at the global object of mWorkerPrivate->GetWrapper()  :-) ) and the runnable is a ModifyBusyCountRunnable or MessageEventRunnable.
So, I'm thinking that maybe I should be using xpc::GetNativeForGlobal(js::GetGlobalForObjectCrossCompartment(mWorkerPrivate->GetWrapper())) to get globalObject.

I need to look at the exiting code to see if this is the right way to go and to make sure we end up with equivalent behaviour (assuming we think it's correct).

Not sure how much time I'll have before the 18th, Birthday tomorrow and then PTO for the rest of the week.

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 8470070 [details] [diff] [review]
Part 1: Replace nsCxPusher in WorkerRunnable::Run v1

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

Ok. I think we should sort this out, but in the mean time I'd like to get these patches landed so that nsCxPusher goes away. I'll push them, and we can file a followup bug when you get back to simplify the compartment entering.

Comment 10

[Bobby Holley (:bholley)]

Flagging bob for info so that we don't lose track of the followup work.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9ea1fc4f59e0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/17fd16e2f47b

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9ea1fc4f59e0
https://hg.mozilla.org/mozilla-central/rev/17fd16e2f47b

Comment 12

[Bob Owen (:bobowen)]

(In reply to Bobby Holley (:bholley) from comment #9)
> Comment on attachment 8470070 [details] [diff] [review]
> Part 1: Replace nsCxPusher in WorkerRunnable::Run v1
> 
> Review of attachment 8470070 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ok. I think we should sort this out, but in the mean time I'd like to get
> these patches landed so that nsCxPusher goes away. I'll push them, and we
> can file a followup bug when you get back to simplify the compartment
> entering.

Thanks Bobby.
I'm sure it's wrong to think of the death of nsCxPusher as a bonus Birthday present. :)

Just thinking, along with the other bugs you've just filed, do we need to look at the other callers of JS_FireOnNewGlobalObject?

Comment 13

[Bobby Holley (:bholley)]

(In reply to Bob Owen (:bobowen) from comment #12)
> Just thinking, along with the other bugs you've just filed, do we need to
> look at the other callers of JS_FireOnNewGlobalObject?

Yes, we'll need an AutoEntryScript for all of those. File a bug?

Comment 14

[Bob Owen (:bobowen)]

(In reply to Bobby Holley (:bholley) from comment #10)
> Flagging bob for info so that we don't lose track of the followup work.

Follow-up bug 1054976 filed.