Description
Joseph Pecoraro
2010-06-15 09:21:07 PDT
Created attachment 58788 [details]
[PATCH] Part 1 - Add Per-Origin quota logic, and persistence.
Created attachment 58789 [details]
[PATCH] Part 2 - Add Total and Per-Origin Quota Settings. (needs part 1)
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.
Attachment 58788 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/3267164 Attachment 58788 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/3267165 Looks like INT64_MAX was missing for gtk and qt. Probably a missing include. 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.
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.
Attachment 58797 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/3316184 Attachment 58797 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/3331193 Attachment 58788 [details] did not build on win: Build output: http://webkit-commit-queue.appspot.com/results/3312188 Attachment 58797 [details] did not build on win: Build output: http://webkit-commit-queue.appspot.com/results/3294196 "#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? 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.
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.
Attachment 58849 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/3320208 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. (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? 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.
Attachment 59425 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/3316585 Attachment 59425 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/3282575 Attachment 59425 [details] did not build on win: Build output: http://webkit-commit-queue.appspot.com/results/3279581 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.
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`
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.
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!
> 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! =)
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. > > 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.
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.
(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. (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. 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
> 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! Comment on attachment 59693 [details]
[PATCH] Part 3 - Refactor storeNewestCache to allow Failure Reason Output (needs part 2)
r=me
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. :)
(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. 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.
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.
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! 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 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 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 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 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. (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. (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. (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? > 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. 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.
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.
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. 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?
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.
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.
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. 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.
Comment on attachment 63291 [details]
[PATCH] Part 6 - LayoutTest and Cleanup
r=me
Created attachment 63391 [details]
[DIFF] These are the changes I added to make to leave the old interface intact.
Doing final building and testing.
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. 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. 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 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 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 yippie ki yi ay I think this can be closed! 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. (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 |