871064 - _PR_InitThreads() should not call PR_SetThreadPriority()

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P2)

Description

[Wan-Teh Chang]

Attachment: Patch

_PR_InitThreads() is an internal function called during NSPR
initialization. _PR_InitThreads() initializes the threading
component and creates the PRThread structure for the calling
thread.

_PR_InitThreads() (in nspr/pr/src/pthreads/ptthread.c) calls
PR_SetThreadPriority() at the end.  Since NSPR is implicitly
initialized by the first NSPR function call, it doesn't make
sense to change the priority of the calling thread.

Note: the other _PR_InitThreads() implementation (in
nspr/pr/src/threads/combined/pruthr.c, used on Windows), does
not change the priority of the calling thread.

In the patch I also assert that the |priority| argument is
PR_PRIORITY_NORMAL. This is true of the only caller of
_PR_InitThreads: http://mxr.mozilla.org/nspr/ident?i=_PR_InitThreads

So the assertions should help keep it that way. We probably
should just remove the |priority| argument from _PR_InitThreads.

Comment 1

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 748322 [details] [diff] [review]
Patch

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

Looks good to me and should finally make the behavior consistent across all POSIX platforms. Do we want to change the Be support too? I think Firefox runs on Haiku too but I'm not sure if they're still using the existing BeOS code or if they have their own port. The relevant code is here anyway:

https://hg.mozilla.org/projects/nspr/file/8927f44164de/pr/src/bthreads/btthread.c#l92

Comment 2

[Wan-Teh Chang]

Gabriele:

Thanks a lot for checking all implementations of _PR_InitThreads.

I believe we no longer support BeOS. I haven't touched the BeOS
code for at least two years.

Patch pushed: https://hg.mozilla.org/projects/nspr/rev/8a9fc0355b09

Comment 3

[Mike Kaply [:mkaply]]

TL;DR - This bug fixed a Firefox crasher on Yosemite over a year before Yosemite was released.

I work on Postbox which is based on old Mozilla source code. We starting seeing a crasher when you open the file dialog and click on "Photos". I tested Firefox and found that it crashed as well before FF23.

I narrowed the regression range down to:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=00b264c7cced&tochange=97aa3da59001

After much building and testing, we discovered that it was this patch that fixed things.

Somehow the thread priority was screwing with the Mac file dialog in this case.

Anyway, just thought I would post here because it was interesting.