1693762 - Mac thread names should not be limited to 15 characters

Status: VERIFIED FIXED

(NSPR :: NSPR, defect)

Description

[Florian Quèze [:florian]]

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).

Comment 1

[Florian Quèze [:florian]]

Attachment: Bug 1693762 - Extend the thread name length limit to 63 characters on Mac, r=kaie.

Comment 2

[Kai Engert (:KaiE:)]

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.

Comment 3

[Kai Engert (:KaiE:)]

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

Comment 4

[Kai Engert (:KaiE:)]

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

Comment 5

[Kai Engert (:KaiE:)]

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.

Comment 6

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/projects/nspr/rev/cbbfa71c7e4a155e62732a817f3a3d5c3d9647a9

Comment 7

[Florian Quèze [:florian]]

(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.

Comment 8

[Florian Quèze [:florian]]

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