Bug 14935 – ResourceLoader should call ResourceHandle::setClient(0) after checking it's still the ResourceHandle's client

• Status: RESOLVED
• Resolution: INVALID
• Component: Page Loading

[Anyang Ren]

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.

[Anyang Ren]

Created attachment 15914 [details]
Verify we're still the ResourceHandle's client before clearing it out

[David Kilzer (:ddkilzer)]

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

[Darin Adler]

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.

[Anyang Ren]

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.

[Adam Roben (:aroben)]

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

[Anyang Ren]

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.