Last Comment Bug 744788 - connectionmgr timer tick needs refreshed setTarget if we went offline
: connectionmgr timer tick needs refreshed setTarget if we went offline
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: 14 Branch
: x86_64 Linux
: -- normal (vote)
: mozilla14
Assigned To: Patrick McManus [:mcmanus]
: Patrick McManus [:mcmanus]
Depends on: 738812
Blocks: 739522
  Show dependency treegraph
Reported: 2012-04-12 08:31 PDT by Patrick McManus [:mcmanus]
Modified: 2012-05-29 11:37 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch 0 (1.15 KB, patch)
2012-04-12 08:34 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
patch v1 (982 bytes, patch)
2012-04-12 14:30 PDT, Patrick McManus [:mcmanus]
honzab.moz: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Patrick McManus [:mcmanus] 2012-04-12 08:31:03 PDT
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] 2012-04-12 08:34:32 PDT
Created attachment 614385 [details] [diff] [review]
patch 0
Comment 2 Honza Bambas (:mayhemer) 2012-04-12 12:40:42 PDT
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] 2012-04-12 14:22:13 PDT
(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] 2012-04-12 14:29:50 PDT
(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] 2012-04-12 14:30:40 PDT
Created attachment 614570 [details] [diff] [review]
patch v1
Comment 6 Honza Bambas (:mayhemer) 2012-04-13 07:46:56 PDT
Comment on attachment 614570 [details] [diff] [review]
patch v1

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

Comment 7 Patrick McManus [:mcmanus] 2012-04-13 12:52:55 PDT
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2012-04-14 06:42:34 PDT
Comment 9 Patrick McManus [:mcmanus] 2012-04-15 10:50:23 PDT
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 2012-04-16 15:39:25 PDT
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] 2012-04-16 19:56:00 PDT
Comment 12 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-10 12:09:05 PDT
Is this a fix QA can verify?
Comment 13 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-29 11:37:08 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.