1152334 - StartWebsocketData null deref mSocketIn

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Patrick McManus [:mcmanus]]

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

https://crash-stats.mozilla.com/report/index/452623e4-3822-4140-9f07-f31372150401
https://crash-stats.mozilla.com/report/index/e94a47f6-69b1-48fc-a4b2-15fad2150407
https://crash-stats.mozilla.com/report/index/50f86bc9-59ef-483a-afee-2c12e2150403

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.

Comment 1

[Patrick McManus [:mcmanus]]

michal do you think you can try and figure this out?

Comment 2

[Michal Novotny [:michal]]

I'll have a look.

Comment 3

[Michal Novotny [:michal]]

Attachment: fix

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()

Comment 4

[Patrick McManus [:mcmanus]]

Comment on attachment 8601065 [details] [diff] [review]
fix

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.

Comment 5

[Daniel Stenberg [:bagder]]

Comment on attachment 8601065 [details] [diff] [review]
fix

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.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f39f5779392d

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f39f5779392d