Bug 46187

Summary: Add internetStatusCallback to ResourceHandleWin
Product: WebKit Reporter: Patrick R. Gansterer <paroga>
Component: PlatformAssignee: Patrick R. Gansterer <paroga>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, commit-queue
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows XP   
Bug Depends on:    
Bug Blocks: 43712    
Attachments:
Description Flags
Patch
none
Patch (removed CALLBACK in cpp) none

Patrick R. Gansterer
Reported 2010-09-21 08:01:28 PDT
see patch
Attachments
Patch (4.18 KB, patch)
2010-09-21 08:34 PDT, Patrick R. Gansterer
no flags
Patch (removed CALLBACK in cpp) (4.27 KB, patch)
2010-09-23 06:55 PDT, Patrick R. Gansterer
no flags
Patrick R. Gansterer
Comment 1 2010-09-21 08:34:44 PDT
Patrick R. Gansterer
Comment 2 2010-09-21 08:44:14 PDT
> 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 3 2010-09-23 06:46:11 PDT
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
Comment 4 2010-09-23 06:48:21 PDT
(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
Comment 5 2010-09-23 06:55:01 PDT
Created attachment 68512 [details] Patch (removed CALLBACK in cpp)
Adam Roben (:aroben)
Comment 6 2010-09-23 07:05:49 PDT
(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
Comment 7 2010-09-23 07:33:04 PDT
(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 8 2010-09-23 08:39:37 PDT
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
Comment 9 2010-09-23 08:39:42 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.