Closed Bug 1152334 Opened 5 years ago Closed 5 years ago

StartWebsocketData null deref mSocketIn

Categories

(Core :: Networking: WebSockets, defect)

32 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: mcmanus, Assigned: michal)

References

Details

Attachments

(1 file)

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.
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]
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.
Attachment #8601065 - Flags: review?(mcmanus) → review?(daniel)
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.
Attachment #8601065 - Flags: review?(daniel) → review+
https://hg.mozilla.org/mozilla-central/rev/f39f5779392d
Status: NEW → RESOLVED
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.