1023969 - Change usages of nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI and derivatives for bug 951991 - batch 6 ... Bluetooth's Revenge

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

[Bob Owen (:bobowen)]

Attachment: Part 1: Replace AutoPushJSContext in BluetoothAdapter GetDevicesTask::ParseSuccessfulReply v1

I don't think that nsTArrayToJSArray needs anything more than an AutoJSAPI.
The JS_SetElement in it is on a new array, so I don't think that can cause problems and I don't think JS_FreezeObject can cause script to be run.
Also, the only errors would be OOM, I believe.

I've put the null check on |mAdapterPtr->GetOwner()| to the start of the function as BluetoothDevice::Create will assert if it is null and there is a similar check in BluetoothManager (see Part 3 to be added shortly).

Comment 2

[Bob Owen (:bobowen)]

Attachment: Part 2: Replace |AutoPushJSContext|s in BluetoothAdapter::SetPropertyByValue v1

This is pretty similar to the part 1.

I added a file scope function to get the Global JS Object.

I seem to have written this code (or what could have and perhaps should have been this code) a number of times for EventTargets.
I was wondering if it would be worth adding the following to DOMEventTargetHelper.h:

JSObject* GetGlobalJSObjectForEventHandlers() const {
  return mOwnerWindow ? static_cast<nsGlobalWindow*>(mOwnerWindow)->GetWrapperPreserveColor()
                      : nullptr;
}

or maybe even add it to nsIDOMEventTarget.idl

Comment 3

[Bob Owen (:bobowen)]

Comment on attachment 8439319 [details] [diff] [review]
Part 2: Replace |AutoPushJSContext|s in BluetoothAdapter::SetPropertyByValue v1

I've got compile issues on B2G try push so I'll cancel for now.

Comment 4

[Bob Owen (:bobowen)]

Hmm, seems like try is still a bit the worse for wear.

What do you think about the idea, of adding something to get the Global JSObject to DOMEventTargetHelper or nsIDOMEventTarget.idl from comment 2?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

We have already
JSContextPtr GetJSContextForEventHandlers();
in nsIDOMEventTarget, so perhaps adding similar for global wouldn't be so bad.
(although I still think any use of JSAPI in DOM code is effectively a bug.)

Comment 6

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #5)
> We have already
> JSContextPtr GetJSContextForEventHandlers();
> in nsIDOMEventTarget, so perhaps adding similar for global wouldn't be so
> bad.

Yeah, it looks like a lot of existing consumers use this stuff, so having something similar for the global would reduce the impedence mismatch of switching away from JSContexts.

> (although I still think any use of JSAPI in DOM code is effectively a bug.)

What would your ideal API look like here?

Comment 7

[Bob Owen (:bobowen)]

Attachment: Part 1: Replace AutoPushJSContext in BluetoothAdapter GetDevicesTask::ParseSuccessfulReply v1 take 2

Now that bug 1025476 is well on its way, I'll crack on with these patches.

I don't think that nsTArrayToJSArray needs anything more than an AutoJSAPI.
The JS_SetElement in it is on a new array, so I don't think that can cause problems and I don't think JS_FreezeObject can cause script to be run.
Also, the only errors would be OOM, I believe.

Comment 8

[Bob Owen (:bobowen)]

Attachment: Part 2: Replace |AutoPushJSContext|s in BluetoothAdapter::SetPropertyByValue v1 take 2

Wow, that was quick.

These are almost exactly the same as Part 1.

Comment 9

[Bob Owen (:bobowen)]

Attachment: Part 3: Replace AutoPushJSContext in BluetoothManager GetAdapterTask::ParseSuccessfulReply v1

Just a WrapNative here, which we've already decided just needs the relevant compartment.

All the other Bluetooth instances are very similar to Parts 1 - 3, so I thought I'd get the OK for these first.

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

Comment 10

[Bob Owen (:bobowen)]

Attachment: Part 4: Replace |AutoPushJSContext|s in BluetoothDevice::SetPropertyByValue v1

Almost identical to Part 2.

Comment 11

[Bob Owen (:bobowen)]

Attachment: Part 5: Replace AutoPushJSContext in bluetooth2 BluetoothAdapter GetDevicesTask::ParseSuccessfulReply v1

Identical to Part 1, except for bluetooth2.

Comment 12

[Bob Owen (:bobowen)]

Attachment: Part 6: Replace |AutoPushJSContext|s in bluetooth2 BluetoothAdapter::SetPropertyByValue v1

Bluetooth2 version of Part 2.

Comment 13

[Bob Owen (:bobowen)]

Attachment: Part 7: Replace |AutoPushJSContext|s in bluetooth2 BluetoothDevice::SetPropertyByValue v1

Bluetooth2 version of Part 4.

Comment 14

[Bob Owen (:bobowen)]

Try push with all patches on top of patch for bug 1025476:
https://tbpl.mozilla.org/?tree=Try&rev=60210f571268

11 AutoPushJSContexts for the price of 7. :-)

Comment 15

[Bob Owen (:bobowen)]

This can go now that bug 1025476 is in m-c.

Try push from below for easy reference:
https://tbpl.mozilla.org/?tree=Try&rev=60210f571268

Landing in part number order.
Thanks.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ced403a64ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/377e24bd3ce0
https://hg.mozilla.org/integration/mozilla-inbound/rev/120937b0d7c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/c56c213d00b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/f42162a34a85
https://hg.mozilla.org/integration/mozilla-inbound/rev/9be7558938f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/72c2544c167d

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/8ced403a64ac
https://hg.mozilla.org/mozilla-central/rev/377e24bd3ce0
https://hg.mozilla.org/mozilla-central/rev/120937b0d7c1
https://hg.mozilla.org/mozilla-central/rev/c56c213d00b3
https://hg.mozilla.org/mozilla-central/rev/f42162a34a85
https://hg.mozilla.org/mozilla-central/rev/9be7558938f9
https://hg.mozilla.org/mozilla-central/rev/72c2544c167d