Closed
Bug 1023969
Opened 10 years ago
Closed 10 years ago
Change usages of nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI and derivatives for bug 951991 - batch 6 ... Bluetooth's Revenge
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: bobowen, Assigned: bobowen)
References
Details
Attachments
(7 files, 2 obsolete files)
Part 1: Replace AutoPushJSContext in BluetoothAdapter GetDevicesTask::ParseSuccessfulReply v1 take 2
(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 |
(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.
Assignee | ||
Comment 1•10 years ago
|
||
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).
Attachment #8439310 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 2•10 years ago
|
||
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
Attachment #8439319 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 3•10 years ago
|
||
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.
Attachment #8439319 -
Flags: review?(bobbyholley)
Assignee | ||
Updated•10 years ago
|
Attachment #8439310 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 4•10 years ago
|
||
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?
Flags: needinfo?(bobbyholley)
Comment 5•10 years ago
|
||
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•10 years ago
|
||
(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?
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 7•10 years ago
|
||
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.
Attachment #8442979 -
Flags: review?(bobbyholley)
Assignee | ||
Updated•10 years ago
|
Attachment #8439310 -
Attachment is obsolete: true
Updated•10 years ago
|
Attachment #8442979 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 8•10 years ago
|
||
Wow, that was quick. These are almost exactly the same as Part 1.
Attachment #8442984 -
Flags: review?(bobbyholley)
Assignee | ||
Updated•10 years ago
|
Attachment #8439319 -
Attachment is obsolete: true
Assignee | ||
Comment 9•10 years ago
|
||
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
Attachment #8442987 -
Flags: review?(bobbyholley)
Updated•10 years ago
|
Attachment #8442984 -
Flags: review?(bobbyholley) → review+
Updated•10 years ago
|
Attachment #8442987 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 10•10 years ago
|
||
Almost identical to Part 2.
Attachment #8443971 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 11•10 years ago
|
||
Identical to Part 1, except for bluetooth2.
Attachment #8443973 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 12•10 years ago
|
||
Bluetooth2 version of Part 2.
Attachment #8443974 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 13•10 years ago
|
||
Bluetooth2 version of Part 4.
Attachment #8443976 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 14•10 years ago
|
||
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. :-)
Updated•10 years ago
|
Attachment #8443971 -
Flags: review?(bobbyholley) → review+
Updated•10 years ago
|
Attachment #8443973 -
Flags: review?(bobbyholley) → review+
Updated•10 years ago
|
Attachment #8443974 -
Flags: review?(bobbyholley) → review+
Updated•10 years ago
|
Attachment #8443976 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 15•10 years ago
|
||
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.
Keywords: checkin-needed
Comment 16•10 years ago
|
||
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
Keywords: checkin-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
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Updated•10 years ago
|
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
•