Closed Bug 1006024 Opened 6 years ago Closed 6 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: bobowen, Assigned: bobowen)

References

Details

Attachments

(10 files, 9 obsolete files)

1.73 KB, patch
bholley
: review+
Details | Diff | Splinter Review
1.17 KB, patch
bholley
: review+
Details | Diff | Splinter Review
1.34 KB, patch
bholley
: review+
Details | Diff | Splinter Review
2.52 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
5.11 KB, patch
bholley
: review+
Details | Diff | Splinter Review
3.85 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
1.99 KB, patch
bholley
: review+
Details | Diff | Splinter Review
2.34 KB, patch
bholley
: review+
Details | Diff | Splinter Review
5.25 KB, patch
bholley
: review+
Details | Diff | Splinter Review
1.98 KB, patch
bholley
: review+
Details | Diff | Splinter Review
Another batch of patches moving nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI etc.
See bug 951991 for details.
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.
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.
Attachment #8417651 - Flags: review?(bobbyholley)
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
Comment on attachment 8417651 [details] [diff] [review]
Part 1: Replace AutoPushJSContext in EventSource::DispatchAllMessageEvents v1

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

Nice!
Attachment #8417651 - Flags: review?(bobbyholley) → review+
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.
Attachment #8418123 - Flags: review?(bobbyholley)
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?
Attachment #8418123 - Flags: review?(bobbyholley) → review+
(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?
Attachment #8418143 - Flags: review?(bobbyholley)
(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!
Attachment #8418143 - Flags: review?(bobbyholley) → review+
Follow up to patch Part 6  from bug 988383.
Attachment #8418661 - Flags: review?(bobbyholley)
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?
Attachment #8418764 - Flags: review?(bobbyholley)
Attachment #8418661 - Flags: review?(bobbyholley) → review+
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.
Attachment #8418764 - Flags: review?(bobbyholley) → review+
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
(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.
(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.
r=bholley from comment 3.

Just realised I could remove the nsCxPusher.h include.
Also a one word change to the comment.
Attachment #8417651 - Attachment is obsolete: true
Attachment #8419946 - Flags: review+
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.
Attachment #8419953 - Flags: review?(bobbyholley)
Attachment #8418764 - Attachment is obsolete: true
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.
Attachment #8419995 - Flags: review?(bobbyholley)
Pretty much identical to Part 5.
Attachment #8419998 - Flags: review?(bobbyholley)
Again very similar, just some unindenting because of the early return.
Attachment #8420006 - Flags: review?(bobbyholley)
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
Attachment #8420014 - Flags: review?(bobbyholley)
(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?
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
Comment on attachment 8419953 [details] [diff] [review]
Part 5: Replace AutoPushJSContext in AudioDestinationNode FireOfflineCompletionEvent v2

Please reflag me when these are ready.
Attachment #8419953 - Flags: review?(bobbyholley)
Attachment #8419995 - Flags: review?(bobbyholley)
Attachment #8419998 - Flags: review?(bobbyholley)
Attachment #8420006 - Flags: review?(bobbyholley)
Attachment #8420014 - Flags: review?(bobbyholley)
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
Attachment #8420242 - Flags: review?(bobbyholley)
Attachment #8419953 - Attachment is obsolete: true
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().
Attachment #8420242 - Flags: review?(bobbyholley) → review+
r=bholley from comment 24 with function renamed to GetGlobalJSObject.
Attachment #8420242 - Attachment is obsolete: true
Attachment #8420266 - Flags: review+
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.
Attachment #8420271 - Flags: review?(bobbyholley)
Attachment #8419995 - Attachment is obsolete: true
I have a strange sense of déjà vu.
Attachment #8420272 - Flags: review?(bobbyholley)
Attachment #8419998 - Attachment is obsolete: true
Same again except the early return means we can unindent the code below.
Attachment #8420274 - Flags: review?(bobbyholley)
Attachment #8420006 - Attachment is obsolete: true
Attachment #8420014 - Flags: review?(bobbyholley)
And we say goodbye to some more JSContext etc. machinery.
Attachment #8420280 - Flags: review?(bobbyholley)
Attachment #8420271 - Flags: review?(bobbyholley) → review+
Attachment #8420014 - Flags: review?(bobbyholley) → review+
Attachment #8420272 - Flags: review?(bobbyholley) → review+
Attachment #8420280 - Flags: review?(bobbyholley) → review+
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.
Attachment #8420274 - Flags: review?(bobbyholley) → review+
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?
(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.
(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.
(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.
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. :)
Attachment #8420274 - Attachment is obsolete: true
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 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
Attachment #8420371 - Flags: review?(bobbyholley)
(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. :-)
Attachment #8420371 - Flags: review?(bobbyholley)
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.
Attachment #8420371 - Attachment is obsolete: true
Attachment #8420517 - Attachment is obsolete: true
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
Attachment #8420274 - Attachment is obsolete: false
All green after a handful of retries on the try push.

Landing in Part number order for the patches.
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.