1063648 - Harder to connect to simulators

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

After landing bug 1062748 for custom transports, it's now much harder to connect to a simulator.  Most connections fail for me.

But I really do not understand why that happens, since bug 1062748 shouldn't change this path.

The transport's output stream fails with NS_ERROR_CONNECTION_REFUSED.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Paul, any guess why this happens?  Maybe there's a race somewhere in the network code?

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

This might be platform-specific, like some other network issues we've seen.

Comment 3

[Paul Rouget [:paul]]

Meh. What happened? You're sure it's related to bug 1062748? Maybe it's another symptom of a race I was running into with connectPipe().

Comment 4

[Jeff Griffiths (:canuckistani) (:⚡︎)]

FWIW I see this every time since Saturday.

Comment 5

[Paul Rouget [:paul]]

The only different I see is that we add an extra reference to the transport.

Comment 6

[Alexandre Poirot [:ochameau]]

It's not realy harder, but just impossible to connect to a simulator.
Bug 1062748 regressed simulators. We shouldn't cache transport.

Comment 7

[Paul Rouget [:paul]]

Attachment: v1

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8485793 [details] [diff] [review]
v1

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

This does fix the simulator issue for me as well.

It is a bit awkward though (this is from the first patch that added custom transports) that the custom transport case can't support the |keepConnecting| mode, since we only pass in an already connected transport.  If we want to have this ability, we'd need to pass in a function to call to make transport in the custom case, in the same way that we use |connectPipe| and |debuggerSocketConnect|.  We can change this in a follow-up, but do you think we want it?  We'd likely have to change the API used by fxdt-adapters.

My suggestions for the current code are:
* Rename |this._transport| to |this._customTransport| since it's now only used for that purpose
* Skip the |keepConnecting| timer when there is a |this._customTransport| since it currently is meaningless
* Clear out |this._customTransport| on disconnect so we don't keep it alive

Comment 9

[Paul Rouget [:paul]]

Attachment: v2

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Filed bug 1064405 about future API change for custom transport.

Comment 11

[Paul Rouget [:paul]]

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

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://hg.mozilla.org/integration/fx-team/rev/01c8709207f2

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/01c8709207f2