876167 - SCTP library crash when processing association abort while closing socket

Status: RESOLVED WONTFIX

(Core :: WebRTC: Networking, defect, P3)

Description

[Randell Jesup [:jesup] (needinfo me)]

With the patch in bug 872978 (patch https://bugzilla.mozilla.org/attachment.cgi?id=754142), the leaks are fixed but a probable bug in the SCTP library is exposed, where it's processing an incoming association ABORT while in the process of closing the local socket.  This occurs on Windows (mostly on opt builds, but seen once on a debug build).

I'll attach stack traces.

The other option is to try to work around it; since this didn't occur before landing the leak-fix patch; we can probably defer SCTP shutdown until the peerconnection itself is shut down in order to land the leak fix.  Under that assumption, marking as blocking-minus, but if we have a safe fix we'd take it and probably uplift it at least to Aurora/23, maybe to beta/22.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=23398227&tree=Try
https://tbpl.mozilla.org/php/getParsedLog.php?id=23386201&tree=Try
https://tbpl.mozilla.org/php/getParsedLog.php?id=23386211&tree=Try

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Attachment: callback

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Note: This also needs the mochitest patch for datachannels from Bug 796894

Comment 4

[Michael Tüxen]

Does the following patch fix the issue:
http://code.google.com/p/sctp-refimpl/source/detail?r=8481

It fixes an issue I can reproduce, but the crash I was able to reproduce
was a couple of lines earlier...

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Trying that...

https://tbpl.mozilla.org/?tree=Try&rev=924b9d40e84a

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Nope.  First Win7 opt testrun hit the same crash

Comment 7

[Michael Tüxen]

Thanks, I saw it. Will continue debugging...

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Attachment: avoid usrsctp library race condition on close with ABORT

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 757063 [details] [diff] [review]
avoid usrsctp library race condition on close with ABORT

includes the 1-liner from rev 8481 (though it may not matter)
This is a workaround patch that avoids the race but does not fix it, and will be backed out when the fixed library is imported.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

See Try at https://tbpl.mozilla.org/?tree=Try&rev=2632e699f5d4

The early oranges were an infrastructure problem; the later ones were a bug in the old version of the mochitests for datachannel.  Before this patch this would have failed 50-75% of Win7 opt mochitest runs.

Comment 11

[Michael Tüxen]

Comment on attachment 757063 [details] [diff] [review]
avoid usrsctp library race condition on close with ABORT

Review of attachment 757063 [details] [diff] [review]:
-----------------------------------------------------------------

Look good to me. r+

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 757063 [details] [diff] [review]
avoid usrsctp library race condition on close with ABORT

r=tuexen

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a00efe9a0e8

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0a00efe9a0e8

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 757063 [details] [diff] [review]
avoid usrsctp library race condition on close with ABORT

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A (bug in upstream library)

User impact if declined: Crash (maybe worse in theory) if an incoming ABORT collides with a local close of the DataChannelConnection.  If the leak fix lands, the mochitests about to be added to the tree will hit this failure frequently on Win7 Opt builds; rarely on debug builds.  In theory this might fail on Linux, but we haven't seen it.

Testing completed (on m-c, etc.): on m-c, ~45 retriggers with no failures.  This is a workaround to avoid the race condition until a root-cause fix can land and be imported (Michael Tuexen is looking at the base problem).

Risk to taking this patch (and alternatives if risky): Low.  Proxies existing close to the STS thread to it can't race against incoming packets on STS.  Risk would be if I missed something in the state of the DataChannelConnection; overall a simple (wallpaper) fix though.

Even if we don't take the leak fix, we should strongly consider taking this, as it could in theory happen to any connection with the right timing - the leak fix makes it easier to hit with the mochitests.

String or IDL/UUID changes made by this patch: none

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking this so we can get early warning if there are regressions or unexpected fallout.

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-beta/rev/de4c9a61681a
aurora is blocked on closure

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2b6a584d710f

Comment 19

[Mihaela Velimiroviciu (:mihaelav)]

I verified the TBPL logs for Mozilla-Beta/Win 7 and the crash does not appear.
Is there anything else QA can do to manually verify this?

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

Not really; the patch that exposed the bug has landed; if you add the datachannel tests patch referenced above, you can see if you hit failures on Win7 opt Try builds when retriggered a number of times.  True verification would be to do that, and then back this patch out and see if you see the crashes.  If you don't see the crashes with this backed out, and the datachannel test in, then you can't be sure because the tests aren't hitting the intermittent.

Not an easy verification.

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

Still needs to be backed out when we update SCTP

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

Attachment: backout temporary fix for SCTP library bug (now fixed)

Comment 23

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8389843 [details] [diff] [review]
backout temporary fix for SCTP library bug (now fixed)

This is just undoing the previous patch.  Since we've updated the SCTP library, we should no longer need to avoid this collision.

Comment 24

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ada8c5a0480

I had a try run that was orange due to non-related issues in the underlying repo, but all the datachannel tests ran without errors.

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7ada8c5a0480

Comment 27

[Randell Jesup [:jesup] (needinfo me)]

Turns out the library bug was *not* fixed; backing out the backout

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=36191746&tree=Fx-Team

Comment 29

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ed8689acdfa

Comment 31

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/7ed8689acdfa

Comment 33

[Randell Jesup [:jesup] (needinfo me)]

Poke to check if we've fixed this in SCTP and can back out the wallpaper

Comment 34

[Randell Jesup [:jesup] (needinfo me)]

Michael - is this bug fixed upstream?  Or do we still need to proxy the socket close?

Comment 35

[Michael Tüxen]

I'm not aware of a specific fix, since I couldn't reproduce it the problem.
Keep the work around for now.

Comment 36

[Bulk Bug Changes for mreavy's org]

Mass change P2->P3 to align with new Mozilla triage process.

Comment 37

[Sylvestre Ledru [:Sylvestre]]

Closing because no crash reported since 12 weeks.