744788 - connectionmgr timer tick needs refreshed setTarget if we went offline

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Patrick McManus [:mcmanus]]

connection manager re-inits an existing nsITimer when it is needed and it is not currently running. However that timer's target is the socket thread at the time of timer creation, and if we have gone offline and back online since that creation that thread no longer exists and therefore the timer events never run.

This might explain why the keep-alive code that should at least repair the bug associated with 739522 didn't have that effect.

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: patch 0

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 614385 [details] [diff] [review]
patch 0

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

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +2010,5 @@
>      }
> +    else {
> +        // In the case we have gone offline since last arming the timer, we
> +        // need to update the event target as the socket thread has changed.
> +        mReadTimeoutTick->SetTarget(mSocketThreadTarget);

Good catch.

You may do this in EnsureSocketThreadTargetIfOnline.  This is done unnecessarily on every tick.

Comment 3

[Patrick McManus [:mcmanus]]

(In reply to Honza Bambas (:mayhemer) from comment #2)

> You may do this in EnsureSocketThreadTargetIfOnline.  This is done
> unnecessarily on every tick.

fwiw as posted it is assigned not on every tick, but every initialization of the timer. (so every quiescent/reactivation cycle). but you're right that it can be less.

EnsureSocketThreadTargetIfOnline confuses me a bit.. there is no point in it where an offline/online transition is noted. Once init'd mSocketThreadTarget never changes (though the real thread it applies to does because it is really a qi of the socket transport service).

which leads me to the right change (attached), just setTarget the timer to be mSocketThreadTarget once when it is created rather than allowing the default nsTimerImpl code to set the current thread explicitly as the target. mSocketThreadTarget will be updated to the right thread that way as the STS reinits. There isn't a concern about using it when its offline.

Comment 4

[Patrick McManus [:mcmanus]]

(In reply to Honza Bambas (:mayhemer) from comment #2)

> 
> Good catch.
> 

I wish I could say I knew it was a problem - the truth is I was trouble shooting another issue and had to answer the question of why my timer tick obviously wasn't running :)

Comment 5

[Patrick McManus [:mcmanus]]

Attachment: patch v1

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 614570 [details] [diff] [review]
patch v1

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

r=honzab

Comment 7

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/885bb3f8d0dd

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/885bb3f8d0dd

Comment 9

[Patrick McManus [:mcmanus]]

Comment on attachment 614570 [details] [diff] [review]
patch v1

This is one of a two patch series (bug 744788 and bug 738812) that should be accepted or declined for aurora atomically.

We've only seen this orange on FF14, but it can happen on FF13. The reason it is more prominent on 14 is the timer event is much more frequent there due to the pipelining changes introduced on 14, but the same timer is used by spdy on 13.

[Approval Request Comment]
Regression caused by (bug #): 728113
User impact if declined: potential crash risk during offline or shutdown
Testing completed (on m-c, etc.): m-c - orange cleared up.
Risk to taking this patch (and alternatives if risky): fairly low.
String changes made by this patch: none.

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 614570 [details] [diff] [review]
patch v1

[Triage Comment]
Approved for landing to help clear up intermittent oranges.

Comment 11

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1c692baf7a1a

Comment 12

[u279076]

Is this a fix QA can verify?

Comment 13

[u279076]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #12)
> Is this a fix QA can verify?

Taking silence as a 'no'. Please mark as [qa+] if there is something QA can do to verify this fix.