Closed
Bug 1360236
Opened 7 years ago
Closed 7 years ago
hide gSocketThread's use in asserts and conditions behind a functional interface
Categories
(Core :: Networking, enhancement)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: froydnj, Assigned: froydnj)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
158.35 KB,
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
As part of Quantum DOM, we're planning on curtailing the use of PR_GetCurrentThread to a few whitelisted places (details to be posted on dev-platform soonish). The various assertions netwerk/ has on gSocketThread are a big piece of the current uses of PR_GetCurrentThread. There are two options for reducing this: 1) Add bool OnSocketThread() to nsSocketTransportService2.{h,cpp} and replace all the current ==/!= tests, whether in assertions or otherwise, against gSocketThread with calls to that (out-of-line) function. 2) As in #1, but replace the MOZ_ASSERT(OnSocketThread()) calls with out-of-line AssertOnSocketThread() functions; we'd also need a corresponding AssertNotOnSocketThread for a few places. In both scenarios, gSocketThread would become a private implementation detail of nsSocketTransportService2.cpp. I was initially going to do #2, but I suspect that #1 is easier to implement and possibly a little more palatable. Do you have a preference?
Flags: needinfo?(mcmanus)
Comment 1•7 years ago
|
||
Thanks nathan. I love those asserts btw - super useful documentation and enforcement. So I appreciate you not just wanting to delete them :) both your approaches are ok by me; I guess I have a slight preference for #1. Another option might be a gecko-wide functon AssertCurrentThread(thread *) that has a nop inline for NDEBUG and does the obvious out of line thing for DEBUG.
Flags: needinfo?(mcmanus)
![]() |
Assignee | |
Comment 2•7 years ago
|
||
This change makes the code a little cleaner and reduces the number of places we call PR_GetCurrentThread, which is important for Quantum DOM scheduling work. The conversion was largely automatic, via: find netwerk/ -name \*.cpp | \ xargs sed -i -e 's/MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread[^;]*/MOZ_ASSERT(OnSocketThread(), "not on socket thread")/' and related invocations, with a few manual tweaks at the end.
Attachment #8862493 -
Flags: review?(mcmanus)
Updated•7 years ago
|
Attachment #8862493 -
Flags: review?(mcmanus) → review+
Updated•7 years ago
|
Pushed by nfroyd@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3dc6e29a0128 make gSocketThread an internal implementation detail of nsSocketTransportService; r=mcmanus
Comment 4•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3dc6e29a0128
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•