Bug 14030 - Add a setClient method to ResourceHandle
Summary: Add a setClient method to ResourceHandle
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 523.x (Safari 3)
Hardware: All All
: P2 Enhancement
Assignee: Brett Wilson (Google)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2007-06-07 13:39 PDT by Brett Wilson (Google)
Modified: 2007-08-02 09:20 PDT (History)
0 users

See Also:


Attachments
Add setClient to ResourceHandle, use it, check for NULL (9.82 KB, patch)
2007-06-07 14:32 PDT, Brett Wilson (Google)
mrowe: review-
Details | Formatted Diff | Diff
New patch that applies cleanly (11.98 KB, patch)
2007-07-24 13:34 PDT, Brett Wilson (Google)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brett Wilson (Google) 2007-06-07 13:39:08 PDT
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.
Comment 1 Brett Wilson (Google) 2007-06-07 14:32:17 PDT
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 2 Maciej Stachowiak 2007-06-26 00:26:35 PDT
Comment on attachment 14903 [details]
Add setClient to ResourceHandle, use it, check for NULL

r=me
Comment 3 Matt Lilek 2007-06-26 16:08:36 PDT
Comment on attachment 14903 [details]
Add setClient to ResourceHandle, use it, check for NULL

This doesn't apply cleanly on ToT anymore
Comment 4 Mark Rowe (bdash) 2007-06-26 22:47:28 PDT
Comment on attachment 14903 [details]
Add setClient to ResourceHandle, use it, check for NULL

Can we get a refreshed patch that applies cleanly please?
Comment 5 Brett Wilson (Google) 2007-07-24 13:03:32 PDT
(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...
Comment 6 Brett Wilson (Google) 2007-07-24 13:34:18 PDT
Created attachment 15670 [details]
New patch that applies cleanly

This is the same patch (it got R+ before) but updated to apply cleanly.
Comment 7 Darin Adler 2007-07-24 14:15:04 PDT
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.
Comment 8 Darin Adler 2007-07-25 09:57:36 PDT
Committed revision 24624.
Comment 9 Brett Wilson (Google) 2007-07-25 14:03:26 PDT
Thanks for the fixes Darin!
Comment 10 Mark Rowe (bdash) 2007-08-02 09:20:14 PDT
<rdar://problem/5380658>