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

Status: RESOLVED FIXED

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

Description

[Bob Owen (:bobowen)]

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

Comment 1

[Bobby Holley (:bholley)]

Note that you shouldn't spend too much time debugging weirdness here until we land bug 997440 (see the first comment in that bug). It's currently blocked on reviews for the dep.

Comment 2

[Bob Owen (:bobowen)]

Attachment: Part 1: Replace AutoPushJSContext in EventSource::DispatchAllMessageEvents v1

I don't think we're running any script here so we should just need an AutoJSAPI and to enter the compartment for the owning / parent object.

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 8417651 [details] [diff] [review]
Part 1: Replace AutoPushJSContext in EventSource::DispatchAllMessageEvents v1

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

Nice!

Comment 4

[Bob Owen (:bobowen)]

Attachment: Part 2: Replace AutoPushJSContext in nsFrameLoader::EnsureMessageManager v1

This looks similar to the case in nsGlobalWindow::GetMessageManager.             
I can't see where this pushed context is needed.                                 
Keeping the null checks just to maintain behaviour.

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 8418123 [details] [diff] [review]
Part 2: Replace AutoPushJSContext in nsFrameLoader::EnsureMessageManager v1

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

::: content/base/src/nsFrameLoader.cpp
@@ +2416,5 @@
>    nsIScriptContext* sctx = mOwnerContent->GetContextForEventHandlers(&rv);
>    NS_ENSURE_SUCCESS(rv, rv);
> +  if (NS_WARN_IF(!sctx || !(sctx->GetNativeContext()))) {
> +    return NS_ERROR_UNEXPECTED;
> +  }

We're going to need to remove this at some point anyway when nsIScriptContext and JSContext go away. So can we just remove them now, possibly in a separate patch?

Comment 6

[Bob Owen (:bobowen)]

Attachment: Part 3: Remove context null checks from nsFrameLoader::EnsureMessageManager v1

(In reply to Bobby Holley (:bholley) from comment #5)
> Comment on attachment 8418123 [details] [diff] [review]

> We're going to need to remove this at some point anyway when
> nsIScriptContext and JSContext go away. So can we just remove them now,
> possibly in a separate patch?

Sure, should I remove the ones in nsGlobalWindow::GetMessageManager as well in another patch?

Comment 7

[Bobby Holley (:bholley)]

(In reply to Bob Owen (:bobowen) from comment #6)
> Sure, should I remove the ones in nsGlobalWindow::GetMessageManager as well
> in another patch?

Yes please!

Comment 8

[Bob Owen (:bobowen)]

Attachment: Part 4: Remove context null checks from nsGlobalWindow::GetMessageManager v1

Follow up to patch Part 6  from bug 988383.

Comment 9

[Bob Owen (:bobowen)]

Attachment: Part 5: Replace AutoPushJSContext in AudioDestinationNode FireOfflineCompletionEvent v1

InitializeBuffers is just creating an array and SetRawChannelContents doesn't seem to use the context at all.

There are three other places that are similar to this.
I don't think that SetRawChannelContents is exposed to anything else, so should I remove the JSContext parameter as a final patch after those three?

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8418764 [details] [diff] [review]
Part 5: Replace AutoPushJSContext in AudioDestinationNode FireOfflineCompletionEvent v1

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

Yes, please remove the cx parameter to GetRawChannelContents.

Comment 11

[Bob Owen (:bobowen)]

So, I've made similar changes (to Part 5), to AudioProcessingEvent::LazilyCreateBuffer [1] and WebAudioDecodeJob::AllocateBuffer [2].
Using |mNode->Context()->GetWrapperPreserveColor()| and |mContext->GetWrapperPreserveColor()| respectively to get the JSObject* for the JSAutoCompartment.

But when it came to Command::Run in ScriptProcessorNodeEngine::SendBuffersToMainThread [3], when I tried using |node->Context()->GetWrapperPreserveColor()|, it was sometimes returning null.
I then realised that the |node| was also an nsWrapperCache, but it returns null for that as well.

This always seems to work:
        nsCOMPtr<nsIGlobalObject> parentObject =
          do_QueryInterface(node->Context()->GetParentObject());
...
        JSAutoCompartment ac(cx, parentObject->GetGlobalJSObject());

So, would it be safer to use this method for the other three cases as well?

I'll not ni you as I can see you're up to your eyeballs at the moment.


[1] http://dxr.mozilla.org/mozilla-central/source/content/media/webaudio/AudioProcessingEvent.cpp#43
[2] http://dxr.mozilla.org/mozilla-central/source/content/media/webaudio/MediaBufferDecoder.cpp#406
[3] http://dxr.mozilla.org/mozilla-central/source/content/media/webaudio/ScriptProcessorNode.cpp#405

Comment 12

[Bobby Holley (:bholley)]

(In reply to Bob Owen (:bobowen) from comment #11)
> So, I've made similar changes (to Part 5), to
> AudioProcessingEvent::LazilyCreateBuffer [1] and
> WebAudioDecodeJob::AllocateBuffer [2].
> Using |mNode->Context()->GetWrapperPreserveColor()| and
> |mContext->GetWrapperPreserveColor()| respectively to get the JSObject* for
> the JSAutoCompartment.
> 
> But when it came to Command::Run in
> ScriptProcessorNodeEngine::SendBuffersToMainThread [3], when I tried using
> |node->Context()->GetWrapperPreserveColor()|, it was sometimes returning
> null.
> I then realised that the |node| was also an nsWrapperCache, but it returns
> null for that as well.
> 
> This always seems to work:
>         nsCOMPtr<nsIGlobalObject> parentObject =
>           do_QueryInterface(node->Context()->GetParentObject());
> ...
>         JSAutoCompartment ac(cx, parentObject->GetGlobalJSObject());
> 
> So, would it be safer to use this method for the other three cases as well?

Assuming node->Context()->GetParentObject() always gives the C++ global, that does sound better, yes.

Comment 13

[Bob Owen (:bobowen)]

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

> > So, would it be safer to use this method for the other three cases as well?
> 
> Assuming node->Context()->GetParentObject() always gives the C++ global,
> that does sound better, yes.

Well, it returns an nsPIDOMWindow* and all the previous code calls 
AudioContext::GetJSContext(), which uses it internally to get an nsIScriptGlobalObject on its way to the JSContext.
So, it should be at least as reliable as the existing code.

I'll get the patches changed and uploaded in the morning.

Comment 14

[Bob Owen (:bobowen)]

Attachment: Part 1: Replace AutoPushJSContext in EventSource::DispatchAllMessageEvents v1

r=bholley from comment 3.

Just realised I could remove the nsCxPusher.h include.
Also a one word change to the comment.

Comment 15

[Bob Owen (:bobowen)]

Attachment: Part 5: Replace AutoPushJSContext in AudioDestinationNode FireOfflineCompletionEvent v2

Changed to use GetParentObject() on the AudioContext to get to the C++ global and then JSObject* for the JSAutoCompartment.

Added an NS_WARN_IF, before returning, if the parent object isn't an nsIGlobalObject, as it seems wrong to just fail silently here.

Comment 16

[Bob Owen (:bobowen)]

Attachment: Part 6: Replace AutoPushJSContext in AudioProcessingEvent::LazilyCreateBuffer v1

Very similar to Part 5.
The only thing I've added is an NS_WARN_IF and return if we don't get an nsIGlobalObject.
The old code didn't check in case GetJSContext() returns null.
Given that the other three variants of this code all do (did), it seems prudent as I think in the old code InitializeBuffers would have crashed.

Comment 17

[Bob Owen (:bobowen)]

Attachment: Part 7: Replace AutoPushJSContext in WebAudioDecodeJob::AllocateBuffer v1

Pretty much identical to Part 5.

Comment 18

[Bob Owen (:bobowen)]

Attachment: Part 8: Replace AutoPushJSContext in ScriptProcessNode - Command::Run v1

Again very similar, just some unindenting because of the early return.

Comment 19

[Bob Owen (:bobowen)]

Attachment: Part 9: Remove unused JSContext parameter from AudioBuffer::SetRawChannelContents v1

Another JSContext* parameter bites the dust.

When I run all the webaudio tests my laptop happily does an impression of an inebriated dolphin and all the tests pass.

Here's a try push:
https://tbpl.mozilla.org/?tree=Try&rev=b7e123f75fad

Comment 20

[Bob Owen (:bobowen)]

(In reply to Bob Owen (:bobowen) from comment #19)

> Here's a try push:
> https://tbpl.mozilla.org/?tree=Try&rev=b7e123f75fad

... which has a load of crashes ... sorry Bobby, should have pushed to try before asking for reviews.

It seems that sometimes in ScriptProcessorNodeEngine...Command::Run, the nsGlobalWindow returned doesn't have a wrapper.
It also doesn't seem to have an outer window, which is why GetJSContext() returned null in the old code and things didn't blow up.

The one I could reproduce locally happened after the test had finished, which seems a bit odd.

Anyway, I can also check GetGlobalJSObject() for null, here's the tests again with this change:
https://tbpl.mozilla.org/?tree=Try&rev=b4933213b327
https://tbpl.mozilla.org/?tree=Try&rev=9900c0dc889a

I'll split it out, so I don't call GetGlobalJSObject() twice, but this all seems to a lot of code to replace one or two lines previously.
It looks like AudioContext::GetJSContext is only used in these places, so maybe I should replace it with |JSObject* GetGlobalJSObjectOrNull()| and then they can all use that?

Comment 21

[Bobby Holley (:bholley)]

(In reply to Bob Owen (:bobowen) from comment #20)
> ... which has a load of crashes ... sorry Bobby, should have pushed to try
> It looks like AudioContext::GetJSContext is only used in these places, so
> maybe I should replace it with |JSObject* GetGlobalJSObjectOrNull()| and
> then they can all use that?

Yeah, that sounds reasonable.

Comment 22

[Bobby Holley (:bholley)]

Comment on attachment 8419953 [details] [diff] [review]
Part 5: Replace AutoPushJSContext in AudioDestinationNode FireOfflineCompletionEvent v2

Please reflag me when these are ready.

Comment 23

[Bob Owen (:bobowen)]

Attachment: Part 5: Replace AutoPushJSContext in AudioDestinationNode FireOfflineCompletionEvent v3

Added AudioContext::GetGlobalJSObjectOrNull and use this to get straight to the global so we can check we have one.

I've got the other patches ready, but I'll wait for the review on this one in case they need similar changes.
Everything looks good on the try push for the relevant tests so far:
https://tbpl.mozilla.org/?tree=Try&rev=3591cf846cb8

Comment 24

[Bobby Holley (:bholley)]

Comment on attachment 8420242 [details] [diff] [review]
Part 5: Replace AutoPushJSContext in AudioDestinationNode FireOfflineCompletionEvent v3

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

r=me with that

::: content/media/webaudio/AudioContext.h
@@ +220,5 @@
>    void Unmute() const;
>  
>    JSContext* GetJSContext() const;
>  
> +  JSObject* GetGlobalJSObjectOrNull() const;

Per the naming convention, the "Get" and "OrNull" are kind of redundant. "Get" generally means "may return null, possible indicating an error", whereas "OrNull" means ("may return null, but that's an allowable result, and no 'failure condition' exists").

Given that we're treating the lack of a global here as a pseudo error condition (and given that these are the same semantics of other GetGlobalJSObject functions),

This should probably be called GetGlobalJSObject().

Comment 25

[Bob Owen (:bobowen)]

Attachment: Part 5: Replace AutoPushJSContext in AudioDestinationNode FireOfflineCompletionEvent v4

r=bholley from comment 24 with function renamed to GetGlobalJSObject.

Comment 26

[Bob Owen (:bobowen)]

Attachment: Part 6: Replace AutoPushJSContext in AudioProcessingEvent::LazilyCreateBuffer v2

As before (comment 16) the only extra change here is that we warn and return if we don't get a global.
Before if there was no JSContext it would have crashed.

Comment 27

[Bob Owen (:bobowen)]

Attachment: Part 7: Replace AutoPushJSContext in WebAudioDecodeJob::AllocateBuffer v2

I have a strange sense of déjà vu.

Comment 28

[Bob Owen (:bobowen)]

Attachment: Part 8: Replace AutoPushJSContext in ScriptProcessorNode - Command::Run v2

Same again except the early return means we can unindent the code below.

Comment 29

[Bob Owen (:bobowen)]

Attachment: Part 10: Remove AudioContext::GetJSContext v1

And we say goodbye to some more JSContext etc. machinery.

Comment 30

[Bobby Holley (:bholley)]

Comment on attachment 8420274 [details] [diff] [review]
Part 8: Replace AutoPushJSContext in ScriptProcessorNode - Command::Run v2

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

I'm assuming most of this is just reindenting.

Comment 31

[Bobby Holley (:bholley)]

Hm, I actually wonder if we perhaps want AutoJSAPIWithErrorsReportedToWindow for these. Can you ask the owner of the code if they expect meaningful user-visible/script-visible errors to be reported to the DOMWindow here?

Comment 32

[Bob Owen (:bobowen)]

(In reply to Bobby Holley (:bholley) from comment #31)
> Hm, I actually wonder if we perhaps want AutoJSAPIWithErrorsReportedToWindow
> for these. Can you ask the owner of the code if they expect meaningful
> user-visible/script-visible errors to be reported to the DOMWindow here?

Sure, would this just be if the float array (in InitializeBuffers) failed to create?
That's the only thing that seems to be happening with the context in the webaudio stuff.

Comment 33

[Bobby Holley (:bholley)]

(In reply to Bob Owen (:bobowen) from comment #32)
> (In reply to Bobby Holley (:bholley) from comment #31)
> > Hm, I actually wonder if we perhaps want AutoJSAPIWithErrorsReportedToWindow
> > for these. Can you ask the owner of the code if they expect meaningful
> > user-visible/script-visible errors to be reported to the DOMWindow here?
> 
> Sure, would this just be if the float array (in InitializeBuffers) failed to
> create?
> That's the only thing that seems to be happening with the context in the
> webaudio stuff.

Ok yeah. Technically the user might miss out on an OOM message to their console, but I think we can just ignore this issue for now.

Comment 34

[Bob Owen (:bobowen)]

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

> > Sure, would this just be if the float array (in InitializeBuffers) failed to
> > create?
> > That's the only thing that seems to be happening with the context in the
> > webaudio stuff.
> 
> Ok yeah. Technically the user might miss out on an OOM message to their
> console, but I think we can just ignore this issue for now.

Just noticed that the ScriptProcessorNode code calls |GetThreadSharedChannelsForRate(cx)| further down, don't no how I missed that before.

This has got various bits of error reporting in it, so I think this one should be AutoJSAPIWithErrorsReportedToWindow.

You obviously have a sixth sense. :-)
I'll fix up the patch.

Comment 35

[Bob Owen (:bobowen)]

Attachment: Part 8: Replace AutoPushJSContext in ScriptProcessorNode - Command::Run v3

Ironically the patch for which I introduced AudioContext::GetGlobalJSObject, now doesn't use it and has to do even more dancing.
I might be being a bit paranoid about that last null check, but it's getting late.

Here's a try push with this patch:
https://tbpl.mozilla.org/?tree=Try&rev=aa7783b40169

I'll r? when everything looks OK.

Oh, and s/don't no how/don't know how/ on comment 34.
A sure sign it's time for me to go and fall asleep in front of a film. :)

Comment 36

[Bob Owen (:bobowen)]

Attachment: Part 8 diff with -b option, to show the patch without the indentation changes.

This is just a an |hg qdiff -b| of patch Part 8, so that you can see the changes without it being clouded by the indentation change.

It would be good if bugzilla showed a diff that was more similar to something like kdiff.

Comment 37

[Bob Owen (:bobowen)]

Comment on attachment 8420371 [details] [diff] [review]
Part 8: Replace AutoPushJSContext in ScriptProcessorNode - Command::Run v3

The try push looks good.

I've also uploaded a diff without the indentation changes:
https://bugzilla.mozilla.org/attachment.cgi?id=8420517

Comment 38

[Bobby Holley (:bholley)]

(In reply to Bob Owen (:bobowen) from comment #34)
> Just noticed that the ScriptProcessorNode code calls
> |GetThreadSharedChannelsForRate(cx)| further down, don't no how I missed
> that before.
> 
> This has got various bits of error reporting in it, so I think this one
> should be AutoJSAPIWithErrorsReportedToWindow.

Hm, which bits of error reporting? All I see is some TypedArray manipulation that shouldn't really fail. Did I miss something?

Basically, the bar should be: Do we expect users or scripts to hit these failures by doing something that they could correct, and would the error messages thrown help them figure out what went wrong?

In the future, we'll hopefully do a better job of making sure that all exceptions eventually reach a place where the user can find them. But for the time being, the boilerplate to retrieve the DOM window is nontrivial, so we shouldn't clutter the code with it unless we know we need it.
 
> You obviously have a sixth sense. :-)

Maybe not. :-)

Comment 39

[Bob Owen (:bobowen)]

Comment on attachment 8420371 [details] [diff] [review]
Part 8: Replace AutoPushJSContext in ScriptProcessorNode - Command::Run v3

(In reply to Bobby Holley (:bholley) from comment #38)
> (In reply to Bob Owen (:bobowen) from comment #34)

> > This has got various bits of error reporting in it, so I think this one
> > should be AutoJSAPIWithErrorsReportedToWindow.
> 
> Hm, which bits of error reporting? All I see is some TypedArray manipulation
> that shouldn't really fail. Did I miss something?
> 
> Basically, the bar should be: Do we expect users or scripts to hit these
> failures by doing something that they could correct, and would the error
> messages thrown help them figure out what went wrong?

They are in JS_StealArrayBufferContents and deeper down.
On closer inspection, you're right, the way in which the functions are being called means the checks never fail, sorry.
The only one I think we could hit is in AllocateArrayBufferContents, but this is another OOM.

I'll re-instate the previous patch and do a fairly full try push.
Although that patch queue in on my Linux boot, so it'll be a bit later.

Comment 40

[Bob Owen (:bobowen)]

Comment on attachment 8420274 [details] [diff] [review]
Part 8: Replace AutoPushJSContext in ScriptProcessorNode - Command::Run v2

Re-instating patch see comment 39.

Almost full try push:
https://tbpl.mozilla.org/?tree=Try&rev=0df0226c82ca

Comment 41

[Bob Owen (:bobowen)]

All green after a handful of retries on the try push.

Landing in Part number order for the patches.
Thanks.

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e4cb79ab1bbf
https://hg.mozilla.org/integration/mozilla-inbound/rev/d83f4b24777c
https://hg.mozilla.org/integration/mozilla-inbound/rev/678a381b9399
https://hg.mozilla.org/integration/mozilla-inbound/rev/87f96941ad50
https://hg.mozilla.org/integration/mozilla-inbound/rev/d53d3bb5d7e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/67bff024b230
https://hg.mozilla.org/integration/mozilla-inbound/rev/29146acaae75
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4b6c12f4f6e
https://hg.mozilla.org/integration/mozilla-inbound/rev/1497517ec644
https://hg.mozilla.org/integration/mozilla-inbound/rev/03077242af73

Comment 43

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e4cb79ab1bbf
https://hg.mozilla.org/mozilla-central/rev/d83f4b24777c
https://hg.mozilla.org/mozilla-central/rev/678a381b9399
https://hg.mozilla.org/mozilla-central/rev/87f96941ad50
https://hg.mozilla.org/mozilla-central/rev/d53d3bb5d7e8
https://hg.mozilla.org/mozilla-central/rev/67bff024b230
https://hg.mozilla.org/mozilla-central/rev/29146acaae75
https://hg.mozilla.org/mozilla-central/rev/b4b6c12f4f6e
https://hg.mozilla.org/mozilla-central/rev/1497517ec644
https://hg.mozilla.org/mozilla-central/rev/03077242af73