Bug 14030 – Add a setClient method to ResourceHandle

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

[Brett Wilson (Google)]

ResourceHandle's ResourceHandleClient ownership is a bit tricky.

In the case of a server redirect to about blank (there is a LayoutTest for this), MainResourceLoader::continueAfterContentPolicy checks shouldLoadAsEmptyDocument() and internally calls didFinishLoading, skipping the normal ResourceHandle methods and releasing the reference to the ResourceHandle (in ResourceLoader::releaseResources).

It counts on the fact that it is the only owner of a reference to the ResourceHandle, such that it will be Release()d at this time, and that releasing will cancel the request. This is fragile and confusing. If somebody else happens to be holding a reference, the m_client member of the ResourceHandle will be invalid, and further callbacks will crash.

In addition, some implementations want to be able to change the client, although this may not be necessary in the current Safari embedding on Mac. We can solve both of these problems by adding a setClient method on ResourceHandle. ResourceLoader::releaseResources will call setClient(NULL) before releasing the handle, eliminating the possibility of invalid pointers in the event another reference is owned. It also allows other implementations to re-route resource requests, e.g. for downloads. On IRC, Maciej seemed to think this was an acceptable approach.

[Brett Wilson (Google)]

Created attachment 14903 [details]
Add setClient to ResourceHandle, use it, check for NULL

This simple patch just adds and implements setClient on ResourceHandle. I added setClient(NULL) in ResourceLoader before the handle is released, which is the important part (so the handle doesn't try to call back on the unowned pointer if it lives beyond this release).

I also added NULL checks before the client is accessed. I ended up changing the Mac (many cases) and CFNet (sometimes it was already checking, so I only needed a few) ports. The Qt port seems to reliable check for NULL already. I am happy to make a new patch without any of these port changes if you would prefer, I just wanted to try to reduce mysterious crashes down the like for the different embedders).

[Maciej Stachowiak]

Comment on attachment 14903 [details]
Add setClient to ResourceHandle, use it, check for NULL

r=me

[Matt Lilek]

Comment on attachment 14903 [details]
Add setClient to ResourceHandle, use it, check for NULL

This doesn't apply cleanly on ToT anymore

[Mark Rowe (bdash)]

Comment on attachment 14903 [details]
Add setClient to ResourceHandle, use it, check for NULL

Can we get a refreshed patch that applies cleanly please?

[Brett Wilson (Google)]

(In reply to comment #4)
> (From update of attachment 14903 [details] [edit])
> Can we get a refreshed patch that applies cleanly please?

Working on it...

[Brett Wilson (Google)]

Created attachment 15670 [details]
New patch that applies cleanly

This is the same patch (it got R+ before) but updated to apply cleanly.

[Darin Adler]

Comment on attachment 15670 [details]
New patch that applies cleanly

+    if (m_handle.get())

Don't need a .get() on the line above.

+        m_handle->setClient(NULL);

We normally use 0 for such things, not NULL.

+    // The client may be NULL, in which case no callbacks will be made.

We don't call this "NULL".

r=me since Maciej reviewed this previously and it looks fine.

[Darin Adler]

Committed revision 24624.

[Brett Wilson (Google)]

Thanks for the fixes Darin!

[Mark Rowe (bdash)]

<rdar://problem/5380658>