Closed Bug 486063 Opened 15 years ago Closed 15 years ago

AutoDialer support on Windows Mobile doesn't work just quite right

Categories

(Core :: Networking, defect)

x86
Windows Mobile 6 Professional
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 1.0a1-wm+ ---

People

(Reporter: dougt, Assigned: dougt)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch v.1 (obsolete) — Splinter Review
Sometimes the autodialer does not make a physical connection to the network and page loads fail.  During investigation I found that the connection failures occurred mainly because we called ConnMgrReleaseConnection which releases the current connection.  It was believed that the timeout would be honored and that connections that were established would not immediately disconnection. However, a small test app shows that calls to ConnMgrReleaseConnection following ConnMgrEstablishConnectionSync would drop the connection.

The following patch does two things:

1) it removes our calls to ConnMgrReleaseConnection.  This doesn't cause a leak, but it does prevent connections from being terminated by us.

2) stubs our our link monitoring code.  The periodic polling using a timer did not provide enough frequency to notify us when we go offline.  ideally we would get an event that signatified a RAS disconnect.  however, the closest API for this on windows mobile is an optional HWND handle in CONNMGR_CONNECTIONINFO.  We can pass a window to be notified when there is a disconnection.  Because of the plumbing required and my understanding that offline notification support is not that important now, I am just stubbing out this class.  If need be, we can file new bug to track this issue.
Attachment #370141 - Flags: review?(jduell.mcbugs)
Attachment #370141 - Flags: review?(jduell.mcbugs) → review+
Comment on attachment 370141 [details] [diff] [review]
patch v.1

I don't know enough about WINCE to say if you're doing the right things or not, but your code certainly "works" now (in that most of your functions are empty, and hence cannot break!).
mfinkle, can you test with this patch when you get back into town?
Attachment #370141 - Flags: review?(bugmail)
Comment on attachment 370141 [details] [diff] [review]
patch v.1

brad, can you look at the wince pieces again.
Comment on attachment 370141 [details] [diff] [review]
patch v.1


> 
> NS_IMETHODIMP
> nsNotifyAddrListener::GetIsLinkUp(PRBool *aIsUp)
> {
>-  *aIsUp = mLinkUp;
>+  *aIsUp = PR_FALSE; /* Always say no so that we can attempt to dial on network failures. */
>   return NS_OK;
> }

it seems like it would be more correct to implement this as 
 
ConnMgrConnectionStatus(mConnectionHandle, status); 
*aIsUp = status == CONNMGR_STATUS_CONNECTED;


> NS_IMETHODIMP
> nsNotifyAddrListener::GetLinkStatusKnown(PRBool *aIsUp)
> {
>-  *aIsUp = mStatusKnown;
>+  *aIsUp = PR_FALSE;
>   return NS_OK;
> }

same

ConnMgrConnectionStatus(mConnectionHandle, status); 
*aIsUp = status != CONNMGR_STATUS_UNKNOWN;


If performance is an issue, please add a comment.
Attachment #370141 - Flags: review?(bugmail) → review-
Attached patch what brad saidSplinter Review
Attachment #370141 - Attachment is obsolete: true
Attachment #370730 - Flags: review?(bugmail)
Comment on attachment 370730 [details] [diff] [review]
what brad said

the cost per call to ConnMgrConnectionStatus is about < 1ms when there are no other callers.  If multiple threads repeatably call this API, the cost increases ( on average 6ms per call ).

This value is worse case as i stressed the contention on any global lock it may be holding internally.  basically, i created a 4 threads which repeatedly called ConnMgrConnectionStatus 10000 times.  The total cost of each thread loop was about 59000 ms.
Attachment #370730 - Flags: review?(bugmail) → review+
Comment on attachment 370730 [details] [diff] [review]
what brad said


>+: mConnectionHandle(NULL)
> {
> }

nit, indentation looks wrong here.  Could also just pull this up to the next line
Attachment #370730 - Flags: superreview?(pavlov)
tracking-fennec: --- → ?
tracking-fennec: ? → 1.0a1-wm+
mfinkle, could you try this patch ?
mfinkle said that this works.  woot.
Comment on attachment 370730 [details] [diff] [review]
what brad said

Please rename the class in nsAutodialWinCE to nsAutodialWinCE.

The code in nsNotifyAddrListener::GetIsLinkUp is the same as the code in GetLinkStatusKnown.  Should these call each other?

How expensive are these calls to ConnMgrConnectionStatus()?  Do they get called frequently enough that the timer made sense?
Attachment #370730 - Flags: superreview?(pavlov) → superreview-
(In reply to comment #10)
> (From update of attachment 370730 [details] [diff] [review])

> The code in nsNotifyAddrListener::GetIsLinkUp is the same as the code in
> GetLinkStatusKnown.  Should these call each other?

their tests are different (*aIsUp = status == CONNMGR_STATUS_CONNECTED;
 vs. *aIsUp = status != CONNMGR_STATUS_UNKNOWN;)  Are you suggesting a helper function should be used?

That being said, aIsUp should be renamed to aIsKnown in the second function for clarity.

> 
> How expensive are these calls to ConnMgrConnectionStatus()?  Do they get called
> frequently enough that the timer made sense?

Doug said they were inexpensive on irc when I asked, but it would be nice to have some data on the bug as to how inexpensive.
regarding ConnMgrConnectionStatus performance, see comment #6.
Attachment #370730 - Flags: superreview- → superreview+
see 488140 for the name change.
pushed http://hg.mozilla.org/mozilla-central/rev/6ffb78941637
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: