|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
58 bytes, text/x-review-board-request
|Details | Review|
1.28 KB, patch
|Details | Diff | Splinter Review|
Created attachment 763387 [details] [diff] [review] remove nsIEntropyCollector and its implementation nsIEntropyCollector seems unnecessary given the current implementation of NSS on modern operating systems. The idea of it is to mix additional entropy from system events. The current implementation in Gecko uses mouse movement events to add entropy. Obviously, this only works on systems that have a mounse--i.e. not mobile platforms. We're already relying on the OS and NSS to give us good randomness without mouse movement events on mobile, those events must not be needed for desktop either. Less obviously, if one wanted to build a content/chrome-separated browser in Gecko with PSM (only) in the chrome process, then this communication channel would be an unnecessarily burdensome complication to deal with. This would also simplify some DOM and PSM code to be friendlier to newcomers. I did not measure whether there is any positive performance impact with this change, with respect to mouse movement tracking performance. Counter-arguments about why this change is not a good idea are also appreciated.
Comment on attachment 763387 [details] [diff] [review] remove nsIEntropyCollector and its implementation The removal of security/manager/ssl/src/PSMContentDownloader.h looks unrelated to this change. Also, that file is not in m-c, it seems. r=jst for the rest.
Attachment #763387 - Flags: review?(jst) → review+
Comment on attachment 763387 [details] [diff] [review] remove nsIEntropyCollector and its implementation I think we need to replace this code with periodic calls to PK11_RandomUpdate() with random bytes from /dev/urandom or RtlGenRandom.
Assignee: brian → nobody
Mass tracking-e10s flag change. Filter bugmail on "2be0fcce-e36a-4e2c-aa80-0e3d33eb5406".
tracking-e10s: --- → +
Status: ASSIGNED → NEW
Duplicate of this bug: 1263015
Created attachment 8751028 [details] MozReview Request: bug 883718 - remove nsIEntropyCollector and implementation r?mounir r?mgoodwin The rationale behind nsIEntropyCollector was to supplement NSS' source of entropy with randomness from mouse move events. This obviously doesn't work on platforms without a mouse (e.g. mobile platforms). Furthermore, as NSS seeds its random number generator with robust randomness from the operating system, this is unnecessary anyway. The primary concern is that initialization of the random number generator must happen after forking, which is exactly what we do with the child process in e10s mode. Review commit: https://reviewboard.mozilla.org/r/51769/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/51769/
Attachment #763387 - Attachment is obsolete: true
Whiteboard: [psm-backlog] → [psm-assigned]
I see there is a new fancy review board :) r+ though I'm not sure I'm the right person to review this :)
Comment on attachment 8751028 [details] MozReview Request: bug 883718 - remove nsIEntropyCollector and implementation r?mounir r?mgoodwin https://reviewboard.mozilla.org/r/51769/#review48779 Looks good to me!
Attachment #8751028 - Flags: review?(mgoodwin) → review+
Thanks for the reviews. Mounir - I chose you as a reviewer based on a quick hg annotate of dom/base/nsGlobalWindow.cpp, but it looks like the changes you made there weren't really specific to the entropy collection. Maybe it would be best if I just redirect the review.
Attachment #8751028 - Flags: review?(mounir) → review?(mrbkap)
Comment on attachment 8751028 [details] MozReview Request: bug 883718 - remove nsIEntropyCollector and implementation r?mounir r?mgoodwin https://reviewboard.mozilla.org/r/51769/#review48895
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Created attachment 8755741 [details] [diff] [review] Followup: Remove nsIBufEntropyCollector.idl nsIBufEntropyCollector.idl is no longer used post https://hg.mozilla.org/mozilla-central/rev/8dd88e2a1976, but was not removed. This (trivial) patch just removes the file. This is NPOTB, but here's a try push anyways: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d58a4ede1eea
Attachment #8755741 - Flags: checkin?
Whoops - thanks.
You need to log in before you can comment on or make changes to this bug.