Bug 56415 - ApplicationCacheGroup is not obsolete after being deleted via ApplicationCacheStorage::deleteEntriesForOrigin
: ApplicationCacheGroup is not obsolete after being deleted via ApplicationCach...
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-03-15 14:31 PST by
Modified: 2011-03-19 15:17 PST (History)


Attachments
Patch (17.03 KB, patch)
2011-03-15 17:11 PST, Anton D'Auria
no flags Review Patch | Details | Formatted Diff | Diff
Patch (16.80 KB, patch)
2011-03-15 17:17 PST, Anton D'Auria
no flags Review Patch | Details | Formatted Diff | Diff
Patch (28.12 KB, patch)
2011-03-17 13:06 PST, Anton D'Auria
no flags Review Patch | Details | Formatted Diff | Diff
Patch (30.99 KB, patch)
2011-03-17 16:47 PST, Anton D'Auria
no flags Review Patch | Details | Formatted Diff | Diff
Patch (32.39 KB, patch)
2011-03-18 12:52 PST, Anton D'Auria
no flags Review Patch | Details | Formatted Diff | Diff
Patch (31.46 KB, patch)
2011-03-18 13:11 PST, Anton D'Auria
no flags Review Patch | Details | Formatted Diff | Diff
Patch (31.42 KB, patch)
2011-03-18 13:34 PST, Anton D'Auria
no flags Review Patch | Details | Formatted Diff | Diff
Patch (31.79 KB, patch)
2011-03-18 16:54 PST, Anton D'Auria
no flags Review Patch | Details | Formatted Diff | Diff
Patch (31.08 KB, patch)
2011-03-18 17:01 PST, Anton D'Auria
no flags Review Patch | Details | Formatted Diff | Diff
Patch (30.23 KB, patch)
2011-03-18 17:12 PST, Anton D'Auria
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-03-15 14:31:04 PST
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).
------- Comment #1 From 2011-03-15 16:31:55 PST -------
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
------- Comment #2 From 2011-03-15 17:11:25 PST -------
Created an attachment (id=85881) [details]
Patch
------- Comment #3 From 2011-03-15 17:17:17 PST -------
(From update of attachment 85881 [details])
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.
------- Comment #4 From 2011-03-15 17:17:22 PST -------
Created an attachment (id=85883) [details]
Patch

Removing stray svn conflict line
------- Comment #5 From 2011-03-15 17:49:25 PST -------
(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.

> 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.
------- Comment #6 From 2011-03-15 18:08:06 PST -------
(In reply to comment #5)
> (From update of attachment 85883 [details] [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.
------- Comment #7 From 2011-03-15 18:16:52 PST -------
(In reply to comment #5)
> (From update of attachment 85883 [details] [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."
------- Comment #8 From 2011-03-15 19:29:32 PST -------
(From update of attachment 85883 [details])
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.
------- Comment #9 From 2011-03-15 21:07:27 PST -------
> 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.
------- Comment #10 From 2011-03-15 21:27:05 PST -------
> 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.
------- Comment #11 From 2011-03-16 11:17:11 PST -------
(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.
------- Comment #12 From 2011-03-17 13:06:53 PST -------
Created an attachment (id=86086) [details]
Patch
------- Comment #13 From 2011-03-17 13:31:23 PST -------
(From update of attachment 86086 [details])
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.
------- Comment #14 From 2011-03-17 13:38:05 PST -------
Attachment 86086 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8193749
------- Comment #15 From 2011-03-17 15:15:00 PST -------
(From update of attachment 86086 [details])
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.
------- Comment #16 From 2011-03-17 16:47:26 PST -------
Created an attachment (id=86114) [details]
Patch
------- Comment #17 From 2011-03-17 16:50:08 PST -------
Windows port skips all appcache tests.
------- Comment #18 From 2011-03-18 11:43:59 PST -------
(From update of attachment 86114 [details])
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.
------- Comment #19 From 2011-03-18 11:51:10 PST -------
> > +    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);
------- Comment #20 From 2011-03-18 12:52:02 PST -------
Created an attachment (id=86199) [details]
Patch
------- Comment #21 From 2011-03-18 13:11:41 PST -------
Created an attachment (id=86201) [details]
Patch
------- Comment #22 From 2011-03-18 13:34:10 PST -------
Created an attachment (id=86203) [details]
Patch

fixed qt skipped-list conflict
------- Comment #23 From 2011-03-18 13:36:34 PST -------
(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.
------- Comment #24 From 2011-03-18 13:45:02 PST -------
(In reply to comment #23)
> (From update of attachment 86203 [details] [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.
------- Comment #25 From 2011-03-18 16:54:26 PST -------
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.
------- Comment #26 From 2011-03-18 16:56:30 PST -------
(In reply to comment #25)
> Created an attachment (id=86242) [details] [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.
------- Comment #27 From 2011-03-18 16:58:45 PST -------
(From update of attachment 86242 [details])
Obsoleting this patch because it contains unrelated changes. Resubmitting...
------- Comment #28 From 2011-03-18 17:01:31 PST -------
Created an attachment (id=86244) [details]
Patch
------- Comment #29 From 2011-03-18 17:05:44 PST -------
(From update of attachment 86244 [details])
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.
------- Comment #30 From 2011-03-18 17:07:29 PST -------
(From update of attachment 86244 [details])
Thanks for cleaning that up.
------- Comment #31 From 2011-03-18 17:12:33 PST -------
Created an attachment (id=86245) [details]
Patch

DOMApplicationCache.h doesn't need to be private.
------- Comment #32 From 2011-03-18 20:19:07 PST -------
(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

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
------- Comment #33 From 2011-03-19 09:45:59 PST -------
(In reply to comment #32)
> (From update of attachment 86245 [details] [details])
> Rejecting attachment 86245 [details] [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?
------- Comment #34 From 2011-03-19 14:29:16 PST -------
(From update of attachment 86245 [details])
Clearing flags on attachment: 86245

Committed r81557: <http://trac.webkit.org/changeset/81557>
------- Comment #35 From 2011-03-19 14:29:21 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #36 From 2011-03-19 15:17:43 PST -------
http://trac.webkit.org/changeset/81557 might have broken GTK Linux 64-bit Debug