Bug 61006 - REGRESSION (r39725?): Resources removed from document can not be freed until the document is deleted
Summary: REGRESSION (r39725?): Resources removed from document can not be freed until ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Scott Graham
URL:
Keywords: InRadar
Depends on: 22994
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-17 17:17 PDT by Eric Seidel (no email)
Modified: 2011-08-01 14:22 PDT (History)
25 users (show)

See Also:


Attachments
test cases that loads images via new Image (834 bytes, text/html)
2011-05-17 17:25 PDT, James Robinson
no flags Details
Patch (9.23 KB, patch)
2011-06-01 20:20 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (11.93 KB, patch)
2011-06-01 20:21 PDT, James Robinson
no flags Details | Formatted Diff | Diff
merge up to ToT and remove bogus protocolIsData check from CachedResourceLoader::requestResource (11.76 KB, patch)
2011-06-02 13:38 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (14.81 KB, patch)
2011-07-14 09:59 PDT, Scott Graham
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (5.75 MB, application/zip)
2011-07-14 11:43 PDT, WebKit Review Bot
no flags Details
Update to fix recently added inspector tests by updating m_documentResources in cache with all new responses (14.60 KB, patch)
2011-07-15 08:04 PDT, Scott Graham
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (5.76 MB, application/zip)
2011-07-15 08:43 PDT, WebKit Review Bot
no flags Details
run a garbage collection on m_documentResources when a load finishes (4.05 KB, patch)
2011-07-15 14:43 PDT, Scott Graham
no flags Details | Formatted Diff | Diff
corrections per comments (5.36 KB, patch)
2011-07-18 08:22 PDT, Scott Graham
no flags Details | Formatted Diff | Diff
rollback, pending crash investigation (4.02 KB, patch)
2011-07-25 12:22 PDT, Scott Graham
no flags Details | Formatted Diff | Diff
rollback, pending crash investigation (4.65 KB, patch)
2011-07-25 13:56 PDT, Scott Graham
no flags Details | Formatted Diff | Diff
GC patch updated with test and fix for crash (11.22 KB, patch)
2011-07-26 09:56 PDT, Scott Graham
no flags Details | Formatted Diff | Diff
GC patch updated with fix and test using window.internals (14.07 KB, patch)
2011-07-27 13:22 PDT, Scott Graham
no flags Details | Formatted Diff | Diff
Patch, the same as the previous one for mac bot. (14.07 KB, patch)
2011-07-27 15:25 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
further update to export symbols for test harness (17.48 KB, patch)
2011-07-27 20:56 PDT, Scott Graham
no flags Details | Formatted Diff | Diff
fix export symbols for test harness (19.50 KB, patch)
2011-07-28 10:04 PDT, Scott Graham
no flags Details | Formatted Diff | Diff
fix sources.filter for gtk port (19.50 KB, patch)
2011-07-28 13:41 PDT, Scott Graham
no flags Details | Formatted Diff | Diff
update based on comments (19.23 KB, patch)
2011-08-01 10:05 PDT, Scott Graham
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2011-05-17 17:17:28 PDT
REGRESSION(r39725?) : All CachedResources are leaked

This has been in the tree for a long time, I'm surprised we've not noticed sooner.

I believe that http://trac.webkit.org/changeset/39725 introduced a leak of all CachedResources.

m_documentResources holds a CachedResourceHandle:
    typedef HashMap<String, CachedResourceHandle<CachedResource> > DocumentResourceMap;

The only time that a CachedResource is ever destroyed is when its handle count hits 0:

    bool canDelete() const { return !hasClients() && !m_request && !m_preloadCount && !m_handleCount && !m_resourceToRevalidate && !m_proxyResource; }
http://trac.webkit.org/browser/trunk/Source/WebCore/loader/cache/CachedResource.h#L176

But the only way to ever be removed from this map is from:
void CachedResourceLoader::removeCachedResource(CachedResource* resource) const

Which is only ever called from ~CachedResource().

This has been reported various times to Chromium (but affects all webkit implementations):
http://code.google.com/p/chromium/issues/detail?id=36142
Comment 1 James Robinson 2011-05-17 17:25:44 PDT
Created attachment 93850 [details]
test cases that loads images via new Image

This test case loads 50 512x512 images via new Image().src = "data:...".  This causes a memory growth of ~400MB.

Since the image is 512x512x4 bytes/pixel, the data URL size is 512*512*4 * 4/3 (base64) * 2 (utf-16 encoding) = 2666kb/image.  Given 50 loads this should be a memory use of 133MB, which means that we are leaking 3 copies of the URL per load.  I imagine that we're doing deep string copies in a few places that perhaps we don't mean to, in addition to leaking it.
Comment 2 James Robinson 2011-05-17 19:42:35 PDT
Looking at a memory profile more closely I think we're actually leaking 2 KURLs, each of which has one WTF::String which holds the data url as utf-16 data and holds on to one native representation of the string, which is stored in an encoding that uses one byte/character for this data (which is all ascii).  In chromium that's a UTF-8 encoded CString, on the mac this is some sort of NSUrl or CF networking data structure.  2 UTF-16 copies + 2 utf-8/ascii copies = the 6x memory blowup we're seeing.  I think fixing the CachedResource leak is likely to fix both of these.
Comment 3 David Kilzer (:ddkilzer) 2011-05-17 21:39:10 PDT
<rdar://problem/9458420>
Comment 4 Antti Koivisto 2011-05-18 03:53:36 PDT
There is no "leak". The resources are freed when the document goes away. The bug is that if page does dynamic adding and removing of resources, the removed resources won't be freed until the document is.

Re-titled appropriately.
Comment 5 Eric Seidel (no email) 2011-05-18 11:11:31 PDT
OK.  I believe this is still a regression, and still seems like it would affect basically every site. :)

The good news is that because our decoded data disposal is good, we don't really notice that we leak every dynamic resource.  We only notice when the URLs for those resources are huge.
Comment 6 James Robinson 2011-05-26 21:52:48 PDT
Related bug: https://bugs.webkit.org/show_bug.cgi?id=61604
Comment 7 James Robinson 2011-06-01 20:20:26 PDT
Created attachment 95710 [details]
Patch
Comment 8 James Robinson 2011-06-01 20:20:48 PDT
Comment on attachment 95710 [details]
Patch

Whoops, no ChangeLog...
Comment 9 James Robinson 2011-06-01 20:21:31 PDT
Created attachment 95711 [details]
Patch
Comment 10 Eric Seidel (no email) 2011-06-01 21:14:10 PDT
Comment on attachment 95711 [details]
Patch

Makes sense to me.  I'm not sure why the "owner" was ever just a pointer, since multiple loaders can be loading the same resource at the same time.
Comment 11 Alexey Proskuryakov 2011-06-01 23:20:53 PDT
Comment on attachment 95711 [details]
Patch

Can we give a loader expert (Antti) a chance to comment before landing?
Comment 12 Alexey Proskuryakov 2011-06-01 23:22:33 PDT
(or Brady)
Comment 13 James Robinson 2011-06-01 23:26:21 PDT
+cc Brady

Sure, I'd appreciate more input from anyone who feels familiar with this code.
Comment 14 Brady Eidson 2011-06-02 08:46:55 PDT
And CC'ing Antti, he really needs to look at this.
Comment 15 Brady Eidson 2011-06-02 08:48:30 PDT
Also, I'd be somewhat surprised if it was *NOT* possible to write a test case for this.

Note that making another test pass is great, but if there's no test to actually test the new behavior intended by this patch directly, it could still regress back later.
Comment 16 James Robinson 2011-06-02 11:17:30 PDT
(In reply to comment #15)
> Also, I'd be somewhat surprised if it was *NOT* possible to write a test case for this.
> 
> Note that making another test pass is great, but if there's no test to actually test the new behavior intended by this patch directly, it could still regress back later.

Can you suggest what sort of test you'd like to see?  I don't expect any direct change in behavior, just a smaller memory footprint.  I don't know how to write a layout test for that.
Comment 17 Brady Eidson 2011-06-02 11:31:05 PDT
(In reply to comment #16)
> (In reply to comment #15)
> > Also, I'd be somewhat surprised if it was *NOT* possible to write a test case for this.
> > 
> > Note that making another test pass is great, but if there's no test to actually test the new behavior intended by this patch directly, it could still regress back later.
> 
> Can you suggest what sort of test you'd like to see?  I don't expect any direct change in behavior, just a smaller memory footprint.  I don't know how to write a layout test for that.

Hmmm I suppose the only thing missing is a callback that a cached resource is actually destroyed.

But using resource load callbacks it might be possible to tell if a resource is loaded from the network layer or the WebCore memory cache.  If that's possible, I would start exploring with a test involving a subresource in a dynamic document, adding and/or removing dynamically to try and trigger it being loaded from the network layer again, instead of from the WebCore memory cache.

Thinking about it more, I would no longer be surprised if a test wasn't possible.

But I'd still *not* be surprised if a test *is* possible.
Comment 18 James Robinson 2011-06-02 11:40:27 PDT
(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > Also, I'd be somewhat surprised if it was *NOT* possible to write a test case for this.
> > > 
> > > Note that making another test pass is great, but if there's no test to actually test the new behavior intended by this patch directly, it could still regress back later.
> > 
> > Can you suggest what sort of test you'd like to see?  I don't expect any direct change in behavior, just a smaller memory footprint.  I don't know how to write a layout test for that.
> 
> Hmmm I suppose the only thing missing is a callback that a cached resource is actually destroyed.
> 
> But using resource load callbacks it might be possible to tell if a resource is loaded from the network layer or the WebCore memory cache.  If that's possible, I would start exploring with a test involving a subresource in a dynamic document, adding and/or removing dynamically to try and trigger it being loaded from the network layer again, instead of from the WebCore memory cache.
> 

I'm pretty sure I'm not changing the semantics of the memory cache - no CachedResource should be deleted while it's in the cache, so this will only allow freeing CachedResources that are already evicted from the memory cache so loads will have to hit the network.


> Thinking about it more, I would no longer be surprised if a test wasn't possible.
> 
> But I'd still *not* be surprised if a test *is* possible.

I'd be much happier with a test as well, of course, but I can't think of one :)
Comment 19 Brady Eidson 2011-06-02 12:15:00 PDT
(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > (In reply to comment #15)
> > > > Also, I'd be somewhat surprised if it was *NOT* possible to write a test case for this.
> > > > 
> > > > Note that making another test pass is great, but if there's no test to actually test the new behavior intended by this patch directly, it could still regress back later.
> > > 
> > > Can you suggest what sort of test you'd like to see?  I don't expect any direct change in behavior, just a smaller memory footprint.  I don't know how to write a layout test for that.
> > 
> > Hmmm I suppose the only thing missing is a callback that a cached resource is actually destroyed.
> > 
> > But using resource load callbacks it might be possible to tell if a resource is loaded from the network layer or the WebCore memory cache.  If that's possible, I would start exploring with a test involving a subresource in a dynamic document, adding and/or removing dynamically to try and trigger it being loaded from the network layer again, instead of from the WebCore memory cache.
> > 
> 
> I'm pretty sure I'm not changing the semantics of the memory cache - no CachedResource should be deleted while it's in the cache, so this will only allow freeing CachedResources that are already evicted from the memory cache so loads will have to hit the network.
> 


If I understand the bug you reported that your patch purports to fix, then:

Before your patch:
- A document has a resource in it, and that document is the only one with a reference to that resource.
- Removing that resource from the document will not immediately free the associated CachedResource.
- Only later - when the document itself is destroyed - will this CachedResource actually be freed.
- So before that Document is destroyed, if you then loaded a *new* document with this same resource, the resource would presumably be loaded from the memory Cache.

After your patch:
- A document has a resource in it, and that document is the only one with a reference to that resource.
- Removing that resource from the document *will* immediately destroy the CachedResource, even if the Document hangs around forever.
- If you then load a *new* document with this same resource, the resource would presumably be loaded from the network layer.

Is this description correct?

If so, it should be possible to test with resource load callbacks.  There is a resource load callback in WebKit that specifies a resource loaded from the memory cache, as opposed to a normal network load (WebFrameLoaderClient::dispatchDidLoadResourceFromMemoryCache)

DumpRenderTree might not listen to this right now, but it could easily.  And your patch would cause a change in behavior for this scenario that could be tested.

*HOWEVER* - In writing out the above description, I noticed something I don't like.  If a resource is removed from a document, then immediately freed, then later added back in to that document, that'd be extremely wasteful - including for these huge data URLs we're concerned with.
Comment 20 James Robinson 2011-06-02 12:46:38 PDT
(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > (In reply to comment #16)
> > > > (In reply to comment #15)
> > > > > Also, I'd be somewhat surprised if it was *NOT* possible to write a test case for this.
> > > > > 
> > > > > Note that making another test pass is great, but if there's no test to actually test the new behavior intended by this patch directly, it could still regress back later.
> > > > 
> > > > Can you suggest what sort of test you'd like to see?  I don't expect any direct change in behavior, just a smaller memory footprint.  I don't know how to write a layout test for that.
> > > 
> > > Hmmm I suppose the only thing missing is a callback that a cached resource is actually destroyed.
> > > 
> > > But using resource load callbacks it might be possible to tell if a resource is loaded from the network layer or the WebCore memory cache.  If that's possible, I would start exploring with a test involving a subresource in a dynamic document, adding and/or removing dynamically to try and trigger it being loaded from the network layer again, instead of from the WebCore memory cache.
> > > 
> > 
> > I'm pretty sure I'm not changing the semantics of the memory cache - no CachedResource should be deleted while it's in the cache, so this will only allow freeing CachedResources that are already evicted from the memory cache so loads will have to hit the network.
> > 
> 
> 
> If I understand the bug you reported that your patch purports to fix, then:
> 
> Before your patch:
> - A document has a resource in it, and that document is the only one with a reference to that resource.

Which implies that the resource is not in the memory cache, since while it is in the memory cache then the DocumentLoader cannot be the sole reference.

> - Removing that resource from the document will not immediately free the associated CachedResource.
> - Only later - when the document itself is destroyed - will this CachedResource actually be freed.

Correct.

> - So before that Document is destroyed, if you then loaded a *new* document with this same resource, the resource would presumably be loaded from the memory Cache.

Not quite!  Presence in the memory cache is not the same thing as existence of the CachedResource.
> 
> After your patch:
> - A document has a resource in it, and that document is the only one with a reference to that resource.
> - Removing that resource from the document *will* immediately destroy the CachedResource, even if the Document hangs around forever.
> - If you then load a *new* document with this same resource, the resource would presumably be loaded from the network layer.
> 
> Is this description correct?
> 
> If so, it should be possible to test with resource load callbacks.  There is a resource load callback in WebKit that specifies a resource loaded from the memory cache, as opposed to a normal network load (WebFrameLoaderClient::dispatchDidLoadResourceFromMemoryCache)
> 
> DumpRenderTree might not listen to this right now, but it could easily.  And your patch would cause a change in behavior for this scenario that could be tested.
> 
> *HOWEVER* - In writing out the above description, I noticed something I don't like.  If a resource is removed from a document, then immediately freed, then later added back in to that document, that'd be extremely wasteful - including for these huge data URLs we're concerned with.

That's not what the patch intends - if the CachedResource is held in the memory cache before this patch then it still should be held alive after this patch.  This patch ensures that the CachedResource can be freed after the resource is evicted from the memory cache even if the DocumentLoader is still around.  A CachedResource can stick around in the memory cache even if there aren't any DocumentLoaders referencing it.
Comment 21 Brady Eidson 2011-06-02 13:28:58 PDT
Okay, think I understand now.  This is only about the URLs in CachedResource potentially being huge, not at all about the resources themselves.

Antti still needs to take a look at this.
Comment 22 James Robinson 2011-06-02 13:32:29 PDT
Yup, agree on both points.  Nate pointed out I have a spurious protocolIsData() check in here as well, I'll update the patch shortly.
Comment 23 James Robinson 2011-06-02 13:38:25 PDT
Created attachment 95800 [details]
merge up to ToT and remove bogus protocolIsData check from CachedResourceLoader::requestResource
Comment 24 James Robinson 2011-06-23 14:52:00 PDT
Friendly ping
Comment 25 Antti Koivisto 2011-06-25 08:32:24 PDT
The pointer help by CachedResourceHandle is updated automatically to point from the proxy resource to the validated one when a cache validation (304 response) succeeds (by CachedResource::switchClientsToRevalidatedResource()). 

By using plain pointers you will lose this mechanism and I don't see anything here that achieves the same effect. This probably means that m_documentResources will end up missing revalidated resources.

To fix this the revalidated resources could be inserted to the m_documentResources manually or maybe CachedResourceHandle could be added a flag that makes it weak.
Comment 26 Scott Graham 2011-07-13 18:02:52 PDT
I attempted to refine this patch to update m_documentResources for the 304 path.

However, if the map is updated from CachedResource::switchClientsToRevalidatedResource, then ~CachedResource for the proxy fails because it expects to find a pointer to itself in the map. I tried deferring the update until after the proxy is destroyed, but it would need to be deferred until after the request is complete as the proxy !canDelete() until then (which is pretty far back up the stack). And, it would need to save off and pass back a Set of CachedResourceLoaders as well as the resource to replace with, which seems ugly.

Naive next guess at a fix might be to flag the proxy resources as such so they don't try to remove themselves? 

Antti's suggestion of instead adding a weak flag to CachedResourceHandle may work better, but it also looks like it would need to touch a lot of places that might retrieve an invalidated weak pointer.
Comment 27 James Robinson 2011-07-13 18:06:08 PDT
The most interesting resources from a memory use perspective are large data: URLs, which never validate/304.  Is it possible to come up with a simpler solution for resources that we know will never have to validate?
Comment 28 Scott Graham 2011-07-14 09:59:23 PDT
Created attachment 100818 [details]
Patch

Add additional handling for 304 by tracking when a CachedResource is used as a proxy
Comment 29 Scott Graham 2011-07-14 10:04:21 PDT
Reviews/comments much wanted. This patch at ToT now reduces memory usage in the test case jamesr was using, but unfortunately there's unrelated code that's still hanging on to things for too long. Usage is around 250MB now, rather than 400, but it still shouldn't be that high.
Comment 30 WebKit Review Bot 2011-07-14 11:43:12 PDT
Comment on attachment 100818 [details]
Patch

Attachment 100818 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9059279

New failing tests:
http/tests/inspector/resource-tree/resource-tree-non-unique-url.html
http/tests/inspector/network/network-cachedresources-with-same-urls.html
Comment 31 WebKit Review Bot 2011-07-14 11:43:20 PDT
Created attachment 100835 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 32 Scott Graham 2011-07-15 08:04:18 PDT
Created attachment 100980 [details]
Update to fix recently added inspector tests by updating m_documentResources in cache with all new responses
Comment 33 WebKit Review Bot 2011-07-15 08:43:17 PDT
Comment on attachment 100980 [details]
Update to fix recently added inspector tests by updating m_documentResources in cache with all new responses

Attachment 100980 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9099257

New failing tests:
http/tests/inspector/resource-tree/resource-tree-reload.html
http/tests/inspector/network/network-content-replacement-xhr.html
Comment 34 WebKit Review Bot 2011-07-15 08:43:25 PDT
Created attachment 100989 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 35 Antti Koivisto 2011-07-15 10:12:49 PDT
This looks generally ok to me, though it is not nice to increase the complexity of the already convoluted code. 

However it occurred to me that it might be easier to simply garbage collect m_documentResources occasionally (everything held alive by m_documentResources only is garbage). Did you already reject that approach for some reason?
Comment 36 Scott Graham 2011-07-15 10:32:39 PDT
Thanks. I agree the patch does not make the code simpler to understand.

GC sounds like a reasonable idea. I guess the only obvious drawback is that it might take "a while" before the resources get freed. It would need only check the refcount of the CachedResource in m_documentResources and decref if 1, correct?

I guess the only tricky thing might be finding a suitable time to run that, to make sure there wasn't someone holding only a raw ptr. Could anyone suggest a suitable place to trigger that from?
Comment 37 Scott Graham 2011-07-15 14:43:03 PDT
Created attachment 101045 [details]
run a garbage collection on m_documentResources when a load finishes
Comment 38 Scott Graham 2011-07-15 14:47:43 PDT
This is much simpler and more localized, but a bit hacky (what if someone else also has a pseudo-weak map of CachedResourceHandles?). But, overall it's probably more straightforward and is a much smaller patch.

The only other concern I have is the cost of iterating the map, which might be expensive if large. Perhaps there is a better place to trigger this from less frequently than loadDone().

This patch doesn't include the unrelated change of CachedResource::m_owningCachedResourceLoader to be a set, I'll patch that separately if it makes sense.
Comment 39 Antti Koivisto 2011-07-16 07:02:11 PDT
You should run this with zero-duration (or a bit longer) timer to avoid O(n^2). Other than that this is much better.
Comment 40 Antti Koivisto 2011-07-16 08:52:13 PDT
Comment on attachment 101045 [details]
run a garbage collection on m_documentResources when a load finishes

View in context: https://bugs.webkit.org/attachment.cgi?id=101045&action=review

> Source/WebCore/loader/cache/CachedResource.h:185
>      bool canDelete() const { return !hasClients() && !m_request && !m_preloadCount && !m_handleCount && !m_resourceToRevalidate && !m_proxyResource; }
> +    unsigned getHandleCount() const { return m_handleCount; }

get* naming is not according to coding style. Please replace this with something like hasOneHandle(), no need to expose the count.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:565
> +    garbageCollectDocumentResources();

This should start a timer (zero duration or more) so we don't get to O(n^2).

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:577
> +    for (DocumentResourceMap::iterator it = m_documentResources.begin(); it != m_documentResources.end(); ++it) {
> +        if (it->second->getHandleCount() == 1) {
> +            m_documentResources.remove(it);
> +            // Iterators invalidated. We don't want to copy keys, because they
> +            // maybe be very large 'data:...'. Rather than restart traversal,
> +            // simply only remove the first free-able resource, and get
> +            // another next time.
> +            return;

This is unnecessary. Map keys here are Strings which are really just pointers to StringImpls. Copying a String is just a pointer copy. You should garbage collect the entire map.
Comment 41 Antti Koivisto 2011-07-16 09:03:05 PDT
Also add a comment explaining why this is done and how it could be improved (weak handles).
Comment 42 Scott Graham 2011-07-18 08:22:45 PDT
Created attachment 101160 [details]
corrections per comments
Comment 43 Scott Graham 2011-07-18 08:25:05 PDT
Thanks for your comments, fixed all of the things you pointed out.
Comment 44 Antti Koivisto 2011-07-19 06:05:39 PDT
r=me
Comment 45 WebKit Review Bot 2011-07-20 11:46:00 PDT
Comment on attachment 101160 [details]
corrections per comments

Clearing flags on attachment: 101160

Committed r91384: <http://trac.webkit.org/changeset/91384>
Comment 46 Mark Rowe (bdash) 2011-07-25 11:48:48 PDT
After this change I’m seeing assertion failures in Mail on Lion when navigating away from some messages.  They’re below CachedResourceLoader::garbageCollectDocumentResourcesTimerFired, and it’s the assertion in CachedResource::unregisterHandle that m_handleCount > 0 that’s being hit.  I’ll write up a new bug as soon as I can find a reproducible case that I can share.
Comment 47 Scott Graham 2011-07-25 12:04:22 PDT
I'm tracking a crash that sounds similar but was only reported when cache was disabled in inspector. I'm going to roll back this behaviour for now, if it's happening more broadly.
Comment 48 Scott Graham 2011-07-25 12:22:09 PDT
Created attachment 101892 [details]
rollback, pending crash investigation
Comment 49 Scott Graham 2011-07-25 13:56:01 PDT
Created attachment 101905 [details]
rollback, pending crash investigation
Comment 50 WebKit Review Bot 2011-07-25 16:57:58 PDT
Comment on attachment 101905 [details]
rollback, pending crash investigation

Clearing flags on attachment: 101905

Committed r91725: <http://trac.webkit.org/changeset/91725>
Comment 51 Scott Graham 2011-07-26 09:56:19 PDT
Created attachment 102018 [details]
GC patch updated with test and fix for crash
Comment 52 Scott Graham 2011-07-27 07:16:52 PDT
This patch is identical to https://bugs.webkit.org/attachment.cgi?id=101160 except for the addition of nulling m_owningCachedResourceLoader when the handles are GCd. This avoids ~CachedResource causing a double deletion via removeCachedResource. There's also a test added for this crash.

Antti, do you have time to review again?
Comment 53 Vsevolod Vlasov 2011-07-27 11:35:24 PDT
Comment on attachment 102018 [details]
GC patch updated with test and fix for crash

View in context: https://bugs.webkit.org/attachment.cgi?id=102018&action=review

> LayoutTests/http/tests/inspector/network/disabled-cache-crash.html:31
> +    NetworkAgent.setCacheDisabled(true, step1);

We need to enable memory cache back again before calling completeTest().

Please note, that we've decided that inspector's disable cache option should only clear memory cache, not disable it (https://bugs.webkit.org/show_bug.cgi?id=65184).

Since disabling memory cache proved to be the easiest way to reproduce the crash, I suggest that you use window.internals approach to allow disabling memory cache from layout tests (see https://bugs.webkit.org/show_bug.cgi?id=30080 for an example).
Comment 54 Scott Graham 2011-07-27 13:22:14 PDT
Created attachment 102178 [details]
GC patch updated with fix and test using window.internals
Comment 55 Scott Graham 2011-07-27 13:23:51 PDT
Fixed comment from vsevik re: re-enabling cache at end of test, and modified the test to use window.internals to disable the memory cache now that the functionality is different in inspector.
Comment 56 Gustavo Noronha (kov) 2011-07-27 14:37:27 PDT
Comment on attachment 102178 [details]
GC patch updated with fix and test using window.internals

Attachment 102178 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9260084
Comment 57 Vsevolod Vlasov 2011-07-27 15:25:28 PDT
Created attachment 102195 [details]
Patch, the same as the previous one for mac bot.
Comment 58 Gustavo Noronha (kov) 2011-07-27 18:06:33 PDT
Comment on attachment 102195 [details]
Patch, the same as the previous one for mac bot.

Attachment 102195 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9247981
Comment 59 Scott Graham 2011-07-27 20:56:46 PDT
Created attachment 102222 [details]
further update to export symbols for test harness
Comment 60 Gustavo Noronha (kov) 2011-07-27 23:39:43 PDT
Comment on attachment 102222 [details]
further update to export symbols for test harness

Attachment 102222 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9265109
Comment 61 Scott Graham 2011-07-28 10:04:45 PDT
Created attachment 102266 [details]
fix export symbols for test harness
Comment 62 Collabora GTK+ EWS bot 2011-07-28 12:23:27 PDT
Comment on attachment 102266 [details]
fix export symbols for test harness

Attachment 102266 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9269169
Comment 63 Scott Graham 2011-07-28 13:41:55 PDT
Created attachment 102294 [details]
fix sources.filter for gtk port
Comment 64 Scott Graham 2011-07-28 15:12:53 PDT
I believe this is really, finally (!) a patch worth looking at. Despite all the patches, the only code change since the last r+ is in nulling m_owningCachedResourceLoader on deletion. The rest of the file changes are for adding a test and the various build file and test harness changes required.
Comment 65 Antti Koivisto 2011-07-31 14:19:38 PDT
Comment on attachment 102294 [details]
fix sources.filter for gtk port

r=me
Comment 66 WebKit Review Bot 2011-08-01 09:18:32 PDT
Comment on attachment 102294 [details]
fix sources.filter for gtk port

Rejecting attachment 102294 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 2

Last 500 characters of output:
1 succeeded at 154 with fuzz 1 (offset 4 lines).
patching file Source/WebKit2/win/WebKit2CFLite.def
Hunk #1 succeeded at 148 with fuzz 2 (offset 6 lines).
Hunk #2 FAILED at 153.
1 out of 2 hunks FAILED -- saving rejects to file Source/WebKit2/win/WebKit2CFLite.def.rej
patching file Source/autotools/symbols.filter
Hunk #1 succeeded at 35 with fuzz 2 (offset 4 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Antti Koivisto', u'--f..." exit_code: 1

Full output: http://queues.webkit.org/results/9284452
Comment 67 David Kilzer (:ddkilzer) 2011-08-01 09:44:11 PDT
Comment on attachment 102294 [details]
fix sources.filter for gtk port

View in context: https://bugs.webkit.org/attachment.cgi?id=102294&action=review

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:591
> +    Vector<String, 10> toDelete;

Nit: It would be nice to make Vector<String, 10> a typedef so you don't have to duplicate the type below.
Nit: toDelete is kind of generic.  What are we about to delete?  Maybe urlsToDelete?  resourcesToDelete?  resourceURLsToDelete?  resourcesToDelete?

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:601
> +    for (Vector<String, 10>::const_iterator idel = toDelete.begin(); idel != toDelete.end(); ++idel)
> +        m_documentResources.remove(*idel);

Nit: idel is a strange iterator name.  Why not just use it?  It's scoped to the for() loop above, so you can "reuse" it here.
Comment 68 Scott Graham 2011-08-01 10:05:14 PDT
Created attachment 102529 [details]
update based on comments
Comment 69 Scott Graham 2011-08-01 10:07:33 PDT
(In reply to comment #67)
> (From update of attachment 102294 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102294&action=review
> 
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:591
> > +    Vector<String, 10> toDelete;
> 
> Nit: It would be nice to make Vector<String, 10> a typedef so you don't have to duplicate the type below.

Thanks, fixed.

> Nit: toDelete is kind of generic.  What are we about to delete?  Maybe urlsToDelete?  resourcesToDelete?  resourceURLsToDelete?  resourcesToDelete?

Thanks, fixed as resourcesToDelete.

> 
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:601
> > +    for (Vector<String, 10>::const_iterator idel = toDelete.begin(); idel != toDelete.end(); ++idel)
> > +        m_documentResources.remove(*idel);
> 
> Nit: idel is a strange iterator name.  Why not just use it?  It's scoped to the for() loop above, so you can "reuse" it here.

Burned too many times by ancient compilers that used the old scoping rules. Thanks, fixed.
Comment 70 WebKit Review Bot 2011-08-01 14:22:21 PDT
Comment on attachment 102529 [details]
update based on comments

Clearing flags on attachment: 102529

Committed r92143: <http://trac.webkit.org/changeset/92143>
Comment 71 WebKit Review Bot 2011-08-01 14:22:53 PDT
All reviewed patches have been landed.  Closing bug.