1362978 - Crash in nsCOMPtr_base::assign_with_AddRef | nsInputStreamReadyEvent::OnInputStreamReady

Status: RESOLVED DUPLICATE of bug 1363372

(Core :: Networking: HTTP, defect)

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is 
report bp-ee1732cc-d833-4fd3-b632-54e4a0170507.
=============================================================

There are 8 crashes in nightly 55 with buildid 20170507030205. In analyzing the backtrace, the regression may have introduced by patch [1] to fix bug 1359938.

[1] https://hg.mozilla.org/mozilla-central/rev?node=4a3896a7e29f605d0bc509ca98095c1960aa70cf

Comment 1

[Calixte Denizet (:calixte)]

There are 4 crashes with signature "RefPtr<T>::assign_with_AddRef | nsInputStreamReadyEvent::OnInputStreamReady" and buildid 20170506030204.

Comment 2

[Dragana Damjanovic [:dragana]]

This one is not an easy one. I have a theory:

This is a crash in inputStream. During TFO we should only wait for outputStream, i.e. we are waiting for a connect. After connect TFO is finished the connection continues to perform in the same way as before.

I was assuming we never call ResumeRecv and I made sure that nsHttpConnection flow does not do it, but h2 does it on its own.
H2 do call ResumeRecv independent of nsHttpConnection. So when we cancel TFO connection we do not clear inputStream callbacks and if the underlying socketTransport is reused, this can happen after RecoverFromError is called and socketTransport retries and reconnects, it can happend that inputStream still has the old callback, which is a old transaction and not the one new one...

I will make a patch that clears inputStream callbacks if TFO connection gets canceled.
I will also disallow call to inputStream->AsyncWait if we are in TFO, but I will ask Patrick to confirm that I will not mess up h2 work flow, we knows H2 code.

I am not sure that this is going to solve this problem, let's see.

Comment 3

[Dragana Damjanovic [:dragana]]

Attachment: bug_1362978_v1.patch

May I forbid calling mSocketIn->AsyncWait(nullptr, 0, 0, nullptr); in ResumeRecv if fastopen is in progress? I am asking if whether is this going to break h2. If my theory is correct the other part of the patch will resolve the crash on it own.

Comment 4

[Dragana Damjanovic [:dragana]]

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

Comment 5

[Patrick McManus [:mcmanus]]

Comment on attachment 8866312 [details] [diff] [review]
bug_1362978_v1.patch

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

I think it is fine to suppress resumerecv as long as it is called unsupressed again when the handshake is over.. and that seems to be missing.

relatedly halfopensocket::setfastopenstatus calls nshttpconnection::setfastopenstatus, but instead of that calling through to its nsAHttpTransaction, the halfopen socket does that directly only if the connection is h1. shouldn't the connection just call through the nsAHttpTransaction which might be h1, or it might be the h2 session which would want to know this for exactly reasons like calling resumerecv() ?

Comment 6

[Dragana Damjanovic [:dragana]]

(In reply to Patrick McManus [:mcmanus] from comment #5)
> Comment on attachment 8866312 [details] [diff] [review]
> bug_1362978_v1.patch
> 
> Review of attachment 8866312 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think it is fine to suppress resumerecv as long as it is called
> unsupressed again when the handshake is over.. and that seems to be missing.

The patch suppresses ResumeRecv in nsHttpConnection if mFastOpen is true. It is set to false in 
nsHttpConnectionMgr::nsHalfOpenSocket::SetFastOpenConnected which is called in both cases on error and on a successful connect

> relatedly halfopensocket::setfastopenstatus calls
> nshttpconnection::setfastopenstatus, but instead of that calling through to
> its nsAHttpTransaction, the halfopen socket does that directly only if the
> connection is h1. shouldn't the connection just call through the
> nsAHttpTransaction which might be h1, or it might be the h2 session which
> would want to know this for exactly reasons like calling resumerecv() ?

You have a point there, but that is a separate issue. This is only used for telemetry. I will fix that I added a check because the function was not implemented in NullHttpTransaction. I will open a separate bug for this one.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34a781e4f023
Remove callbacks on socketInput stream if we cancel a TFO socketTransport. r=mcmanus

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/34a781e4f023

Comment 9

[Mike Taylor [:miketaylr]]

Dragana, should this still be left open?

Comment 10

[Dragana Damjanovic [:dragana]]

(In reply to Mike Taylor [:miketaylr] from comment #9)
> Dragana, should this still be left open?

I have turned off tfo, so I do not know if this is fix or not.
I want to land one more patch and do some testing before turning TFO back on.

I will leave this bug open until I confirm that it is fixed.

Comment 11

[Dragana Damjanovic [:dragana]]

this is still present with TFO from 31.5.

Comment 12

[Jason Duell]

Crash signatures here are still high--between the 3 crash sigs listed above, we're getting around 60 crashes/day on 56.

Comment 13

[Dragana Damjanovic [:dragana]]

This is tfo crash. TFO will be disabled in 56.

Comment 15

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.