1242597 - reference loop in nsSocketTransport AsyncCopy

Status: NEW

(Core :: Networking, defect, P3)

Description

[Patrick McManus [:mcmanus]]

when using an async copier with a pipe, a refernece loop can form as
follows:

copier holds reference to transport (via &mOutput argument to
NS_AsynCopy) and eventually through nsStreamUtils.h calls
transport::outputstream::asyncwait using the copier as the
callback. the asyncwait() method holds a reference to the copier until
calling OnOutputStream ready. The copier also references transport, so
a loop is formed and if OnOutputStreamReady is not called it does not
get broken - both the copier and the transport are leaked.

(an example of this is in OpenOutputStream() where the code path does NS_AsyncCopy)

the patch rejects asyncwait() calls for sockets that have been already
been detached. (detaching results in the outputstreamready function
being invoked and the callback being cleared.)

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: Fix nsSocketTransport reference loop

Comment 2

[Patrick McManus [:mcmanus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac01f86f2367&selectedJob=15825599

Comment 3

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e32d3531b96def705f7e5a84045365d29716ce1
Bug 1242597 - Fix nsSocketTransport reference loop r=dragana

Comment 4

[Carsten Book [:Tomcat]]

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

Comment 5

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22f3ea5449d75019d43888b75036ab4790ff4780
Bug 1242597 - backout 7e32d3531b96def705f7e5a84045365d29716ce1 r=backout

Comment 6

[Patrick McManus [:mcmanus]]

backed out for crashes in 1243424

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/22f3ea5449d7

Comment 8

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(In reply to Carsten Book [:Tomcat] from comment #7)
> https://hg.mozilla.org/mozilla-central/rev/22f3ea5449d7

This looks like backout, so reopening
or I'm wrong Carsten Book [:Tomcat]?

Comment 9

[Carsten Book [:Tomcat]]

(In reply to Virtual_ManPL [:Virtual] from comment #8)
> (In reply to Carsten Book [:Tomcat] from comment #7)
> > https://hg.mozilla.org/mozilla-central/rev/22f3ea5449d7
> 
> This looks like backout, so reopening
> or I'm wrong Carsten Book [:Tomcat]?

indeed seems the tool didn't detected this, thanks!

Comment 10

[Jason Duell]

Worth working on but Patrick doesn't have cycles right now. Volunteers?

Comment 11

[Dragana Damjanovic [:dragana]]

Let's land this, and see if we still have the same crash.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a0f2b93749a
Fix nsSocketTransport reference loop. r=dragana

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out because it appears to be the cause of leaksanitizer failures like https://treeherder.mozilla.org/logviewer.html#?job_id=34106368&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d4a3ed75340

Comment 14

[Firefox Bug Husbandry Bot]

Bulk priority update: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 16

[Dragana Damjanovic [:dragana]]

I will not have time to work on this bug, so I am unassigning myself.