Closed
Bug 744788
Opened 12 years ago
Closed 12 years ago
connectionmgr timer tick needs refreshed setTarget if we went offline
Categories
(Core :: Networking: HTTP, defect)
Tracking
()
RESOLVED
FIXED
mozilla14
Tracking | Status | |
---|---|---|
firefox12 | --- | unaffected |
firefox13 | --- | fixed |
firefox14 | --- | fixed |
People
(Reporter: mcmanus, Assigned: mcmanus)
References
Details
(Whiteboard: [qa-])
Attachments
(1 file, 1 obsolete file)
982 bytes,
patch
|
mayhemer
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #614385 -
Flags: review?(honzab.moz)
Comment 2•12 years ago
|
||
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.
Assignee | ||
Comment 3•12 years ago
|
||
(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.
Summary: connectionmgr timer tick needs refreshed setTaget if we went offline → connectionmgr timer tick needs refreshed setTarget if we went offline
Assignee | ||
Comment 4•12 years ago
|
||
(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 :)
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #614385 -
Attachment is obsolete: true
Attachment #614385 -
Flags: review?(honzab.moz)
Attachment #614570 -
Flags: review?(honzab.moz)
Comment 6•12 years ago
|
||
Comment on attachment 614570 [details] [diff] [review] patch v1 Review of attachment 614570 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab
Attachment #614570 -
Flags: review?(honzab.moz) → review+
Assignee | ||
Comment 7•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/885bb3f8d0dd
Comment 8•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/885bb3f8d0dd
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 9•12 years ago
|
||
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.
Attachment #614570 -
Flags: approval-mozilla-aurora?
Comment 10•12 years ago
|
||
Comment on attachment 614570 [details] [diff] [review] patch v1 [Triage Comment] Approved for landing to help clear up intermittent oranges.
Attachment #614570 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 11•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/1c692baf7a1a
Comment 13•12 years ago
|
||
(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.
Whiteboard: [qa?] → [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•