1311251 - Crash in PR_Connect | mozilla::net::nsUDPSocket::Connect

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

This bug was filed from the Socorro interface and is 
report bp-c903d537-e670-4ed9-925e-d5d042161018.
=============================================================

17 crashes on nightly since build 20161014060324
but total 84 crashes since 20160702030219

Correlation on aurora shows:

(100.0% in signature vs 12.02% overall) address = 0x0
(75.00% in signature vs 09.02% overall) Addon "Video DownloadHelper" = true
(62.50% in signature vs 08.78% overall) useragent_locale = fr

https://mozilla.github.io/stab-crashes/correlations.html?product=Firefox&channel=aurora&signature=PR_Connect%20|%20mozilla::net::nsUDPSocket::Connect

Comment 1

[Honza Bambas (:mayhemer)]

SC, when you find time, could you take a look at this?  I know you have some experience with our UDP code.

Thanks.

Comment 2

[Shian-Yow Wu [:swu]]

This seems to be issue of null pointer mFD when calling PR_Connect().

Comment 3

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Shian-Yow Wu [:swu] from comment #2)
> This seems to be issue of null pointer mFD when calling PR_Connect().

Thanks for the information! It's still not obvious to me why nsUDPSocket::Connect is called if previous socket binding is failed. However we should always check the validity of mFD before using it.

Comment 4

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Bug 1311251 - null check for mFD before use it.

Review commit: https://reviewboard.mozilla.org/r/96184/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/96184/

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 8815154 [details]
Bug 1311251 - null check for mFD before use it.

Before I can r this patch I need to ask: on which thread can nsUDPSocket::CloseSocket() be called?  

the author didn't bother to add a thread assertion to that method, so hard to say and maybe it's intended to be called on more than a single thread.  If it can be called on the main thread, which probably can (as it's called from an IDL Close() method), then we have a bigger problem and this patch is not enough.

Comment 6

[Honza Bambas (:mayhemer)]

Note that nsUDPSocket::Close calls CloseSocket() under a lock, but it seems like it just protects the listener and not mFD.  This needs some larger cleanup.

Comment 7

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8815154 [details]
Bug 1311251 - null check for mFD before use it.

Sure, let me check if any other assertion/error handling need to add as well.

Comment 8

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Honza Bambas (:mayhemer) from comment #5)
> Comment on attachment 8815154 [details]
> Bug 1311251 - null check for mFD before use it.
> 
> Before I can r this patch I need to ask: on which thread can
> nsUDPSocket::CloseSocket() be called?  
> 
> the author didn't bother to add a thread assertion to that method, so hard
> to say and maybe it's intended to be called on more than a single thread. 
> If it can be called on the main thread, which probably can (as it's called
> from an IDL Close() method), then we have a bigger problem and this patch is
> not enough.

The thread model is complicated because nsIUDPSocket allows to be used on either main thread or PBackground thread. So, we can separate functions in nsUDPSocket into three groups:
 1) runs on main thread or PBackground thread, i.e. IDL methods/attributes
 2) runs on STS thread, i.e. socket control / IO
 3) don't care about thread, i.e. utility functions

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Any updates on this? We're running out of time to get a fix landed on 51.

Comment 10

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)
> Any updates on this? We're running out of time to get a fix landed on 51.

Didn't notice we need this bug fixed on 51 in the beginning. To provide a minimal patch, I'm thinking about just making nsUDPSocket::Close thread-safe (which addressed the concern in comment #5).

Comment 11

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: make sure PR_Close is on STS thread

Review commit: https://reviewboard.mozilla.org/r/101518/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/101518/

Comment 12

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8815154 [details]
Bug 1311251 - null check for mFD before use it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96184/diff/1-2/

Comment 13

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8822644 [details]
make sure PR_Close is on STS thread

@Honza, is making nsUDPSocket::Close thread-safe addressing your previous concern in comment #5?

Comment 14

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=26d7e1adf5dd

Comment 15

[Honza Bambas (:mayhemer)]

I understand this is an urgent bug, but the fix is still not complete.  I believe we should use the same mechanism as we have in nsSocketTransport - a class that wraps FD with a refcounter and let the code accesses the mFD member only under a lock, while doing any IO on it outside the lock.  Close of the FD is then done when the ref count drops to zero.

It's more work.  But I'm afraid the two patches here (and mainly the CloseSocket one) will just shift the problem, not fix it.

Or, would just the FD non-null check in nsUDPSocket::Connect be enough to quick-fix the obvious crash here?  That might be OK to uplift (to stop this being a top-crash, at least) and solve the rest in a followup.

Comment 16

[Honza Bambas (:mayhemer)]

Comment on attachment 8815154 [details]
Bug 1311251 - null check for mFD before use it.

https://reviewboard.mozilla.org/r/96184/#review104262

Let's uplift this up to beta.  It should at least fix the immediate crash here.

Comment 17

[Honza Bambas (:mayhemer)]

Comment on attachment 8822644 [details]
make sure PR_Close is on STS thread

https://reviewboard.mozilla.org/r/101518/#review104264

No, let's rather do this properly and let it ride the trains.  See comment 15.

Comment 18

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8815154 [details]
Bug 1311251 - null check for mFD before use it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96184/diff/2-3/

Comment 19

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Thanks for the advise. I filed Bug 1330180 as a follow-up to comment #15.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b29345665e1d
null check for mFD before use it. r=mayhemer

Comment 21

[Carsten Book [:Tomcat]]

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora/Beta approval on this when you get a chance.

Comment 23

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8815154 [details]
Bug 1311251 - null check for mFD before use it.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1194259
[User impact if declined]: user might hit top 1 firefox crash
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: n/a
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: simply do null check to prevent crash
[String changes made/needed]: n/a

Comment 24

[Gerry Chang [:gchang]]

Comment on attachment 8815154 [details]
Bug 1311251 - null check for mFD before use it.

Fix a crash. Beta51+ & Aurora52+. Should be in 51 beta 14.

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dca36dc05543

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4b75f0404cd6