Bug 40627 – Limit ApplicationCache Total and Per-Origin Storage Capacity (Quotas)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Joseph Pecoraro]

Provide a way to limit the storage capacity of application caches
instead of allowing it to grow without checks.

[Joseph Pecoraro]

Created attachment 58788 [details]
[PATCH] Part 1 - Add Per-Origin quota logic, and persistence.

[Joseph Pecoraro]

Created attachment 58789 [details]
[PATCH] Part 2 - Add Total and Per-Origin Quota Settings. (needs part 1)

[WebKit Review Bot]

Attachment 58788 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/page/ChromeClient.h:45:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

<rdar://problem/8032000>

[Early Warning System Bot]

Attachment 58788 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3267164

[WebKit Review Bot]

Attachment 58788 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3267165

[Joseph Pecoraro]

Looks like INT64_MAX was missing for gtk and qt. Probably a missing include.

[Joseph Pecoraro]

Created attachment 58797 [details]
[PATCH] Part 1 - Add Per-Origin quota logic, and persistence. (include stdint.h)

Added "#include <stdint.h>" to ApplicationCacheStorage.h to try and fix gtk and qt.

[WebKit Review Bot]

Attachment 58797 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/page/ChromeClient.h:45:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 58797 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3316184

[Early Warning System Bot]

Attachment 58797 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3331193

[WebKit Review Bot]

Attachment 58788 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3312188

[WebKit Review Bot]

Attachment 58797 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3294196

[Joseph Pecoraro]

"#include <limits>" includes "std::numeric_limits<long long>::max()"

However, I can't set that to a global:

  static const int64_t noQuota = std::numeric_limits<long long>::max();
  -> Compile Error "a function call cannot appear in a constant-expression"

I can change this instead to a function:

  static const int64_t noQuota() { return std::numeric_limits<long long>::max(); }

But I don't like that solution. Is there an easy way to just #define INT64_MAX
for gtk and qt, or is another solution preferred?

[Joseph Pecoraro]

Created attachment 58849 [details]
[PATCH] Part 3 - Test Case

This adds a test case but it doesn't call setQuota directly on an ApplicationCacheGroup,
instead it uses the preference, and creates a new cache group by changing the manifest url
(this causes a new group to be created).

After speaking with Alexey, and making these tests, I think "Per-Origin" is the wrong
name for this bug. And ApplicationCacheGroupQuota is misleading, because its
really a quota on the ApplicationCaches themselves, it just happens that the size counter
is in ApplicationCacheGroup because thats where the resource loads happen.

So, feel free to suggest better names or suggest that I move things around.

[WebKit Review Bot]

Attachment 58849 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKitTools/DumpRenderTree/LayoutTestController.cpp:736:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 58849 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3320208

[Alexey Proskuryakov]

I'm discussing this patch with Joseph. Not sure if per-appcache quota is the right way to go, given that we usually have per-origin quotas.

[Joseph Pecoraro]

(In reply to comment #18)
> I'm discussing this patch with Joseph. Not sure if per-appcache quota
> is the right way to go, given that we usually have per-origin quotas.

Background: 
-----------

I took a look at this and thought about what changes might be necessary
for this to happen. Databases use per-origin quotas. They do so with
"DatabaseTracker" which uses its own sqlite database (for persistence)
to store origins and their quotas. See
DatabaseTracker::openTrackerDatabase in:
http://trac.webkit.org/browser/trunk/WebCore/storage/DatabaseTracker.cpp

In Safari you can see this in ~/Library/Safari/Databases/Databases.db:

>   > sqlite3 ~/Library/Safari/Databases/Databases.db
>   > .dump
>   CREATE TABLE Origins ( origin TEXT UNIQUE ON CONFLICT REPLACE
>                        , quota INTEGER NOT NULL ON CONFLICT FAIL);
>   CREATE TABLE Databases ( guid INTEGER PRIMARY KEY AUTOINCREMENT
>                          , origin TEXT, name TEXT, displayName TEXT, estimatedSize INTEGER, path TEXT);


Application cache's currently only use origins in the fetching
process to select particular resources, ignore others, and so
on. Once the resources are downloading it doesn't look like
the origin is needed. So it didn't need to keep it around.
Instead, if it makes it far enough into the process, just keeps
the manifest url in its permanent tables see
ApplicationCacheStorage::openDatabase in:
http://trac.webkit.org/browser/trunk/WebCore/loader/appcache/ApplicationCacheStorage.cpp

In Safari the tables are put somewhere under /private/var/...:

>   > sqlite3 ApplicationCache.db
>   > .tables
>   CacheAllowsAllNetworkRequests  CacheResources               
>   CacheEntries                   CacheWhitelistURLs           
>   CacheGroups                    Caches                       
>   CacheResourceData              FallbackURLs
>   > .schema CacheGroups
>   CREATE TABLE CacheGroups ( id INTEGER PRIMARY KEY AUTOINCREMENT
>                            , manifestHostHash INTEGER NOT NULL ON CONFLICT FAIL
>                            , manifestURL TEXT UNIQUE ON CONFLICT FAIL
>                            , newestCache INTEGER);


My Patch So Far:
----------------

The patches I put up so far tacked a quota onto the `CacheGroups`
table. This means that if the website used a different manifest
URL, its using a different ApplicationCacheGroup with a different
quota. So, a website would be able use multiple manifests to
work around the individual quota limit. And, as Alexey points out
they could use <iframe>s to make use of each cache group's resources
all on a single page (bypassing the supposed quota). Dirty, but
possible.


Per-Origin Quotas:
------------------

To avoid the above problem we can use per-origin quotas. Taking
an approach similar to Databases we can just add a "Origins"
table to match up origins and quotas. The table schema could even
be the same as the Databases schema above:

    CREATE TABLE Origins ( origin TEXT UNIQUE ON CONFLICT REPLACE
                         , quota INTEGER NOT NULL ON CONFLICT FAIL);

CacheGroups already have a manifest URL from which the origin can
be extracted. WebCore::SecurityOrigin handles this nicely.

The complexity is tracking the quota across multiple cache groups.
WebCore::OriginQuotaManager (currently ENABLE(STORAGE) in
WebCore/storage/OriginQuotaManager.{cpp,h}) and OriginQuotaRecord
does this for Databases. The same model looks like it could carry
over to Application Cache Groups and share some code.


My Conclusion:
--------------

Sounds like real per-origin is probably the best solution. Do you
you think I should take that approach and r- what I have here and
"do it right". Or continue with what I have here, and open up a new
bug to transfer it over to real per-origin?

[Joseph Pecoraro]

Created attachment 59425 [details]
[PATCH] Part 1 - Add Total and Per-Origin Quota Preferences.

Taking a different approach. Making the individual parts a little smaller so they
are easier to review. This part adds the Preferences. They are not page/Settings,
they are set on the global ApplicationCacheStorage object returned from
cacheStorage(). Setting the maximumSize empties the database, so I only
set it when the preference changes instead of the usual place. Part 2 will
make use of this preference. I'm just making some tweaks.

[Early Warning System Bot]

Attachment 59425 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3316585

[WebKit Review Bot]

Attachment 59425 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3282575

[WebKit Review Bot]

Attachment 59425 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3279581

[Joseph Pecoraro]

Created attachment 59545 [details]
[PATCH] Part 2 - Update Schema and enforce Per-Origin Quotas (needs part 1)

ChangeLog says it all. I made a decision here that could be debated:

In the Patch:
I ensure that `Origins` records only exist when there is a `Caches`
record that needs it. So, the lifetime of the quota is tied to whether
or not the origin has an application cache. If all caches on that
origin get deleted then the quota will be deleted as well.
  + There are never extra Origins records.
  - Requires an extra SELECT to count the number of Origin Records

Alternate Solution:
`Origins` records, and therefore quotas, should remain permanently,
or until the database is explicitly emptied. This way, once they are set
by the user for an origin, if the cache is deleted and later restored the
originally set quota would still be in effect.
  + Prevent reaching a low quota multiple times if caches come and go
  - May leave around stale Origins records.

Finally, I started taking the Databases Quota management approach, but
felt that was far to heavyweight. The approach I took just grabs the quota
and checks it again before committing to the new cache. The second
check may need to move into a SQL Transaction for storeNewestCache.

[Joseph Pecoraro]

Created attachment 59692 [details]
[PATCH] Part 2 - Update Schema and enforce Per-Origin Quotas (needs part 1)

After some more thought I changed things. This is much simpler as well.
  - persists `Origins` records even if caches are deleted
  - normalized `origin` foreign key to `CacheGroups` instead of `Caches`

[Joseph Pecoraro]

Created attachment 59693 [details]
[PATCH] Part 3 - Refactor storeNewestCache to allow Failure Reason Output (needs part 2)

This is a little bit of refactoring that would be easier to review outside
of part 2. ChangeLog says it all.

[Joseph Pecoraro]

Created attachment 59800 [details]
[PATCH] Part 4 - Client Notification when the Quota is Reached (needs part 3)


Client notifications. I ran into an issue with WebSecurityOrigin. WebSecurityOrigin is
used in a private interface (WebUIDelegatePrivate.h). It also has Database Quota
management baked in. And finally it uses (unsigned long long) types. The problems are:

  - (unsigned long long) makes sense for a quota, but they are stored in a SQLite DB as
    (long long) anyways. So, there is a downcast. I've changed the interface in
    WebSecurityOrigin to prevent any clients from providing too high a value, but
    Database's still have a potential downcast issue.
    
  - To not break the interface, I kept (WebSecurityOrigin*) as the type. And I give
    generic implementations of the quota management functions (usage, quota, setQuota)
    and instead rely on subclasses to handle it correctly. There are now Database
    and ApplicationCache subclasses which handle each of their respective quotas.

I think I'll stop here. Part 5 would be adding a test to make sure it all works, but
I don't want to go that far unless I've got an okay on some of the patches before!

[Joseph Pecoraro]

> Part 5 would be adding a test to make sure it all works [...]

Clarification: I've tested this and made sure it works but a
test will make sure it works with future changes! =)

[David Kilzer (:ddkilzer)]

Comment on attachment 59425 [details]
[PATCH] Part 1 - Add Total and Per-Origin Quota Preferences.

> diff --git a/WebCore/DerivedSources.make b/WebCore/DerivedSources.make
> +ifeq ($(findstring ENABLE_OFFLINE_WEB_APPLICATIONS,$(FEATURE_DEFINES)), ENABLE_OFFLINE_WEB_APPLICATIONS)
> +    WEBCORE_EXPORT_DEPENDENCIES := $(WEBCORE_EXPORT_DEPENDENCIES) WebCore.OfflineWebApplications.exp
> +endif
> diff --git a/WebCore/WebCore.OfflineWebApplications.exp b/WebCore/WebCore.OfflineWebApplications.exp
> +__ZN7WebCore23ApplicationCacheStorage14setMaximumSizeEx
> +__ZN7WebCore23ApplicationCacheStorage16storeCopyOfCacheERKNS_6StringEPNS_20ApplicationCacheHostE
> +__ZN7WebCore23ApplicationCacheStorage17setCacheDirectoryERKNS_6StringE
> +__ZN7WebCore23ApplicationCacheStorage18vacuumDatabaseFileEv
> +__ZN7WebCore23ApplicationCacheStorage21setDefaultOriginQuotaEx
> +__ZN7WebCore23ApplicationCacheStorage5emptyEv
> diff --git a/WebCore/WebCore.base.exp b/WebCore/WebCore.base.exp
> -__ZN7WebCore23ApplicationCacheStorage14setMaximumSizeEx
> -__ZN7WebCore23ApplicationCacheStorage16storeCopyOfCacheERKNS_6StringEPNS_20ApplicationCacheHostE
> -__ZN7WebCore23ApplicationCacheStorage17setCacheDirectoryERKNS_6StringE
> -__ZN7WebCore23ApplicationCacheStorage18vacuumDatabaseFileEv
> -__ZN7WebCore23ApplicationCacheStorage5emptyEv
> diff --git a/WebCore/WebCore.xcodeproj/project.pbxproj b/WebCore/WebCore.xcodeproj/project.pbxproj
> +		A57302A611CBD9F500484CC3 /* WebCore.Geolocation.exp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.exports; path = WebCore.Geolocation.exp; sourceTree = "<group>"; };
> +		A57302A711CBD9F600484CC3 /* WebCore.OfflineWebApplications.exp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.exports; path = WebCore.OfflineWebApplications.exp; sourceTree = "<group>"; };
> @@ -11420,9 +11422,11 @@
> +				A57302A611CBD9F500484CC3 /* WebCore.Geolocation.exp */,
> +				A57302A711CBD9F600484CC3 /* WebCore.OfflineWebApplications.exp */,

This mechanism has changed on trunk since the patch was made (e.g. bit rot set it).  However, this is the correct approach, so it just needs to be updated to work with the new mechanism.

> diff --git a/WebCore/loader/appcache/ApplicationCacheStorage.h b/WebCore/loader/appcache/ApplicationCacheStorage.h
> +    static const int64_t noQuota = INT64_MAX;

It's unfortunate that this constant isn't "-1" since it's a signed value.  Has this ship already sailed for existing users?

> diff --git a/WebKit/mac/WebView/WebPreferences.mm b/WebKit/mac/WebView/WebPreferences.mm
> +- (int64_t)applicationCacheTotalQuota
> +{
> +    return [self _longLongValueForKey:WebKitApplicationCacheTotalQuota];
> +}

I think an ASSERT() here would be good to make sure the dictionary and WebApplicationCache don't get out of sync, since you set the values in to places in -setApplicationCacheTotalQuota: below.

> +- (void)setApplicationCacheTotalQuota:(int64_t)quota
> +{
> +    [self _setLongLongValue:quota forKey:WebKitApplicationCacheTotalQuota];
> +
> +    // Application Cache Preferences are stored on the global cache storage manager, not in Settings.
> +    [WebApplicationCache setMaximumSize:quota];
> +}

r=me after updating the WebCore.*.exp changes to use the new WebCore.exp.in mechanism on trunk.

[Joseph Pecoraro]

> > diff --git a/WebCore/loader/appcache/ApplicationCacheStorage.h b/WebCore/loader/appcache/ApplicationCacheStorage.h
> > +    static const int64_t noQuota = INT64_MAX;
> 
> It's unfortunate that this constant isn't "-1" since it's a signed value.  Has this ship already sailed for existing users?

I few points on this int64 part:

  - session storage uses the max as noQuota, so I went with the flow.
  - this is signed only because SQLite doesn't store unsigned. I thought about this later, and
    I think this value should actually be unsigned, and I check and log cases where the value
    attempts to be set more than INT64_MAX.
  - I actually use -1 later on to mean "unknown" or "not set" rather than using a boolean.

Also the build bots don't like INT64_MAX on other platforms. Do you know of a
platform independent constant I can use? Using numeric_limit<int64_t> gave
me a warning at compile time.

[Adam Barth]

Comment on attachment 59692 [details]
[PATCH] Part 2 - Update Schema and enforce Per-Origin Quotas (needs part 1)

WebCore/ChangeLog:10
 +          Added an `Origins` table to the application cache databases.
Please don't use back-quotes in change logs.  Normal quotes are fine.

[Joseph Pecoraro]

(In reply to comment #31)
> (From update of attachment 59692 [details])
> WebCore/ChangeLog:10
>  +          Added an `Origins` table to the application cache databases.
> Please don't use back-quotes in change logs.  Normal quotes are fine.

Good to know. What is the reason? Typically I use backticks around code
(because of Markdown) and when describing a shell command inline. I
can refrain from putting them in ChangeLogs.

[David Kilzer (:ddkilzer)]

(In reply to comment #30)
> > > diff --git a/WebCore/loader/appcache/ApplicationCacheStorage.h b/WebCore/loader/appcache/ApplicationCacheStorage.h
> > > +    static const int64_t noQuota = INT64_MAX;
> > 
> > It's unfortunate that this constant isn't "-1" since it's a signed value.  Has this ship already sailed for existing users?
> 
> I few points on this int64 part:
> 
>   - session storage uses the max as noQuota, so I went with the flow.
>   - this is signed only because SQLite doesn't store unsigned. I thought about this later, and
>     I think this value should actually be unsigned, and I check and log cases where the value
>     attempts to be set more than INT64_MAX.
>   - I actually use -1 later on to mean "unknown" or "not set" rather than using a boolean.

Okay.

> Also the build bots don't like INT64_MAX on other platforms. Do you know of a
> platform independent constant I can use? Using numeric_limit<int64_t> gave
> me a warning at compile time.

Maybe numeric_limit<long long>?  I think int64_t is just a typedef on most platforms.  At worst, I suppose we could define it for platforms that are missing it.

[David Kilzer (:ddkilzer)]

Comment on attachment 59692 [details]
[PATCH] Part 2 - Update Schema and enforce Per-Origin Quotas (needs part 1)

WebCore/loader/appcache/ApplicationCacheStorage.h:87
 +      static const int64_t unknown = -1;

I think this should be called "unknownQuota" to make it more descriptive.


WebCore/loader/appcache/ApplicationCacheStorage.cpp:426
 +      // Using the count to determine if a record existed or not is a safe way to determine=

Not sure if it's a mail encoding issue, but there is a stray "=" at the end of "determine" here.


WebCore/loader/appcache/ApplicationCacheStorage.cpp:461
 +                  " WHERE Origins.origin=?"

Please put spaces around the "=" operator (unless this is the prevailing style for bound values in this file).

WebCore/loader/appcache/ApplicationCacheStorage.cpp:462
 +                  "   AND Caches.id!=?";

Ditto.

WebCore/loader/appcache/ApplicationCacheStorage.cpp:468
 +                  " WHERE Origins.origin=?";

Ditto.


WebCore/loader/appcache/ApplicationCacheStorage.cpp:501
 +      SQLiteStatement updateStatement(m_database, "UPDATE Origins SET quota=? WHERE origin=?");

Ditto.

r=me

[Joseph Pecoraro]

> WebCore/loader/appcache/ApplicationCacheStorage.h:87
>  +      static const int64_t unknown = -1;
> 
> I think this should be called "unknownQuota" to make it more descriptive.

Sounds good.

> WebCore/loader/appcache/ApplicationCacheStorage.cpp:426
>  +      // Using the count to determine if a record existed or not is a safe way to determine=
> 
> Not sure if it's a mail encoding issue, but there is a stray "=" at the end of "determine" here.

My fault, a typo. I'll fix.

> WebCore/loader/appcache/ApplicationCacheStorage.cpp:461
>  +                  " WHERE Origins.origin=?"
> 
> Please put spaces around the "=" operator (unless this is the prevailing style for bound values in this file).

No space is the prevailing style:
http://trac.webkit.org/browser/trunk/WebCore/loader/appcache/ApplicationCacheStorage.cpp

Thanks!

[David Kilzer (:ddkilzer)]

Comment on attachment 59693 [details]
[PATCH] Part 3 - Refactor storeNewestCache to allow Failure Reason Output (needs part 2)

r=me

[David Kilzer (:ddkilzer)]

Comment on attachment 59800 [details]
[PATCH] Part 4 - Client Notification when the Quota is Reached (needs part 3)

WebCore/WebCore.OfflineWebApplications.exp

REMINDER: These changes will need to be migrated to WebCore/WebCore.exp.in in trunk.

WebCore/loader/appcache/ApplicationCacheGroup.cpp:1024
 +      RefPtr<Frame> frameProtector = m_frame;
 +      OriginQuotaReachedCallbackTimer* timer = new OriginQuotaReachedCallbackTimer(this, frameProtector.get());

Do you really need a frameProtector if the OriginQuotaReachedCallbackTimer keeps its own RefPtr<> to the frame?

WebKit/WebKit.xcodeproj/project.pbxproj: 
 -  		51079D170CED11B00077247D /* WebSecurityOrigin.mm in Sources */ = {isa = PBXBuildFile; fileRef = 51079D140CED11B00077247D /* WebSecurityOrigin.mm */; };

Please don't delete and the re-add files to the Xcode project file.  It causes more work when merging later, and creates unnecessary churn.  Please revert the project file and then move existing files and add the new files to fix this.


WebKit/mac/WebCoreSupport/WebApplicationCacheSecurityOrigin.mm:40
 +      return 0;

Just add the #endif above this line instead of having an #else / return 0; / #endif.

WebKit/mac/WebCoreSupport/WebApplicationCacheSecurityOrigin.mm:52
 +      return 0;

Ditto.


WebKit/mac/WebView/WebUIDelegatePrivate.h:92
 +  @protocol WebSecurityOriginQuotaManagement;

What is this and where is it defined?!

WebKit/mac/WebView/WebUIDelegatePrivate.h:93
 +  @protocol WebSecurityOriginQuotaManagementUnsigned;

Ditto.  I don't see this used anywhere, either.

WebKit/mac/WebView/WebUIDelegatePrivate.h:127
 +  - (void)webView:(WebView *)sender frame:(WebFrame *)frame exceededDatabaseQuotaForSecurityOrigin:(WebSecurityOrigin<WebSecurityOriginQuotaManagement> *)origin database:(NSString *)databaseIdentifier;

I don't see @protocol WebSecurityOriginQuotaManagement defined anywhere.

WebKit/mac/WebView/WebUIDelegatePrivate.h:137
 +  - (void)webView:(WebView *)sender exceededApplicationCacheOriginQuotaForSecurityOrigin:(WebSecurityOrigin<WebSecurityOriginQuotaManagement> *)origin;

Ditto.

r=me assuming you can explain or remove the mysterious @protocols used.  :)

[Joseph Pecoraro]

(In reply to comment #37)
> WebCore/loader/appcache/ApplicationCacheGroup.cpp:1024
>  +      RefPtr<Frame> frameProtector = m_frame;
>  +      OriginQuotaReachedCallbackTimer* timer = new OriginQuotaReachedCallbackTimer(this, frameProtector.get());
> 
> Do you really need a frameProtector if the OriginQuotaReachedCallbackTimer keeps its own RefPtr<> to the frame?

I will look into this. I could use some guidance on lifetime management. While the manifest
resources are downloading there is a Frame. However, OriginQuotaReachedCallbackTimer
is triggered asynchronously  at which point the frame could have been deleted. I try and
keep it around, so that when the callback triggers it can notify the client via:
  
  frame->page()->chrome()->client()->reachedApplicationCacheOriginQuota(m_origin.get());

I was worried that because the callback runs asynchronously I would need to protect the
Frame. However, now I think that even with the protector, the Frame's page() could have been
deleted. I'll look into this a bit more.


> WebKit/WebKit.xcodeproj/project.pbxproj: 
>  -          51079D170CED11B00077247D /* WebSecurityOrigin.mm in Sources */ = {isa = PBXBuildFile; fileRef = 51079D140CED11B00077247D /* WebSecurityOrigin.mm */; };
> 
> Please don't delete and the re-add files to the Xcode project file.  It causes more work when merging
> later, and creates unnecessary churn.  Please revert the project file and then move existing files and add
> the new files to fix this.

I seem to recall moved the file between groups. From WebView to WebCoreSupport,
or something similar. I should have mentioned this in the ChangeLog.


> WebKit/mac/WebCoreSupport/WebApplicationCacheSecurityOrigin.mm:40
>  +      return 0;
> 
> Just add the #endif above this line instead of having an #else / return 0; / #endif.

I thought this would produce an "unreachable statement" if the #if condition was
true. The produced could would look like:

  return ...;
  return 0;



>  +  @protocol WebSecurityOriginQuotaManagement;
>  +  @protocol WebSecurityOriginQuotaManagementUnsigned;
> r=me assuming you can explain or remove the mysterious @protocols used.  :)

Sorry about that, this is stale code. I'll remove them. I did a few iterations
here learning Obj-C and writing a "good" typed interface that respected
the types of the quotas.

This was because the Database's quota is (unsigned long long)
and the ApplicationCacheQuota is (long long). I ended up removing it
all, because it would make a change to the existing public API. I've
considered changing ApplicationCache's quota to be unsigned because
logically it makes sense to have the quota be unsigned. The reason I went
with just (long long) was because the value is stored in SQLite, which really
does store it as signed, so there would be downcast.

I am working on opening a bug for this, I just have to verify I can make
it happen.

[Joseph Pecoraro]

Created attachment 63091 [details]
[PATCH] Updated Part 1 - Checking Compilation on Other Platforms

Changed constant noQuota to a function noQuota(). This no longer allows "const" =/,
but it should compile fine on other platforms.

[Joseph Pecoraro]

Comment on attachment 63091 [details]
[PATCH] Updated Part 1 - Checking Compilation on Other Platforms

Okay, it appears to build on all platforms (windows try bot didn't make it around to it). Part 4 required a minor change, which David Kilzer okayed. I'll be landing these tonight once the bots calm down a bit.

[Joseph Pecoraro]

r64397 = d6f9d5d2898e3055125cb1d66f07cdb60c952cd4
http://trac.webkit.org/changeset/64397

r64398 = 37a5f072cf184af754ecf65f9e99de0f256538a2
http://trac.webkit.org/changeset/64398

r64399 = 21e047d7334ca1e78f7bacfa64b26e430ff18b8d
http://trac.webkit.org/changeset/64399

r64400 = 21fcd10785af135db90d464f52ba4b2a9e844ec2
http://trac.webkit.org/changeset/64400

Watching the bots!

[WebKit Review Bot]

http://trac.webkit.org/changeset/64400 might have broken SnowLeopard Intel Release (Tests)
The following changes are on the blame list:
http://trac.webkit.org/changeset/64400
http://trac.webkit.org/changeset/64397
http://trac.webkit.org/changeset/64398
http://trac.webkit.org/changeset/64399

[WebKit Review Bot]

http://trac.webkit.org/changeset/64397 might have broken SnowLeopard Intel Release (Tests)
The following changes are on the blame list:
http://trac.webkit.org/changeset/64400
http://trac.webkit.org/changeset/64397
http://trac.webkit.org/changeset/64398
http://trac.webkit.org/changeset/64399

[WebKit Review Bot]

http://trac.webkit.org/changeset/64398 might have broken SnowLeopard Intel Release (Tests)
The following changes are on the blame list:
http://trac.webkit.org/changeset/64400
http://trac.webkit.org/changeset/64397
http://trac.webkit.org/changeset/64398
http://trac.webkit.org/changeset/64399

[WebKit Review Bot]

http://trac.webkit.org/changeset/64399 might have broken SnowLeopard Intel Release (Tests)
The following changes are on the blame list:
http://trac.webkit.org/changeset/64400
http://trac.webkit.org/changeset/64397
http://trac.webkit.org/changeset/64398
http://trac.webkit.org/changeset/64399

[Joseph Pecoraro]

Landed a fix for the test here:
r64403 = b0417f9810fceef5fd87853cdc66b97de9dabc86
http://trac.webkit.org/changeset/64403

I was creating a WebDatabaseSecurityOrigin (a subclass of WebSecurityOrigin) and
passing it along as a WebSecurityOrigin, and when functions were called on it
that were implemented in the subclass, it didn't run the subclass' functions... it
ran the empty ones.

I'm going to have to clean this up, but I appreciate any insight into why the
subclasses' functions weren't called. I'll leave this open but clear the r+ flags so
they don't show up in any queues.

[David Kilzer (:ddkilzer)]

(In reply to comment #46)
> Landed a fix for the test here:
> r64403 = b0417f9810fceef5fd87853cdc66b97de9dabc86
> http://trac.webkit.org/changeset/64403
> 
> I was creating a WebDatabaseSecurityOrigin (a subclass of WebSecurityOrigin) and
> passing it along as a WebSecurityOrigin, and when functions were called on it
> that were implemented in the subclass, it didn't run the subclass' functions... it
> ran the empty ones.
> 
> I'm going to have to clean this up, but I appreciate any insight into why the
> subclasses' functions weren't called. I'll leave this open but clear the r+ flags so
> they don't show up in any queues.

Are you sure the object was a WebDatabaseSecurityOrigin?  What does "po <variable>" say in gdb?  Were the signatures for the overridden methods identical?  Unlike C++, Objective-C should still call the subclass' methods even if it was cast to a super class pointer.

[David Kilzer (:ddkilzer)]

(In reply to comment #47)
> (In reply to comment #46)
> > I was creating a WebDatabaseSecurityOrigin (a subclass of WebSecurityOrigin) and
> > passing it along as a WebSecurityOrigin, and when functions were called on it
> > that were implemented in the subclass, it didn't run the subclass' functions... it
> > ran the empty ones.
> > 
> > I'm going to have to clean this up, but I appreciate any insight into why the
> > subclasses' functions weren't called. I'll leave this open but clear the r+ flags so
> > they don't show up in any queues.
> 
> Are you sure the object was a WebDatabaseSecurityOrigin?  What does "po <variable>"
> say in gdb?  Were the signatures for the overridden methods identical?  Unlike C++,
> Objective-C should still call the subclass' methods even if it was cast to a super class
> pointer.

I think the difference in the return values (long long vs unsigned long long) between the class and the subclass may have contributed to this issue.

[David Kilzer (:ddkilzer)]

(In reply to comment #48)
> (In reply to comment #47)
> > (In reply to comment #46)
> > > I was creating a WebDatabaseSecurityOrigin (a subclass of WebSecurityOrigin) and
> > > passing it along as a WebSecurityOrigin, and when functions were called on it
> > > that were implemented in the subclass, it didn't run the subclass' functions... it
> > > ran the empty ones.
> > > 
> > > I'm going to have to clean this up, but I appreciate any insight into why the
> > > subclasses' functions weren't called. I'll leave this open but clear the r+ flags so
> > > they don't show up in any queues.
> > 
> > Are you sure the object was a WebDatabaseSecurityOrigin?  What does "po <variable>"
> > say in gdb?  Were the signatures for the overridden methods identical?  Unlike C++,
> > Objective-C should still call the subclass' methods even if it was cast to a super class
> > pointer.
> 
> I think the difference in the return values (long long vs unsigned long long) between
> the class and the subclass may have contributed to this issue.

Nevermind, that doesn't make sense.  I also get compiler errors if the method signatures don't match.

I know this is a dumb question, but are you sure the #if ENABLE(DATABASE) macro was defined when compiling this file?  Were you able to step into the C++ code in those methods in gdb?

[Joseph Pecoraro]

> Are you sure the object was a WebDatabaseSecurityOrigin?  What does "po <variable>"
> say in gdb?  Were the signatures for the overridden methods identical?  Unlike C++,
> Objective-C should still call the subclass' methods even if it was cast to a super class
> pointer.

I'll have to check what "po" prints. I had concerns, so my diagnosis consisted of setting a
breakpoint on WebDatabaseSecurityOrigin's setQuota and WebSecurityOrigin's setQuota.
I expected the latter's to never be called... however it was called from DumpRenderTree's
UIDelegate exceededDatabaseQuotaForOrigin.

The code path would have been approximately along the lines of:

> WebDatabaseSecurityOrigin *webOrigin = [[WebDatabaseSecurityOrigin alloc] _initWithWebCoreSecurityOrigin:frame->document()->securityOrigin()];
> CallUIDelegate(m_webView, @selector(webView:frame:exceededDatabaseQuotaForSecurityOrigin:database:), kit(frame), webOrigin, (NSString *)databaseName);
> [webOrigin release];

I'll investigate this.


> I know this is a dumb question, but are you sure the #if ENABLE(DATABASE) macro
> was defined when compiling this file?  Were you able to step into the C++ code in
> those methods in gdb?

My breakpoint for WebDatabaseSecurityOrigin was hit and did go into the
DatabaseTracker code as expected in other cases.

[Joseph Pecoraro]

Created attachment 63254 [details]
[PATCH] Part 5 - Refactor Per Origin Quota Management

This factors out Quota Management from WebSecurityOrigin into managers.
Because now per-origin quotas are not just for Databases, but also for
ApplicationCaches, and maybe other things in the future.

[WebKit Review Bot]

Attachment 63254 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/mac/WebCoreSupport/WebSecurityOriginPrivate.h:34:  _applicationCacheQuotaManager is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebCoreSupport/WebSecurityOriginPrivate.h:35:  _databaseQuotaManager is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebCoreSupport/WebSecurityOriginPrivate.h:48:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Comment on attachment 63254 [details]
[PATCH] Part 5 - Refactor Per Origin Quota Management

> +// Returns the quota of this security origin in bytes.

Whoops, a leftover. This will be removed.

[Joseph Pecoraro]

Created attachment 63290 [details]
[PATCH] Part 5 - Refactor Per Origin Quota Management

I think those style errors are wrong but I fixed some other style issues.
Also, I made sure to release the new quota managers. Should I do the
same in finialize?

[Joseph Pecoraro]

Created attachment 63291 [details]
[PATCH] Part 6 - LayoutTest and Cleanup

This test the behavior, which is a good thing because the async
callback, as I suspected, caused issues. The cache group was being
deleted before the callback fired. Very bad. So I removed the callback
completely.

[WebKit Review Bot]

Attachment 63290 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/mac/WebCoreSupport/WebSecurityOriginPrivate.h:34:  _applicationCacheQuotaManager is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebCoreSupport/WebSecurityOriginPrivate.h:35:  _databaseQuotaManager is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebCoreSupport/WebSecurityOriginPrivate.h:48:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

I think this breaks older clients, so people using Webkit nightlies with Safari 5.0.1 would be broken.

General rule of thumb is we can make new WebKit apps that require a newer version of WebKit, but we can't make newer versions of WebKit break older WebKit apps.

You could leave the existing API around, leave a FIXME with a "this is deprecated" comment, and file a bug to remove it after a round or two of major releases.

[David Kilzer (:ddkilzer)]

Comment on attachment 63290 [details]
[PATCH] Part 5 - Refactor Per Origin Quota Management

r=me as long as you leave the old WebSecurityOrigin methods in place to make Safari 5 work with newer WebKit nightly builds.

[David Kilzer (:ddkilzer)]

Comment on attachment 63291 [details]
[PATCH] Part 6 - LayoutTest and Cleanup

r=me

[Joseph Pecoraro]

Created attachment 63391 [details]
[DIFF] These are the changes I added to make to leave the old interface intact.

Doing final building and testing.

[Joseph Pecoraro]

Landed the final Parts 5 and 6:

r64612 = af3ef94bc530d7f6e09d78c0509a0a42efd48b22
http://trac.webkit.org/changeset/64612

r64613 = 87ee39b6fff8dee09225a8ae49398457613ee265
http://trac.webkit.org/changeset/64613

Watching the bots for new test results.

[Eric Seidel (no email)]

FYI you can use "webkit-patch rebaseline" to pull down new results for tests which already have results.

Its an experimental command and won't necessarily do all that you want (like add new results when they're missing).  But it is helpful sometimes.  Patches welcome.

[WebKit Review Bot]

http://trac.webkit.org/changeset/64612 might have broken Qt Linux Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/64612
http://trac.webkit.org/changeset/64613

[WebKit Review Bot]

http://trac.webkit.org/changeset/64613 might have broken Qt Linux Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/64612
http://trac.webkit.org/changeset/64613

[Joseph Pecoraro]

Follow up patch skipping the tests on other platforms:
r64617 = 3aae04fe6480ebba40975848e4ad02acb355d135 (refs/remotes/trunk)
http://trac.webkit.org/changeset/64617

Filed bugs for other platforms:
Qt: https://bugs.webkit.org/show_bug.cgi?id=43455
Gtk: https://bugs.webkit.org/show_bug.cgi?id=43456
Win: https://bugs.webkit.org/show_bug.cgi?id=43458
Chromium: https://bugs.webkit.org/show_bug.cgi?id=43459

[Joseph Pecoraro]

yippie ki yi ay I think this can be closed!

[Alexey Proskuryakov]

There seem to be new crashes on BuildBot, could they be related to this change? See <http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r64630%20(18352)/>, but sadly, I don't have a stack trace.

[Joseph Pecoraro]

(In reply to comment #67)
> There seem to be new crashes on BuildBot, could they be related to this change? See 
> <http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r64630%20(18352)/>,
> but sadly, I don't have a stack trace.

It happened again on the bots run for r64645. So I opened:
<http://webkit.org/b/43506> http/tests/appcache/idempotent-update.html occasionally crashes on Leopard Bot