Closed Bug 1152334 Opened 5 years ago Closed 5 years ago

StartWebsocketData null deref mSocketIn


(Core :: Networking: WebSockets, defect)

32 Branch
Not set



Tracking Status
firefox41 --- fixed


(Reporter: mcmanus, Assigned: michal)




(1 file)

There are actually quite a few of these in crash-stats.. here ar a few:

the issue is mSocketIn->AsyncWait(this, 0, 0, mSocketThread) where mSocketIn == nullptr

After a quick look I suspect that mSocketIn is being set to null on the socket thread through some kind of cancel path and this check is on the main thread - so a if() isn't going to be enough to get the job done.
michal do you think you can try and figure this out?
Flags: needinfo?(michal.novotny)
I'll have a look.
Assignee: nobody → michal.novotny
Flags: needinfo?(michal.novotny)
Attached patch fixSplinter Review
mSocketIn is nulled out only in WebSocketChannel::CleanupConnection(). I've tried to track down all possible callstacks ending in WebSocketChannel::CleanupConnection() on the socket thread while WebSocketChannel::StartWebsocketData() is running on the target thread and AFAICS the only possibility is from WebSocketChannel::Notify() due to ping timeout. This seems weird because the timer is initialized just before the crash on the same thread. But the ping timer is also created in WebSocketChannel::Observe() and the logic there is wrong because it checks only whether AsyncOpen() was already called but it should IMO check also mDataStarted. Also the timer and related variables are accessed on multiple threads without a lock, so let's fix this and see if it fixes the crash.

This patch changes:
 - mDataStarted is checked on NS_NETWORK_LINK_DATA_CHANGED notification
 - mPingTimer and related variable are accessed only on socket thread 
 - mPingTimer is initialized after the call to mSocketIn->AsyncWait() in WebSocketChannel::StartWebsocketData()
Attachment #8601065 - Flags: review?(mcmanus)
Comment on attachment 8601065 [details] [diff] [review]

Review of attachment 8601065 [details] [diff] [review]:

thanks michal - this makes sense to me from a high level. I'll delegate the review to daniel.
Attachment #8601065 - Flags: review?(mcmanus) → review?(daniel)
Comment on attachment 8601065 [details] [diff] [review]

Review of attachment 8601065 [details] [diff] [review]:

I really like how this improves readability of the code for websocket pinging and acting on the network change event.
Attachment #8601065 - Flags: review?(daniel) → review+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
See Also: → 1227136
You need to log in before you can comment on or make changes to this bug.