Closed Bug 1202468 Opened 4 years ago Closed 4 years ago

nsSocketTransportService::mPendingSocketQ is locked unnecessarily

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: froydnj, Assigned: mcmanus)

Details

(Whiteboard: [necko-active])

Attachments

(1 file)

AFAICT, mPendingSocketQ is only ever accessed on the socket transport thread:

https://dxr.mozilla.org/mozilla-central/search?q=mPendingSocketQ&redirect=true&case=true&limit=99&offset=0

NotifyWhenCanAttachSocket, where events are added to the queue, has an explicit assertion that we are on the socket thread.

DetachSocket, where events are removed from the queue, has no such assertion, but a try run with such an assertion added does not result in failures.  The comment hereabouts:

http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsSocketTransportService2.cpp#188

suggests that the following functions are only supposed to be called on the socket thread.  Tracing through the call chains suggests that Detach Socket is indeed only called on the socket thread.

We should remove this unnecessary locking, either by refactoring nsEventQueue, or finding a different data structure to use.
I think you're right that the locked class is un-necessary.

I also think this code path is pretty unusual (its an overflow kind of thing) and the removing the lock won't impact most cases.
Assignee: nobody → mcmanus
Whiteboard: [necko-active]
Attachment #8755490 - Flags: review?(dd.mozilla)
Attachment #8755490 - Flags: review?(dd.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/fd50e33b4e9b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.