1. Delete all Safari HTML5 databases. 2. WebKitTools/Scripts/build-webkit --debug (not sure if the --debug flag changes anything, but i haven't tried this in release mode) 3. WebKitTools/Scripts/run-safari --debug 4. Run any code that tries to open a database. RESULTS: openDatabase() fails (returns NULL) EXPECTED: openDatabase() succeeds. PROBLEM: When we try to open a database in a new origin (that is not in the tracker database Databases.db), DatabaseTracker::canEstablishDatabase() makes a call to ScriptExecutionContext::databaseExceededQuota(), which eventually calls WebChromeClient::exceededDatabaseQuota(), which is supposed to give this new origin 5MB of space. That does not happen: WebChromeClient::exceededDatabaseQuota() calls some private delegate, which does not change anything, as far as the database code is concerned. This problem does not exist in Safari. It only shows up when running Safari using the WebKit library built from the open source tree.
<rdar://problem/7798809>
Still tracing through code now, but this probably broke in http://trac.webkit.org/changeset/56293
Just a data point: this issue does not appear in ToT Webkit + Safari 4.0.4
I still don't know *for sure* that this was 56293, but am still highly suspicious. What's happening is that in WebChromeClient::exceededDatabaseQuota(), Safari is asking for the current details for the database using DatabaseTracker::detailsForNameAndOrigin(). Previously, Safari would actually get a valid answer from WebKit. Now, it's getting nothing back because the "too be created" database isn't found in m_proposedDatabases.
"to be created", of course.
(In reply to comment #3) > Just a data point: this issue does not appear in ToT Webkit + Safari 4.0.4 Yah. I haven't tracked down exactly what changed in Safari 4.0.5 on Windows quite yet (note it doesn't even affect Safari 4.0.5 on Mac.) But - as my previous comments show - I think I have the specific WebKit change in behavior narrowed down. Still working on it.
(In reply to comment #4) > I still don't know *for sure* that this was 56293, but am still highly > suspicious. > > What's happening is that in WebChromeClient::exceededDatabaseQuota(), Safari is > asking for the current details for the database using > DatabaseTracker::detailsForNameAndOrigin(). > > Previously, Safari would actually get a valid answer from WebKit. > > Now, it's getting nothing back because the "too be created" database isn't > found in m_proposedDatabases. I haven't tried running a fix yet, but I'll bet this is the bug: the search through m_proposed databases uses '==' to compare the SecurityOrigins, but it's stored a threadsafeCopy, so a pointer comparison always fails. It should be using SecurityOriginHash to compare them. I'll go get set up to test a fix.
(In reply to comment #7) > > I haven't tried running a fix yet, but I'll bet this is the bug: the search > through m_proposed databases uses '==' to compare the SecurityOrigins, but it's > stored a threadsafeCopy, so a pointer comparison always fails. It should be > using SecurityOriginHash to compare them. I'll go get set up to test a fix. I had suspected this and just confirmed it stepping through. Thanks for working on a fix - I'll be ready to strike on the review.
Created attachment 52455 [details] Patch
Comment on attachment 52455 [details] Patch Brady--I do my webkit dev on a Mac, so I can't verify that this patch fixes the bug. Would you mind doing a quick test? I've at least made sure it does no harm.
Comment on attachment 52455 [details] Patch r+ I can't get to testing it on Windows this instant, but I agree with the whole line of thinking about the source of the bug, and there's definitely no harm in this change. In fact, I can tell you that Safari Mac working and your patch was an accident before, and this makes it work "the old, more correct way" :) Go ahead and land it and I'll confirm on Windows later.
Comment on attachment 52455 [details] Patch Actually setting r+ this time. :)
I'm actually not a committer yet; could you please cq+ or commit?
Done.
Comment on attachment 52455 [details] Patch Clearing flags on attachment: 52455 Committed r57036: <http://trac.webkit.org/changeset/57036>
All reviewed patches have been landed. Closing bug.
Comment on attachment 52455 [details] Patch > for (HashSet<ProposedDatabase*>::iterator iter = m_proposedDatabases.begin(); iter != m_proposedDatabases.end(); ++iter) > - if ((*iter)->first == origin && (*iter)->second.name() == name) > + if ((*iter)->second.name() == name && SecurityOriginHash::hash((*iter)->first) == SecurityOriginHash::hash(origin)) > return String(); Having identical hash codes does not guarantee equality. I think you should be using SecurityOrigin::equal instead.
Indeed, Adam is totally correct. Keen eye, thank you!
Created attachment 52562 [details] Fix the fix. D'oh! Here's the right fix.
Comment on attachment 52562 [details] Fix the fix. Yup.
Comment on attachment 52562 [details] Fix the fix. Clearing flags on attachment: 52562 Committed r57128: <http://trac.webkit.org/changeset/57128>
This is still an issue in Safari + ToT WebKit on Windows.
Hopefully Eric U can revisit this asap, because I won't be able to get to it for a bit. Why on Earth are any layout tests failing? I checked the windows skipped list and I don't think any of them are skipped.
(In reply to comment #24) > Hopefully Eric U can revisit this asap, because I won't be able to get to it > for a bit. What with the webkit meeting and some urgent personal business taking me out of town, I'm afraid I won't be able to look at this until at least 4/22. > Why on Earth are any layout tests failing? I checked the windows skipped list > and I don't think any of them are skipped. That may be Safari-only code; I'm not sure the layout tests invoke that particular path.
I'll see if I can fix this today. Layout tests don't fail because they run in DRT, which correctly allocates 5MB to a new origin.
(In reply to comment #26) > I'll see if I can fix this today. Layout tests don't fail because they run in > DRT, which correctly allocates 5MB to a new origin. Maybe we need to add a way to make DRT act like Safari in this respect, so that we can test this.
Created attachment 53376 [details] another patch Tested on Windows Vista.
Attachment 53376 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/ChangeLog:6: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Will fix the TAB in WebCore/ChangeLog when landing, unless there are other comments.
Comment on attachment 53376 [details] another patch r=me Please fix the tab, and also please explain the fix in ChangeLog. It looks like it's changing the same code that the previous patch changed, but it's actually applying the same fix at another location.
Second patch landed as r57754.