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)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
fennec | 1.0a1-wm+ | --- |
People
(Reporter: dougt, Assigned: dougt)
Details
Attachments
(1 file, 1 obsolete file)
9.07 KB,
patch
|
blassey
:
review+
pavlov
:
superreview+
|
Details | Diff | 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.
Assignee | ||
Updated•15 years ago
|
Attachment #370141 -
Flags: review?(jduell.mcbugs)
Updated•15 years ago
|
Attachment #370141 -
Flags: review?(jduell.mcbugs) → review+
Comment 1•15 years ago
|
||
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!).
Assignee | ||
Comment 2•15 years ago
|
||
mfinkle, can you test with this patch when you get back into town?
Assignee | ||
Updated•15 years ago
|
Attachment #370141 -
Flags: review?(bugmail)
Assignee | ||
Comment 3•15 years ago
|
||
Comment on attachment 370141 [details] [diff] [review] patch v.1 brad, can you look at the wince pieces again.
Comment 4•15 years ago
|
||
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-
Assignee | ||
Comment 5•15 years ago
|
||
Attachment #370141 -
Attachment is obsolete: true
Attachment #370730 -
Flags: review?(bugmail)
Assignee | ||
Comment 6•15 years ago
|
||
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.
Updated•15 years ago
|
Attachment #370730 -
Flags: review?(bugmail) → review+
Comment 7•15 years ago
|
||
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
Assignee | ||
Updated•15 years ago
|
Attachment #370730 -
Flags: superreview?(pavlov)
Assignee | ||
Updated•15 years ago
|
tracking-fennec: --- → ?
Updated•15 years ago
|
tracking-fennec: ? → 1.0a1-wm+
Assignee | ||
Comment 8•15 years ago
|
||
mfinkle, could you try this patch ?
Assignee | ||
Comment 9•15 years ago
|
||
mfinkle said that this works. woot.
Comment 10•15 years ago
|
||
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-
Comment 11•15 years ago
|
||
(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.
Assignee | ||
Comment 12•15 years ago
|
||
regarding ConnMgrConnectionStatus performance, see comment #6.
Updated•15 years ago
|
Attachment #370730 -
Flags: superreview- → superreview+
Assignee | ||
Comment 13•15 years ago
|
||
see 488140 for the name change.
Comment 14•15 years ago
|
||
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.
Description
•