As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 744788 - connectionmgr timer tick needs refreshed setTarget if we went offline
: connectionmgr timer tick needs refreshed setTarget if we went offline
Status: RESOLVED FIXED
[qa-]
:
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]
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
fixed
fixed


Attachments
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 User image 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 User image Patrick McManus [:mcmanus] 2012-04-12 08:34:32 PDT
Created attachment 614385 [details] [diff] [review]
patch 0
Comment 2 User image 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 User image 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 User image 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 User image Patrick McManus [:mcmanus] 2012-04-12 14:30:40 PDT
Created attachment 614570 [details] [diff] [review]
patch v1
Comment 6 User image 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]:
-----------------------------------------------------------------

r=honzab
Comment 7 User image Patrick McManus [:mcmanus] 2012-04-13 12:52:55 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/885bb3f8d0dd
Comment 8 User image :Ms2ger (⌚ UTC+1/+2) 2012-04-14 06:42:34 PDT
https://hg.mozilla.org/mozilla-central/rev/885bb3f8d0dd
Comment 9 User image 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 User image 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 User image Patrick McManus [:mcmanus] 2012-04-16 19:56:00 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/1c692baf7a1a
Comment 12 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-10 12:09:05 PDT
Is this a fix QA can verify?
Comment 13 User image 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.