If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 55

Status

()

Core
Networking
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 months ago
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)
(Assignee)

Updated

6 months ago
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)
(Assignee)

Comment 2

6 months ago
Created attachment 8862493 [details] [diff] [review]
make gSocketThread an internal implementation detail of nsSocketTransportService

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
status-firefox55: --- → affected
status-firefox57: affected → ---

Comment 3

6 months 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

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3dc6e29a0128
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.