Bug 46187 – Add internetStatusCallback to ResourceHandleWin

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Patrick R. Gansterer]

see patch

[Patrick R. Gansterer]

Created attachment 68241 [details]
Patch

[Patrick R. Gansterer]

> WebCore/platform/network/win/ResourceHandleWin.cpp:297
>  void ResourceHandle::onRequestComplete(LPARAM lParam)
Will be changed to "bool onRequestComplete(void)" in a further patch (The file doesn't compile anyway at the moment).

[Adam Roben (:aroben)]

Comment on attachment 68241 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68241&action=review

> WebCore/platform/network/win/ResourceHandleWin.cpp:266
> +void CALLBACK ResourceHandle::internetStatusCallback(HINTERNET internetHandle, DWORD_PTR context, DWORD internetStatus,

No need for "CALLBACK" here. It's only needed on the declaration.

> WebCore/platform/network/win/ResourceHandleWin.cpp:279
> +    switch (internetStatus) {
> +    case INTERNET_STATUS_REDIRECT:
> +        handle->d->m_redirectUrl = String(static_cast<UChar*>(statusInformation), statusInformationLength);
> +        callOnMainThread(callOnRedirect, handle);
> +        break;
> +
> +    case INTERNET_STATUS_REQUEST_COMPLETE:
> +        callOnMainThread(callOnRequestComplete, handle);
> +        break;

What guarantees that the ResourceHandle hasn't been deleted by the time callOnRedirect/callOnRequestComplete are called on the main thread? For that matter, what guarantees that the ResourceHandle hasn't been deleted by the time internetStatusCallback is called?

[Patrick R. Gansterer]

(In reply to comment #3)
> (From update of attachment 68241 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68241&action=review
> 
> > WebCore/platform/network/win/ResourceHandleWin.cpp:266
> > +void CALLBACK ResourceHandle::internetStatusCallback(HINTERNET internetHandle, DWORD_PTR context, DWORD internetStatus,
> 
> No need for "CALLBACK" here. It's only needed on the declaration.
> 
> > WebCore/platform/network/win/ResourceHandleWin.cpp:279
> > +    switch (internetStatus) {
> > +    case INTERNET_STATUS_REDIRECT:
> > +        handle->d->m_redirectUrl = String(static_cast<UChar*>(statusInformation), statusInformationLength);
> > +        callOnMainThread(callOnRedirect, handle);
> > +        break;
> > +
> > +    case INTERNET_STATUS_REQUEST_COMPLETE:
> > +        callOnMainThread(callOnRequestComplete, handle);
> > +        break;
> 
> What guarantees that the ResourceHandle hasn't been deleted by the time callOnRedirect/callOnRequestComplete are called on the main thread? For that matter, what guarantees that the ResourceHandle hasn't been deleted by the time internetStatusCallback is called?
ref() in start() will do that: http://trac.webkit.org/browser/trunk/WebCore/platform/network/win/ResourceHandleWin.cpp#L438

[Patrick R. Gansterer]

Created attachment 68512 [details]
Patch (removed CALLBACK in cpp)

[Adam Roben (:aroben)]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 68241 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=68241&action=review
> > 
> > > WebCore/platform/network/win/ResourceHandleWin.cpp:266
> > > +void CALLBACK ResourceHandle::internetStatusCallback(HINTERNET internetHandle, DWORD_PTR context, DWORD internetStatus,
> > 
> > No need for "CALLBACK" here. It's only needed on the declaration.
> > 
> > > WebCore/platform/network/win/ResourceHandleWin.cpp:279
> > > +    switch (internetStatus) {
> > > +    case INTERNET_STATUS_REDIRECT:
> > > +        handle->d->m_redirectUrl = String(static_cast<UChar*>(statusInformation), statusInformationLength);
> > > +        callOnMainThread(callOnRedirect, handle);
> > > +        break;
> > > +
> > > +    case INTERNET_STATUS_REQUEST_COMPLETE:
> > > +        callOnMainThread(callOnRequestComplete, handle);
> > > +        break;
> > 
> > What guarantees that the ResourceHandle hasn't been deleted by the time callOnRedirect/callOnRequestComplete are called on the main thread? For that matter, what guarantees that the ResourceHandle hasn't been deleted by the time internetStatusCallback is called?
> ref() in start() will do that: http://trac.webkit.org/browser/trunk/WebCore/platform/network/win/ResourceHandleWin.cpp#L438

The ref() in start() will probably help, but not without knowing where the corresponding deref() happens. I see in attachment 68285 [details] that it's going to happen on onRequestComplete. But what happens if the request gets cancelled? E.g., it could get cancelled while we're inside internetStatusCallback, or between when internetStatusCallback returns and callOnRedirect is called, etc. Do we need to worry about those cases?

[Patrick R. Gansterer]

(In reply to comment #6)
> The ref() in start() will probably help, but not without knowing where the corresponding deref() happens. I see in attachment 68285 [details] that it's going to happen on onRequestComplete. But what happens if the request gets cancelled? E.g., it could get cancelled while we're inside internetStatusCallback, or between when internetStatusCallback returns and callOnRedirect is called, etc. Do we need to worry about those cases?
A cancel() does not deref(), it will only close the handle and set it to null. deref() will happen when onRequestComplete() is called from MainThread.
see in the "complete" version: http://gitorious.org/+wincewebkit-developers/webkit/wincewebkit/blobs/wincegdi/WebCore/platform/network/win/ResourceHandleWin.cpp#line209

[WebKit Commit Bot]

Comment on attachment 68512 [details]
Patch (removed CALLBACK in cpp)

Clearing flags on attachment: 68512

Committed r68144: <http://trac.webkit.org/changeset/68144>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.