Bug 33004 – NetworkStateNotifier ignores all state changes after the first

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Patrick R. Gansterer]

The current implementation fires the callback only when the network changes the first time.

[Patrick R. Gansterer]

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.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45584 [details] without any errors.

[Maciej Stachowiak]

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.

[Patrick R. Gansterer]

(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 :-(

[Steve Falkenburg]

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

[Adam Roben (:aroben)]

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()?

[Patrick R. Gansterer]

(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.

[Patrick R. Gansterer]

Created attachment 50104 [details]
The patch (removed destructor)

[David Levin]

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.

[Patrick R. Gansterer]

Created attachment 51673 [details]
The patch (removed unrelated changes)

[Adam Barth]

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.

[Patrick R. Gansterer]

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.

[Adam Roben (:aroben)]

(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.

[Adam Roben (:aroben)]

I have a patch for this.

[Adam Roben (:aroben)]

Created attachment 59256 [details]
Patch

[Anders Carlsson]

Comment on attachment 59256 [details]
Patch

r=me if you add a comment to addrChangeCallback!

[Adam Roben (:aroben)]

Committed r61554: <http://trac.webkit.org/changeset/61554>

[WebKit Review Bot]

http://trac.webkit.org/changeset/61554 might have broken Tiger Intel Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/61553
http://trac.webkit.org/changeset/61554
http://trac.webkit.org/changeset/61555
http://trac.webkit.org/changeset/61556
http://trac.webkit.org/changeset/61557