Closed Bug 1360236 Opened 7 years ago Closed 7 years ago

hide gSocketThread's use in asserts and conditions behind a functional interface


(Core :: Networking, enhancement)

Not set



Tracking Status
firefox55 --- fixed


(Reporter: froydnj, Assigned: froydnj)


(Blocks 1 open bug)



(1 file)

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)
Blocks: 1359903
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)
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)
Attachment #8862493 - Flags: review?(mcmanus) → review+
Assignee: nobody → nfroyd
Pushed by
make gSocketThread an internal implementation detail of nsSocketTransportService; r=mcmanus
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.