Revision 24624 added ResourceHandle::setClient(). The only call to this new method is in ResourceLoader::releaseResources, and it passes 0 as the 'client' argument. So it's not clear whether ResourceHandle::setClient() is intended to be called with a non-NULL 'client' argument. That would allow someone else to change the client of the ResourceHandle created by ResourceLoader. When that happens, the m_handle->setClient(0) call in ResourceLoader::releaseResources() would be messing with this other client, preventing the ResourceHandle from calling back to this other client. I think ResourceHandle::setClient() needs to be changed to only accept a null 'client' argument (or renamed ResourceHandle::clearClient with no argument), or ResourceLoader::releaseResources() needs to validate its assumption that it is still the ResourceHandle's client before calling m_handle->setClient(0). I will attach a patch that does the latter.
Created attachment 15914 [details] Verify we're still the ResourceHandle's client before clearing it out
Comment on attachment 15914 [details] Verify we're still the ResourceHandle's client before clearing it out In the future, please set the review? flag on patches. Thanks! At minimum, this patch will also need a ChangeLog entry. (I'm leaving it as review? so that the patch will be reviewed.) See this page for details: http://webkit.org/coding/contributing.html
Comment on attachment 15914 [details] Verify we're still the ResourceHandle's client before clearing it out I don't think there's any reason to make this code change. It's only needed if ResourceLoader shares a ResourceHandle with another client. That's never done, so we don't need the code. Unless there's something I'm missing and this actually happens.
This can happen if a WebKit embedder calls ResourceLoader::handle(), a public method, and then call ResourceHandle::setClient on the returned ResourceHandle to set its client to something else. I'd like to know if this usage of ResourceHandle::setClient is allowed. If not, perhaps ResourceHandle::setClient should be constrained to prevent a WebKit embedder from making this mistake. I just wanted to bring this issue to your attention. Please feel free to mark this bug invalid or wontfix after you have considered this issue. Thank you for your quick response.
(In reply to comment #4) > This can happen if a WebKit embedder calls ResourceLoader::handle(), > a public method, and then call ResourceHandle::setClient on the > returned ResourceHandle to set its client to something else. I'd > like to know if this usage of ResourceHandle::setClient is allowed. > If not, perhaps ResourceHandle::setClient should be constrained > to prevent a WebKit embedder from making this mistake. I don't think there's a way to make this happen through the WebKit API, so I don't think it's possible for a "WebKit embedder" to get into this situation. Perhaps you're talking about someone who is making changes to WebCore/WebKit itself? If so, seeing the patches that are running into this situation would make this whole discussion much clearer.
Adam, sorry about the late reply. Yes, I was experimenting with calling handle->setClient() in WebFrameLoaderClient::download to set the ResourceHandle's client to some other object that handles the download. But I don't need to do that any more. So I'm closing this bug.