Closed Bug 1371136 Opened 2 years ago Closed 2 years ago

Don't use global context in performance monitoring service

Categories

(Toolkit :: Performance Monitoring, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: billm, Assigned: billm)

Details

Attachments

(1 file)

Attached patch patchSplinter Review
This is another case where a JSContext is getting stored globally. I changed the code to use AutoJSAPI instead. There's also code in GenerateUniqueGroupId that was using the JSContext as a kind of "thread ID". I think that using mozilla::GetCurrentPhysicalThread() should serve just as well. See here for a description of that function:
http://searchfox.org/mozilla-central/rev/1a054419976437d0778a2b89be1b00207a744e94/xpcom/threads/nsThreadUtils.h#1790-1825
Attachment #8875559 - Flags: review?(continuation)
Comment on attachment 8875559 [details] [diff] [review]
patch

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

::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp
@@ +126,5 @@
>   */
>  void
> +GenerateUniqueGroupId(uint64_t uid, uint64_t processId, nsAString& groupId)
> +{
> +  uint64_t threadId = reinterpret_cast<uint64_t>(GetCurrentPhysicalThread());

I suspect that this will be a bit slower (apparently, `GetCurrentPhysicalThread()` needs access to local storage). However, since this is called at most once per context (used to be twice) in the entire life of the context, this should not matter.

Could you mention this in your commit message?
Attachment #8875559 - Flags: feedback+
Attachment #8875559 - Flags: review?(continuation) → review+
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/23ad1168311b
Use contexts more safely in performance monitoring service (r=mccr8)
https://hg.mozilla.org/mozilla-central/rev/23ad1168311b
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.