Bug 33004 - NetworkStateNotifier ignores all state changes after the first
Summary: NetworkStateNotifier ignores all state changes after the first
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Adam Roben (:aroben)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-28 18:44 PST by Patrick R. Gansterer
Modified: 2010-06-21 12:34 PDT (History)
6 users (show)

See Also:


Attachments
The patch (6.57 KB, patch)
2009-12-28 18:48 PST, Patrick R. Gansterer
aroben: review-
Details | Formatted Diff | Diff
The patch (removed destructor) (5.94 KB, patch)
2010-03-05 10:44 PST, Patrick R. Gansterer
levin: review-
Details | Formatted Diff | Diff
The patch (removed unrelated changes) (3.30 KB, patch)
2010-03-25 13:28 PDT, Patrick R. Gansterer
abarth: review-
Details | Formatted Diff | Diff
Patch (now with WTF::createThread) (3.35 KB, patch)
2010-06-20 12:09 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch (1.77 KB, patch)
2010-06-21 09:28 PDT, Adam Roben (:aroben)
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick R. Gansterer 2009-12-28 18:44:48 PST
The current implementation fires the callback only when the network changes the first time.
Comment 1 Patrick R. Gansterer 2009-12-28 18:48:40 PST
Created attachment 45584 [details]
The patch

This patch creates a new thread only for waiting for a network change. Is there an other possibility to something like that already? Is it allowed to start a thread here?
This patch also closes the handles.
Comment 2 WebKit Review Bot 2009-12-28 18:54:00 PST
style-queue ran check-webkit-style on attachment 45584 [details] without any errors.
Comment 3 Maciej Stachowiak 2009-12-28 20:21:02 PST
Comment on attachment 45584 [details]
The patch

It's a little unfortunate to make a special thread just for this purpose. Is there another way to do it?

I'm also concerned about the amount of work being done in the destructor.
Comment 4 Patrick R. Gansterer 2009-12-28 21:41:57 PST
(In reply to comment #3)
> It's a little unfortunate to make a special thread just for this purpose. Is
> there another way to do it?
I tried it with the RegisterWaitForSingleObject but i didn't get it running. UnregisterWait must be called _after_ the _finished_ callback, otherwise it will return ERROR_IO_PENDING. The missing UnregisterWait seams to be the reason why it only works the first time. Even RegisterWaitForSingleObject isn't aviable unter WinCE.

> I'm also concerned about the amount of work being done in the destructor.
I'm too, but i didn't find a better place :-(
Comment 5 Steve Falkenburg 2010-01-05 14:10:37 PST
Using WTF threading calls instead of calling CreateThread() directly would be better, since that calls _beginthreadex where available. See http://support.microsoft.com/kb/104641
Comment 6 Adam Roben (:aroben) 2010-01-05 14:47:13 PST
Comment on attachment 45584 [details]
The patch

> +NetworkStateNotifier::~NetworkStateNotifier()
> +{
> +    m_closeAddrChangeWaitThread = true;
> +    ::CloseHandle(m_overlapped.hEvent);
> +    ::WaitForSingleObject(m_waitHandle, INFINITE);
> +    ::CloseHandle(m_waitHandle);
>  }

NetworkStateNotifier is a singleton that is never destroyed. This code will never be called.

It seems like the real fix here is to call NotifyAddrChange again after we receive a notification. Why can't we just add another call to NotifyAddrChange/RegisterWaitForSingleObject inside addressChanged()?
Comment 7 Patrick R. Gansterer 2010-01-06 01:57:13 PST
(In reply to comment #6)
> NetworkStateNotifier is a singleton that is never destroyed. This code will
> never be called.
I'll remove the destructor.

> It seems like the real fix here is to call NotifyAddrChange again after we
> receive a notification. Why can't we just add another call to
> NotifyAddrChange/RegisterWaitForSingleObject inside addressChanged()?
To register again you must unregister first. This can only be done outside of the callback, because UnregisterWait will return ERROR_IO_PENDING otherwise.
Maybe there is a working solution with RegisterWaitForSingleObject, but i didn't tried it very hard, because RegisterWaitForSingleObject isn't available on WinCE anyway.
Comment 8 Patrick R. Gansterer 2010-03-05 10:44:19 PST
Created attachment 50104 [details]
The patch (removed destructor)
Comment 9 David Levin 2010-03-25 11:21:51 PDT
Comment on attachment 50104 [details]
The patch (removed destructor)

There are so many whitespaces (and perhaps newline?) changes in here that it is hard to review/figure out what changed. Please revert the unnecessary parts of the change (even if it means using a different editor than you usually do) and keep the change focused on the fix.

Also, there are changes to remove "onLine()" and "setNetworkAccessAllowed()" which is only there for QT and these seem totally unrelated to the fix.

So r- due to too many unrelated changes.
Comment 10 Patrick R. Gansterer 2010-03-25 13:28:35 PDT
Created attachment 51673 [details]
The patch (removed unrelated changes)
Comment 11 Adam Barth 2010-06-20 09:07:02 PDT
Comment on attachment 51673 [details]
The patch (removed unrelated changes)

You don't seem to have incorporated previous feedback on your patch.  In particular, you're still using ::CreateThread instead of WTF threads, as requested in Comment #5.
Comment 12 Patrick R. Gansterer 2010-06-20 12:09:56 PDT
Created attachment 59208 [details]
Patch (now with WTF::createThread)

(In reply to comment #11)
> You don't seem to have incorporated previous feedback on your patch.  In particular, you're still using ::CreateThread instead of WTF threads, as requested in Comment #5.
Done.
Comment 13 Adam Roben (:aroben) 2010-06-21 09:15:29 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > It seems like the real fix here is to call NotifyAddrChange again after we
> > receive a notification. Why can't we just add another call to
> > NotifyAddrChange/RegisterWaitForSingleObject inside addressChanged()?
> To register again you must unregister first. This can only be done outside of the callback, because UnregisterWait will return ERROR_IO_PENDING otherwise.
> Maybe there is a working solution with RegisterWaitForSingleObject, but i didn't tried it very hard, because RegisterWaitForSingleObject isn't available on WinCE anyway.

It looks like we don't actually need to call RegisterWaitForSingleObject. Unless you specify WT_EXECUTEONLYONCE, RegisterWaitForSingleObject will continue waiting and calling your callback every time the object is signalled (until UnregisterWait is called). So all we need to do is call NotifyAddrChanged again every time our callback is called.
Comment 14 Adam Roben (:aroben) 2010-06-21 09:15:51 PDT
I have a patch for this.
Comment 15 Adam Roben (:aroben) 2010-06-21 09:28:49 PDT
Created attachment 59256 [details]
Patch
Comment 16 Anders Carlsson 2010-06-21 09:29:51 PDT
Comment on attachment 59256 [details]
Patch

r=me if you add a comment to addrChangeCallback!
Comment 17 Adam Roben (:aroben) 2010-06-21 10:45:58 PDT
Committed r61554: <http://trac.webkit.org/changeset/61554>