Closed Bug 1693762 Opened 1 year ago Closed 1 year ago

Mac thread names should not be limited to 15 characters

Categories

(NSPR :: NSPR, defect)

defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: florian, Assigned: florian)

References

Details

Attachments

(1 obsolete file)

The code at https://searchfox.org/mozilla-central/rev/281009d3b7e1e1496b9524d5478ff4f0b7369573/ipc/chromium/src/base/platform_thread_mac.mm#63-65 says the actual limit on Mac is 63 characters. The code in PR_SetCurrentThreadName should match this behavior. The 15 character limit appears to be correct for Linux.

Currently we see truncated thread names in about:processes (with toolkit.aboutProcesses.showThreads set to true in about:config).

Assignee: nobody → florian
Status: NEW → ASSIGNED
See Also: → 1694186

Note that changes to NSPR cannot be directly made in mozilla-central.
We shouldn't use phabricator with mozilla-central, but with https://hg.mozilla.org/projects/nspr

Because NSPR is maintained and released as a separate project, it will be necessary to commit the change to the NSPR repository (I can do that, once reviewed).

We don't take individual patches to NSPR in mozilla-central. A new NSPR release must be planned for the change to be picked up. I will handle that, too.

The change looks good to me. We don't need a new phab patch, I can land it manually into NSPR.

Blocks: 1694371

r=kaie for the code contents of the patch (but not for a patch against mozilla-central)

Florian, what's your plan for testing that the code works as expected?

I need to add a testing tag when uplifting the new NSPR to m-c.
I can use testing-exception-elsewhere, but assume that you'll help to test that this change in NSPR is still working for you.

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 4.30

(In reply to Kai Engert (:KaiE:) from comment #5)

Florian, what's your plan for testing that the code works as expected?

It will be visible in about:processes, and I don't see how this could regress, so I don't think adding automated tests is worth the effort. But thanks for asking!

https://phabricator.services.mozilla.com/D105990 is a similar change made for the threading code of the JS engine, we landed with "testing-exception-other: this difference is visible to users of the profiler, but I don't think it's worth testing.".

https://phabricator.services.mozilla.com/D106417 (adding thread names to specific threads) landed with "testing-exception-other: The code was tested manually, it can't fail or regress in any meaningful way so it does not warrant dedicated tests."

I think it should be fine to copy this for the testing tag of your bug.

Attachment #9204170 - Attachment is obsolete: true

I verified in today's Nightly that the change works as expected.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.