connectionmgr timer tick needs refreshed setTarget if we went offline

RESOLVED FIXED in Firefox 13

Status

()

Core
Networking: HTTP
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

14 Branch
mozilla14
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox12 unaffected, firefox13 fixed, firefox14 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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

5 years ago
Created attachment 614385 [details] [diff] [review]
patch 0
Attachment #614385 - Flags: review?(honzab.moz)
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

5 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

5 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

5 years ago
Created attachment 614570 [details] [diff] [review]
patch v1
Attachment #614385 - Attachment is obsolete: true
Attachment #614385 - Flags: review?(honzab.moz)
Attachment #614570 - Flags: review?(honzab.moz)
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

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/885bb3f8d0dd
https://hg.mozilla.org/mozilla-central/rev/885bb3f8d0dd
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
(Assignee)

Updated

5 years ago
status-firefox12: --- → unaffected
status-firefox13: --- → affected
status-firefox14: --- → fixed
(Assignee)

Comment 9

5 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 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

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/1c692baf7a1a
status-firefox13: affected → fixed
Is this a fix QA can verify?
Whiteboard: [qa?]
(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.