Closed
Bug 1017030
Opened 10 years ago
Closed 10 years ago
Change usages of nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI and derivatives for bug 951991 - batch 5
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: bobowen, Assigned: bobowen)
References
Details
Attachments
(6 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
|
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'm using similar reasoning to bug 988383 comment 27 here. In that, any meaningful errors would have happened during the serialisation in PreDispatch. I notice that the context passed to PreDispatch isn't pushed, but we're OK as that's guaranteed to be off main thread. I also noticed that I think we could get rid of the |mWorkerPrivate->AssertIsOnWorkerThread();| in ConsoleRunnable::Dispatch as |WorkerPrivate::GetJSContext()| already calls it.
Attachment #8438508 -
Flags: review?(bobbyholley)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•10 years ago
|
||
This is very similar to part 1, except for removing the way it used to get to the global for the JSAutoCompartment.
Attachment #8438511 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 3•10 years ago
|
||
This one is even closer to bug 988383 part 4.
Attachment #8438517 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 4•10 years ago
|
||
I'm fairly sure that because of what we are calling JS_DefineFunctions with here that it pretty much just drops down to the bottom of the function and calls DefineFunction and I don't think that cares what context it gets.
Attachment #8438527 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 5•10 years ago
|
||
Looks like the JSON parsing can report errors, although I'm not sure if aMessage might be guaranteed to be error free.
Attachment #8438532 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 6•10 years ago
|
||
As far as I can tell the only errors we could get from StringToJsval are OOM ones. And this was the patch that prompted my question about JSObject*. :-)
Attachment #8438535 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 7•10 years ago
|
||
Here's an old try push (I accidentally merged that last two patches): https://tbpl.mozilla.org/?tree=Try&rev=9d6fd600eb5c
Updated•10 years ago
|
Attachment #8438508 -
Flags: review?(bobbyholley) → review+
Updated•10 years ago
|
Attachment #8438511 -
Flags: review?(bobbyholley) → review+
Comment 8•10 years ago
|
||
Comment on attachment 8438517 [details] [diff] [review] Part 3: Replace AutoPushJSContext in PostMessageRunnable::Run v1 Review of attachment 8438517 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/MessagePort.cpp @@ +279,4 @@ > > + AutoJSAPI jsapi; > + JSContext* cx = jsapi.cx(); > + JSAutoCompartment ac(cx, globalObject->GetGlobalJSObject()); Note that in the non-window case, this switches us from using the SafeJSContext global to using the actual global. I think that's the right way to go, though.
Attachment #8438517 -
Flags: review?(bobbyholley) → review+
Updated•10 years ago
|
Attachment #8438527 -
Flags: review?(bobbyholley) → review+
Updated•10 years ago
|
Attachment #8438532 -
Flags: review?(bobbyholley) → review+
Comment 9•10 years ago
|
||
Comment on attachment 8438535 [details] [diff] [review] Part 6: Replace AutoPushJSContext in nsDeviceStorage StringToJsval v1 Review of attachment 8438535 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/devicestorage/nsDeviceStorage.cpp @@ +1703,4 @@ > return JSVAL_NULL; > } > > + AutoJSAPI jsapi; The fact that the AutoJSAPI here exists between the naked JSObject* and the JSAutoCompartment might be flagged as a rooting hazard by the analysis (run browser rooting analysis in your trychooser syntax to see). sfink was looking at this exact issue, so you should talk to him and see where he is about it. Just reordering and putting the AutoJSAPI at the top is probably enough to fix this though.
Attachment #8438535 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #9) > Comment on attachment 8438535 [details] [diff] [review] > The fact that the AutoJSAPI here exists between the naked JSObject* and the > JSAutoCompartment might be flagged as a rooting hazard by the analysis (run > browser rooting analysis in your trychooser syntax to see). > > sfink was looking at this exact issue, so you should talk to him and see > where he is about it. Just reordering and putting the AutoJSAPI at the top > is probably enough to fix this though. Ah, OK, I'll kick one off once the trees are open. Wouldn't it be nice if the Global JSObject had it's own derived type? It might help in situations like this and maybe it would have other uses. Thanks for all the reviews. Might be tomorrow before I get those first fixed Bluetooth patches up for bug 1023969, as I can't test my fixes on try and I've never tried building B2G locally.
Assignee | ||
Comment 11•10 years ago
|
||
try push including the rooting analysis looks good: https://tbpl.mozilla.org/?tree=Try&rev=dc2bc024f53d Landing in part number order please. Thanks.
Keywords: checkin-needed
Comment 12•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/75855f82feae https://hg.mozilla.org/integration/mozilla-inbound/rev/43e89a29e47b https://hg.mozilla.org/integration/mozilla-inbound/rev/8486fe4272ce https://hg.mozilla.org/integration/mozilla-inbound/rev/ed77ac7d2c62 https://hg.mozilla.org/integration/mozilla-inbound/rev/b64357012f74 https://hg.mozilla.org/integration/mozilla-inbound/rev/6d572d9513ce
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/75855f82feae https://hg.mozilla.org/mozilla-central/rev/43e89a29e47b https://hg.mozilla.org/mozilla-central/rev/8486fe4272ce https://hg.mozilla.org/mozilla-central/rev/ed77ac7d2c62 https://hg.mozilla.org/mozilla-central/rev/b64357012f74 https://hg.mozilla.org/mozilla-central/rev/6d572d9513ce
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
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
•