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.
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).
Comment on attachment 14903 [details] Add setClient to ResourceHandle, use it, check for NULL r=me
Comment on attachment 14903 [details] Add setClient to ResourceHandle, use it, check for NULL This doesn't apply cleanly on ToT anymore
Comment on attachment 14903 [details] Add setClient to ResourceHandle, use it, check for NULL Can we get a refreshed patch that applies cleanly please?
(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...
Created attachment 15670 [details] New patch that applies cleanly This is the same patch (it got R+ before) but updated to apply cleanly.
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.
Committed revision 24624.
Thanks for the fixes Darin!
<rdar://problem/5380658>