Closed Bug 871064 Opened 12 years ago Closed 12 years ago

_PR_InitThreads() should not call PR_SetThreadPriority()

Categories

(NSPR :: NSPR, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wtc, Assigned: wtc)

Details

Attachments

(1 file)

Attached patch PatchSplinter Review
_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.
Attachment #748322 - Flags: review?(gsvelto)
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
Attachment #748322 - Flags: review?(gsvelto) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: