Bug 56415 – ApplicationCacheGroup is not obsolete after being deleted via ApplicationCacheStorage::deleteEntriesForOrigin

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Anton D'Auria]

After ApplicationCacheStorage::deleteEntriesForOrigin() is called, this assertion fails in ApplicationCacheGroup::cacheGroupDestroyed(ApplicationCacheGroup* group):
    ASSERT(m_cachesInMemory.get(group->manifestURL()) == group);

bool ApplicationCacheStorage::deleteCacheGroup(const String& manifestURL) should call group->makeObsolete() instead of cacheGroupMadeObsolete(group). ApplicationCacheGroup::makeObsolete() actually calls cacheGroupMadeObsolete() and sets the group's internal state that tracks its obsolescence to true (m_isObsolete).

[Anton D'Auria]

this is the backtrace:


Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x00000000bbadbeef
Crashed Thread:  0  Dispatch queue: com.apple.main-thread

Thread 0 Crashed:  Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x000000010101b6f7 WebCore::ApplicationCacheStorage::cacheGroupDestroyed(WebCore::ApplicationCacheGroup*) + 303 (ApplicationCacheStorage.cpp:336)
1   com.apple.WebCore             	0x0000000101009e0d WebCore::ApplicationCacheGroup::~ApplicationCacheGroup() + 791 (ApplicationCacheGroup.cpp:95)
2   com.apple.WebCore             	0x0000000101005361 WebCore::ApplicationCacheGroup::cacheDestroyed(WebCore::ApplicationCache*) + 263 (ApplicationCacheGroup.cpp:385)
3   com.apple.WebCore             	0x00000001010010e9 WebCore::ApplicationCache::~ApplicationCache() + 83 (ApplicationCache.cpp:52)
4   com.apple.WebCore             	0x000000010101005d WTF::RefCounted<WebCore::ApplicationCache>::deref() + 43 (RefCounted.h:141)
5   com.apple.WebCore             	0x000000010101008d void WTF::derefIfNotNull<WebCore::ApplicationCache>(WebCore::ApplicationCache*) + 37 (PassRefPtr.h:59)
6   com.apple.WebCore             	0x00000001010100e6 WTF::PassRefPtr<WebCore::ApplicationCache>::~PassRefPtr() + 24 (PassRefPtr.h:74)
7   com.apple.WebCore             	0x0000000101005247 WebCore::ApplicationCacheGroup::disassociateDocumentLoader(WebCore::DocumentLoader*) + 595 (ApplicationCacheGroup.cpp:370)
8   com.apple.WebCore             	0x00000001010138ee WebCore::ApplicationCacheHost::~ApplicationCacheHost() + 222 (ApplicationCacheHost.cpp:61)
9   com.apple.WebCore             	0x00000001012537b9 void WTF::deleteOwnedPtr<WebCore::ApplicationCacheHost>(WebCore::ApplicationCacheHost*) + 36 (OwnPtrCommon.h:59)
10  com.apple.WebCore             	0x00000001012537dc WTF::OwnPtr<WebCore::ApplicationCacheHost>::~OwnPtr() + 24 (OwnPtr.h:57)
11  com.apple.WebCore             	0x000000010124e4d6 WebCore::DocumentLoader::~DocumentLoader() + 174 (DocumentLoader.cpp:115)
12  com.apple.WebKit              	0x0000000100a175c4 WebDocumentLoaderMac::~WebDocumentLoaderMac() + 184 (WebDocumentLoaderMac.h:40)
13  com.apple.WebCore             	0x000000010100c527 WTF::RefCounted<WebCore::DocumentLoader>::deref() + 63 (RefCounted.h:141)
14  com.apple.WebCore             	0x000000010100f82d void WTF::derefIfNotNull<WebCore::DocumentLoader>(WebCore::DocumentLoader*) + 41 (PassRefPtr.h:59)
15  com.apple.WebCore             	0x00000001013d8196 WTF::RefPtr<WebCore::DocumentLoader>::operator=(WebCore::DocumentLoader*) + 56 (RefPtr.h:136)
16  com.apple.WebCore             	0x00000001013d0760 WebCore::FrameLoader::setDocumentLoader(WebCore::DocumentLoader*) + 360 (FrameLoader.cpp:1787)
17  com.apple.WebCore             	0x00000001013d0798 WebCore::FrameLoader::detachViewsAndDocumentLoader() + 54 (FrameLoader.cpp:2620)
18  com.apple.WebCore             	0x00000001013d04ca WebCore::FrameLoader::detachFromParent() + 122 (FrameLoader.cpp:2606)
19  com.apple.WebCore             	0x00000001013d05e6 WebCore::FrameLoader::detachChildren() + 76 (FrameLoader.cpp:2507)
20  com.apple.WebCore             	0x00000001013d04a7 WebCore::FrameLoader::detachFromParent() + 87 (FrameLoader.cpp:2600)
21  com.apple.WebKit              	0x0000000100aea641 -[WebView(WebPrivate) _close] + 298 (WebView.mm:1144)
22  com.apple.WebKit              	0x0000000100ad8f4f -[WebView close] + 32 (WebView.mm:3209)
23  DumpRenderTree                	0x0000000100012519 dumpRenderTree(int, char const**) + 593 (DumpRenderTree.mm:689)
24  DumpRenderTree                	0x000000010001261f main + 97 (DumpRenderTree.mm:719)
25  DumpRenderTree                	0x0000000100002250 start + 52

[Anton D'Auria]

Created attachment 85881 [details]
Patch

[Adam Barth]

Comment on attachment 85881 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        When deleting all cache entries for an origin, call ApplicationCacheGroup::makeObsolete() on each cache group for the origin, instead of ApplicationCacheStorage::cacheGroupMadeObsolete(), which did not update the m_isObsolete state of the group. This caused an assertion failure in ApplicationCacheStorage::cacheGroupDestroyed() when the DocumentLoader was getting destroyed.

Holy long lines, Batman.

> Source/WebCore/ChangeLog:236
> +>>>>>>> .r81198

This conflict marker should probably be removed.

[Anton D'Auria]

Created attachment 85883 [details]
Patch

Removing stray svn conflict line

[Alexey Proskuryakov]

Comment on attachment 85883 [details]
Patch

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

I don't think that the approach here is safe. To forget a used application cache, you should call ApplicationCacheGroup::makeObsolete() - ApplicationCacheStorage::deleteCacheGroup() is just untested and unused code that has been landed by mistake, as far as I can tell.

It's not good to manipulate ApplicationCacheStorage directly behind the back of an existing ApplicationCacheGroup.

> Source/WebCore/ChangeLog:7
> +        ApplicationCacheGroup is not obsolete after being deleted via ApplicationCacheStorage::deleteEntriesForOrigin
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=56415

We don't usually put a blank line between bug title and number.

> Source/WebKit/mac/ChangeLog:7
> +        ApplicationCacheGroup is not obsolete after being deleted via ApplicationCacheStorage::deleteEntriesForOrigin
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=56415

Extra blank line, again.

> Tools/DumpRenderTree/LayoutTestController.cpp:393
> +    JSRetainPtr<JSStringRef> originUrl(Adopt, JSValueToStringCopy(context, arguments[0], exception));

"URL" should be capitalized.

[Anton D'Auria]

(In reply to comment #5)
> (From update of attachment 85883 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85883&action=review
> 
> I don't think that the approach here is safe. To forget a used application cache, you should call ApplicationCacheGroup::makeObsolete() - ApplicationCacheStorage::deleteCacheGroup() is just untested and unused code that has been landed by mistake, as far as I can tell.
> 
> It's not good to manipulate ApplicationCacheStorage directly behind the back of an existing ApplicationCacheGroup.

What you are proposing will only handle the in-memory appcache. This change does call ApplicationCacheGroup::makeObsolete() if the ApplicationCacheGroup is in memory, via ApplicationCacheGroup::deleteCacheGroup(), which also handles the case when the appcache is just on disk.

[Anton D'Auria]

(In reply to comment #5)
> (From update of attachment 85883 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85883&action=review
> It's not good to manipulate ApplicationCacheStorage directly behind the back of an existing ApplicationCacheGroup.

We're only modifying ApplicationCacheStorage if an ApplicationCacheGroup isn't in m_cachesInMemory, so we won't be doing it "behind the back of an existing ApplicationCacheGroup."

[Joseph Pecoraro]

Comment on attachment 85883 [details]
Patch

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

I think Anton's fix here is correct. But I won't change review status, because
I know Alexey is more familiar with the code than I am. To me it looks like
this code gracefully handles caches:

  1. If the CacheGroup is in memory, it gracefully marks it as obsolete.
  2. If the CacheGroup is not in memory, it deletes it from the SQL database.

I do have plenty of comments on the patch though!

> Source/WebKit/mac/ChangeLog:13
> +        (+[WebApplicationCache deleteOrigin:]):

Based on your comment above, would a better name be: deleteCacheGroupsForOrigin:?

> Source/WebKit/mac/WebCoreSupport/WebApplicationCache.mm:32
>  #import "WebApplicationCache.h"
> +#import "WebSecurityOriginInternal.h"
> +
>  #import <WebCore/ApplicationCacheStorage.h>
> +#import <WebCore/SecurityOrigin.h>

Nit: I think this should actually be:

  #import "WebApplicationCache.h"

  #import "WebSecurityOriginInternal.h"
  #import <WebCore/ApplicationCacheStorage.h>
  #import <WebCore/SecurityOrigin.h>

The header file for this implementation file is up front, and along.
Then the rest follow, alphabetically sorted.

> Tools/ChangeLog:5
> +        https://bugs.webkit.org/show_bug.cgi?id=56415

Nit: Missing Bug Title.

> Tools/DumpRenderTree/LayoutTestController.cpp:2119
>          { "clearAllApplicationCaches", clearAllApplicationCachesCallback, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete },
> +        { "clearApplicationCacheForOrigin", clearApplicationCacheForOriginCallback, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete },
>          { "clearAllDatabases", clearAllDatabasesCallback, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete },

Nit: I think this was alphabetical order.

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:750
> +void LayoutTestController::clearApplicationCacheForOrigin(const CppArgumentList&, CppVariant* result)
> +{
> +    // FIXME: implement to support Application Cache Quotas.
> +    result->setNull();

This comment, and the "FIXME: implement" comments for the other stubbed ports
are all slightly inaccurate or not very helpful. The function does not deal with application
cache quotas (probably just a copy+paste error), it is meant to delete caches for
a particular origin. With that in mind, a better comment might be:

  FIXME: implement to test deleting Application Caches for an Origin.

I am still not a fan of "clear" meaning "delete" but you're being consistent.

> LayoutTests/http/tests/appcache/origin-delete-expected.txt:5
> +This test checks that application cache groups correctly become obsolete during deletion for a specific origin by exercising the WebCore::ApplicationCacheStorage::deleteEntriesForOrigin(), WebCore::ApplicationCacheStorage::cacheGroupDestroyed() path.
> +
> +PASS - cached iframe-1
> +PASS - cached iframe-2
> +

This test says it is checking that the application cache groups are becoming obsolete,
but I don't see that actually be checked, or output that would say so.

> LayoutTests/http/tests/appcache/origin-delete.html:50
> +        layoutTestController.clearApplicationCacheForOrigin("http://127.0.0.1:8000");
> +        finishTest();

After this call you should be checking if the application cache in the two iframes
is obsolete or not. The fact that this test didn't ASSERT so far is good, but to
also test that the behavior is correct (that the application caches are obsolete)
would be even better.

You can check the status of an applicationCache through it's status propery.
http://dev.w3.org/html5/spec/offline.html#dom-appcache-status

At this point in your test, before calling finishTest(), I would expect
a function verifyObsolete() which:

  1) sends a "are you obsolete?" message to each of the iframes
  2) waits for a response from each.
  3) outputs the responses, so there is something visible in the expected results.

The iframes html files would then need to:

  1) handle the message
  2) Return a response checking its appcache status:

      (applicationCache.status === applicationCache.OBSOLETE)

Does that make sense? Hopefully such a test is already failing and
is fixed after your change.

> LayoutTests/http/tests/appcache/origin-delete.html:58
> +<p>This test checks that application cache groups correctly become obsolete during deletion for a specific origin by
> + exercising the WebCore::ApplicationCacheStorage::deleteEntriesForOrigin(), WebCore::ApplicationCacheStorage::cacheGroupDestroyed() path.

Nit: I think putting function names in the description is not very future proof.
I make the same mistake all the time. The functions could change names,
move around, or be deleted all together. It would be better to describe what
the test is actually testing, using Application Cache terminology, which will
always stay the same.

[Alexey Proskuryakov]

> We're only modifying ApplicationCacheStorage if an ApplicationCacheGroup isn't in m_cachesInMemory, so we won't be doing it "behind the back of an existing ApplicationCacheGroup."

This is an inversion of responsibilities. It's ApplicationCache and ApplicationCacheGroup that are an interface to this system - a storage class is responsible for disk storage, not for having general logic like this.

It is important to maintain this design, because sooner or later we'll need to rewrite the storage part, as it's quite inefficient.

[Joseph Pecoraro]

> I don't think that the approach here is safe. To forget a used application
> cache, you should call ApplicationCacheGroup::makeObsolete() -
> ApplicationCacheStorage::deleteCacheGroup() is just untested and
> unused code that has been landed by mistake, as far as I can tell.

> This is an inversion of responsibilities. It's ApplicationCache and
> ApplicationCacheGroup that are an interface to this system - a storage
> class is responsible for disk storage, not for having general logic like this.

Oh I see what you mean. That sounds smart. Good point.

[Anton D'Auria]

(In reply to comment #10)
> > I don't think that the approach here is safe. To forget a used application
> > cache, you should call ApplicationCacheGroup::makeObsolete() -
> > ApplicationCacheStorage::deleteCacheGroup() is just untested and
> > unused code that has been landed by mistake, as far as I can tell.
> 
> > This is an inversion of responsibilities. It's ApplicationCache and
> > ApplicationCacheGroup that are an interface to this system - a storage
> > class is responsible for disk storage, not for having general logic like this.
> 
> Oh I see what you mean. That sounds smart. Good point.

I also agree. ApplicationCacheStorage::deleteEntriesForOrigin() should not be obsoleting the in-memory cache, but should just remove the entries from disk.

[Anton D'Auria]

Created attachment 86086 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 86086 [details]
Patch

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

I didn't fully review everything in this patch, but it looks good.

> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:1131
> +void ApplicationCacheGroup::deleteCacheGroupsForOrigin(SecurityOrigin* origin)

I can't decide whether this looks better on ApplicationCacheGroup or on ApplicationCache...

> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:1140
> +    // Multiple manifest URLs might share the same SecurityOrigin, so we might be creating extra, wasted origins here.
> +    // The current schema doesn't allow for a more efficient way of deleting by origin.

Is there a reason to create a SecurityOrigin, not just use protocolHostAndPortAreEqual()?

> Source/WebCore/loader/appcache/ApplicationCacheStorage.h:77
>      ApplicationCacheGroup* findOrCreateCacheGroup(const KURL& manifestURL);
> +    ApplicationCacheGroup* getInMemoryCacheGroup(const String& manifestURL);

Naming nit: we usually use "get" prefix only with functions that return the result in an out parameter. I would suggest making this look more like findOrCreateCacheGroup() above.

> Source/WebCore/loader/appcache/ApplicationCacheStorage.h:85
> +    

Unneeded extra blank line.

> Source/WebKit2/WebProcess/ApplicationCache/WebApplicationCacheManager.cpp:33
> +#include <WEbCore/ApplicationCacheGroup.h>

Typo: WEbCore.

[Early Warning System Bot]

Attachment 86086 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8193749

[Joseph Pecoraro]

Comment on attachment 86086 [details]
Patch

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

Much nicer! I have some nits below, but there are some things that I think would prevent
landing. r- for these:

  • This test will fail on ports that have not implemented layoutTestController.clearApplicationCacheForOrigin.
    It would be good to add the test to those ports Skipped list, with bugzilla bug + comment.

  • This appears to update LayoutTests/http/tests/appcache/resources/origin-usage-iframe-1.html but
    the file does not show up in the patch. It also might be worth creating a new resource, so that
    if a test changes it doesn't accidentally break the other test.

> Source/WebKit/mac/WebCoreSupport/WebApplicationCache.mm:32
>  #import <WebCore/ApplicationCacheStorage.h>
> +#import <WebCore/ApplicationCacheGroup.h>

Nit: alphabetically swap these two. (Might have been my fault).

> Tools/DumpRenderTree/LayoutTestController.h:53
>      void clearAllApplicationCaches();
> +    void clearApplicationCacheForOrigin(JSStringRef name);
>      void clearAllDatabases();

Nit: probably best to be alphabetical here too, but no big deal.

> LayoutTests/ChangeLog:9
> +        These tests create appcache for the DRT origin, delete appcache for the origin,
> +        and verify that the appcache has obsolete status.

Nit: "These test create" => "This test creates". I also tend to prefix Application Cache with "the" or "an", but that may be personal preference.

> LayoutTests/http/tests/appcache/origin-delete.html:37
> +        if (event.data == "PASS - cached iframe-1") {

Nit: With JavaScript, almost always, ==/!= can be ===/!==. It makes it clearer as well. There are a couple cases here.

> LayoutTests/http/tests/appcache/origin-delete.html:42
> +            var getStatus = "appcache_status";
> +            frame.contentWindow.postMessage(getStatus, "*");

Nit: No need for the temp variable. It doesn't add anything extra here.

[Anton D'Auria]

Created attachment 86114 [details]
Patch

[Anton D'Auria]

Windows port skips all appcache tests.

[Joseph Pecoraro]

Comment on attachment 86114 [details]
Patch

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

Thanks this looks good! r=me

> Source/WebCore/loader/appcache/ApplicationCacheStorage.h:77
> +    ApplicationCacheGroup* inMemoryCacheGroupIfExists(const String& manifestURL);

Nit: method could be marked const.

[Alexey Proskuryakov]

> > +    ApplicationCacheGroup* inMemoryCacheGroupIfExists(const String& manifestURL);
> 
> Nit: method could be marked const.

I suggested making it more like findOrCreateCacheGroup. That would involve a different name and argument type:

     ApplicationCacheGroup* findInMemoryCacheGroup(const KURL& manifestURL);

[Anton D'Auria]

Created attachment 86199 [details]
Patch

[Anton D'Auria]

Created attachment 86201 [details]
Patch

[Anton D'Auria]

Created attachment 86203 [details]
Patch

fixed qt skipped-list conflict

[Joseph Pecoraro]

Comment on attachment 86203 [details]
Patch

Nice. I see you didn't switch to KURL, but this still looks good.
Alexey, let me know if you'd prefer that switch. Anton might
have a reason why he didn't.

[Anton D'Auria]

(In reply to comment #23)
> (From update of attachment 86203 [details])
> Nice. I see you didn't switch to KURL, but this still looks good.
> Alexey, let me know if you'd prefer that switch. Anton might
> have a reason why he didn't.

I tried it by making a KURL with the origin's port, host, and protocol, but protocolHostAndPortAreEqual() was failing. I wasn't sure if it was worth 15 minutes of investigation, but it's nagging me so I'll take a look at it. Let me know if I shouldn't bother.

[Anton D'Auria]

Created attachment 86242 [details]
Patch

Comparing KURLs instead of SecurityOrigins when searching for in-memory cache groups, which avoids unnecessary creation of SecurityOrigins.

[Anton D'Auria]

(In reply to comment #25)
> Created an attachment (id=86242) [details]
> Patch
> 
> Comparing KURLs instead of SecurityOrigins when searching for in-memory cache groups, which avoids unnecessary creation of SecurityOrigins.

This patch also has the new signature - ApplicationCacheStorage::findInMemoryCacheGroup(const KURL& manifestURL) const.

[Anton D'Auria]

Comment on attachment 86242 [details]
Patch

Obsoleting this patch because it contains unrelated changes. Resubmitting...

[Anton D'Auria]

Created attachment 86244 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 86244 [details]
Patch

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

Looks good to me.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:471
> -		1A8F6BC30DB55CDC001DB794 /* DOMApplicationCache.h in Headers */ = {isa = PBXBuildFile; fileRef = 1A8F6BB80DB55CDC001DB794 /* DOMApplicationCache.h */; settings = {ATTRIBUTES = (); }; };
> +		1A8F6BC30DB55CDC001DB794 /* DOMApplicationCache.h in Headers */ = {isa = PBXBuildFile; fileRef = 1A8F6BB80DB55CDC001DB794 /* DOMApplicationCache.h */; settings = {ATTRIBUTES = (Private, ); }; };

I don't understand this change.

[Joseph Pecoraro]

Comment on attachment 86244 [details]
Patch

Thanks for cleaning that up.

[Anton D'Auria]

Created attachment 86245 [details]
Patch

DOMApplicationCache.h doesn't need to be private.

[WebKit Commit Bot]

Comment on attachment 86245 [details]
Patch

Rejecting attachment 86245 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'land-a..." exit_code: 2

Last 500 characters of output:
m/chromium-win/tables/mozilla/marvin/x_tr_align_justify-expected.checksum
	M	LayoutTests/platform/chromium-win/tables/mozilla/marvin/x_th_align_justify-expected.png
	M	LayoutTests/ChangeLog
r81544 = 1f55c6380698ee1544ebbb50d9d8b04571ca3e1e (refs/remotes/trunk)
	M	Source/WebKit2/ChangeLog
	M	Source/WebKit2/win/WebKit2.vcproj
r81545 = b66653fc1e431957bd7e70af2635673a92bbcc66 (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.

Full output: http://queues.webkit.org/results/8199590

[Anton D'Auria]

(In reply to comment #32)
> (From update of attachment 86245 [details])
> Rejecting attachment 86245 [details] from commit-queue.
> 
> Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'land-a..." exit_code: 2
> Full output: http://queues.webkit.org/results/8199590

It looks like dcommit failed because the svn server was unreachable:

From the full output: RA layer request failed: OPTIONS of 'http://svn.webkit.org/repository/webkit/trunk': could not connect to server (http://svn.webkit.org) at /usr/local/git/libexec/git-core/git-svn line 574

Could you c+ it again for the commit queue to pick it up and try again?

[WebKit Commit Bot]

Comment on attachment 86245 [details]
Patch

Clearing flags on attachment: 86245

Committed r81557: <http://trac.webkit.org/changeset/81557>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/81557 might have broken GTK Linux 64-bit Debug