Closed
Bug 1006024
Opened 11 years ago
Closed 11 years ago
Change usages of nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI and derivatives for bug 951991 - batch 3
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: bobowen, Assigned: bobowen)
References
Details
Attachments
(10 files, 9 obsolete files)
(deleted),
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bobowen
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bobowen
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
Another batch of patches moving nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI etc.
See bug 951991 for details.
Comment 1•11 years ago
|
||
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.
Assignee | ||
Comment 2•11 years ago
|
||
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 | ||
Updated•11 years ago
|
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
Comment 3•11 years ago
|
||
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+
Assignee | ||
Comment 4•11 years ago
|
||
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 5•11 years ago
|
||
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+
Assignee | ||
Comment 6•11 years ago
|
||
(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)
Comment 7•11 years ago
|
||
(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!
Updated•11 years ago
|
Attachment #8418143 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 8•11 years ago
|
||
Follow up to patch Part 6 from bug 988383.
Attachment #8418661 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 9•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #8418661 -
Flags: review?(bobbyholley) → review+
Comment 10•11 years ago
|
||
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+
Assignee | ||
Comment 11•11 years ago
|
||
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•11 years ago
|
||
(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.
Assignee | ||
Comment 13•11 years ago
|
||
(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.
Assignee | ||
Comment 14•11 years ago
|
||
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+
Assignee | ||
Comment 15•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #8418764 -
Attachment is obsolete: true
Assignee | ||
Comment 16•11 years ago
|
||
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)
Assignee | ||
Comment 17•11 years ago
|
||
Pretty much identical to Part 5.
Attachment #8419998 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 18•11 years ago
|
||
Again very similar, just some unindenting because of the early return.
Attachment #8420006 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 19•11 years ago
|
||
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)
Assignee | ||
Comment 20•11 years ago
|
||
(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)
Comment 21•11 years ago
|
||
(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 22•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #8419995 -
Flags: review?(bobbyholley)
Updated•11 years ago
|
Attachment #8419998 -
Flags: review?(bobbyholley)
Updated•11 years ago
|
Attachment #8420006 -
Flags: review?(bobbyholley)
Updated•11 years ago
|
Attachment #8420014 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 23•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #8419953 -
Attachment is obsolete: true
Comment 24•11 years ago
|
||
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+
Assignee | ||
Comment 25•11 years ago
|
||
r=bholley from comment 24 with function renamed to GetGlobalJSObject.
Attachment #8420242 -
Attachment is obsolete: true
Attachment #8420266 -
Flags: review+
Assignee | ||
Comment 26•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #8419995 -
Attachment is obsolete: true
Assignee | ||
Comment 27•11 years ago
|
||
I have a strange sense of déjà vu.
Attachment #8420272 -
Flags: review?(bobbyholley)
Assignee | ||
Updated•11 years ago
|
Attachment #8419998 -
Attachment is obsolete: true
Assignee | ||
Comment 28•11 years ago
|
||
Same again except the early return means we can unindent the code below.
Attachment #8420274 -
Flags: review?(bobbyholley)
Assignee | ||
Updated•11 years ago
|
Attachment #8420006 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8420014 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 29•11 years ago
|
||
And we say goodbye to some more JSContext etc. machinery.
Attachment #8420280 -
Flags: review?(bobbyholley)
Updated•11 years ago
|
Attachment #8420271 -
Flags: review?(bobbyholley) → review+
Updated•11 years ago
|
Attachment #8420014 -
Flags: review?(bobbyholley) → review+
Updated•11 years ago
|
Attachment #8420272 -
Flags: review?(bobbyholley) → review+
Updated•11 years ago
|
Attachment #8420280 -
Flags: review?(bobbyholley) → review+
Comment 30•11 years ago
|
||
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+
Comment 31•11 years ago
|
||
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?
Assignee | ||
Comment 32•11 years ago
|
||
(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•11 years ago
|
||
(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.
Assignee | ||
Comment 34•11 years ago
|
||
(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.
Assignee | ||
Comment 35•11 years ago
|
||
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. :)
Assignee | ||
Updated•11 years ago
|
Attachment #8420274 -
Attachment is obsolete: true
Assignee | ||
Comment 36•11 years ago
|
||
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.
Assignee | ||
Comment 37•11 years ago
|
||
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)
Comment 38•11 years ago
|
||
(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. :-)
Updated•11 years ago
|
Attachment #8420371 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 39•11 years ago
|
||
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
Assignee | ||
Updated•11 years ago
|
Attachment #8420517 -
Attachment is obsolete: true
Assignee | ||
Comment 40•11 years ago
|
||
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
Assignee | ||
Comment 41•11 years ago
|
||
All green after a handful of retries on the try push.
Landing in Part number order for the patches.
Thanks.
Keywords: checkin-needed
Comment 42•11 years ago
|
||
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
Keywords: checkin-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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•