36671 2010-03-26 12:29:43 -0700 window.openDatabase() always fails for new databases when using WebKit nightly with Safari 4.0.5 2010-04-16 15:12:04 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) All All RESOLVED FIXED InRadar P1 Normal --- 1 dumi ericu ap aroben beidson commit-queue dimich ericu laszlo.gombos levin webkit.review.bot oldest_to_newest 204712 0 dumi 2010-03-26 12:29:43 -0700 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. 204715 1 aroben 2010-03-26 12:36:43 -0700 <rdar://problem/7798809> 208044 2 beidson 2010-04-02 13:33:11 -0700 Still tracing through code now, but this probably broke in http://trac.webkit.org/changeset/56293 208048 3 dimich 2010-04-02 13:47:27 -0700 Just a data point: this issue does not appear in ToT Webkit + Safari 4.0.4 208050 4 beidson 2010-04-02 13:48:22 -0700 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. 208052 5 beidson 2010-04-02 13:49:52 -0700 "to be created", of course. 208054 6 beidson 2010-04-02 13:52:25 -0700 (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. 208064 7 ericu 2010-04-02 14:17:05 -0700 (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. 208065 8 beidson 2010-04-02 14:21:20 -0700 (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. 208092 9 52455 ericu 2010-04-02 15:16:08 -0700 Created attachment 52455 Patch 208095 10 52455 ericu 2010-04-02 15:17:29 -0700 Comment on attachment 52455 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. 208103 11 52455 beidson 2010-04-02 15:23:52 -0700 Comment on attachment 52455 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. 208104 12 52455 beidson 2010-04-02 15:24:07 -0700 Comment on attachment 52455 Patch Actually setting r+ this time. :) 208106 13 ericu 2010-04-02 15:27:09 -0700 I'm actually not a committer yet; could you please cq+ or commit? 208132 14 beidson 2010-04-02 16:18:09 -0700 Done. 208166 15 52455 commit-queue 2010-04-02 17:16:47 -0700 Comment on attachment 52455 Patch Clearing flags on attachment: 52455 Committed r57036: <http://trac.webkit.org/changeset/57036> 208168 16 commit-queue 2010-04-02 17:16:54 -0700 All reviewed patches have been landed. Closing bug. 208464 17 52455 aroben 2010-04-05 07:04:48 -0700 Comment on attachment 52455 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. 208500 18 beidson 2010-04-05 08:52:04 -0700 Indeed, Adam is totally correct. Keen eye, thank you! 208660 19 52562 ericu 2010-04-05 13:11:40 -0700 Created attachment 52562 Fix the fix. D'oh! Here's the right fix. 208674 20 52562 beidson 2010-04-05 13:41:32 -0700 Comment on attachment 52562 Fix the fix. Yup. 208919 21 52562 commit-queue 2010-04-05 23:33:08 -0700 Comment on attachment 52562 Fix the fix. Clearing flags on attachment: 52562 Committed r57128: <http://trac.webkit.org/changeset/57128> 208920 22 commit-queue 2010-04-05 23:33:14 -0700 All reviewed patches have been landed. Closing bug. 210637 23 dumi 2010-04-09 01:56:07 -0700 This is still an issue in Safari + ToT WebKit on Windows. 210766 24 beidson 2010-04-09 09:37:04 -0700 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. 210839 25 ericu 2010-04-09 12:13:34 -0700 (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. 210865 26 dumi 2010-04-09 13:07:40 -0700 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. 210867 27 aroben 2010-04-09 13:13:32 -0700 (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. 212619 28 53376 dumi 2010-04-14 15:34:22 -0700 Created attachment 53376 another patch Tested on Windows Vista. 212620 29 webkit.review.bot 2010-04-14 15:35:32 -0700 Attachment 53376 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. 212622 30 dumi 2010-04-14 15:37:03 -0700 Will fix the TAB in WebCore/ChangeLog when landing, unless there are other comments. 213385 31 53376 ap 2010-04-16 13:18:07 -0700 Comment on attachment 53376 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. 213446 32 dumi 2010-04-16 15:12:04 -0700 Second patch landed as r57754. 52455 2010-04-02 15:16:08 -0700 2010-04-05 07:04:48 -0700 Patch bug-36671-20100402151607.patch text/plain 1743 ericu SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA1NzAyNSkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTUgQEAKKzIwMTAtMDQtMDIgIEVyaWMgVWhyaGFuZSAgPGVyaWN1QGNocm9taXVt Lm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICB3 aW5kb3cub3BlbkRhdGFiYXNlKCkgYWx3YXlzIGZhaWxzIGZvciBuZXcgZGF0YWJhc2VzIHdoZW4g dXNpbmcgV2ViS2l0IG5pZ2h0bHkgd2l0aCBTYWZhcmkgNC4wLjUuICBUaGlzIGlzIGNhdXNlZCBi eSBhIFNlY3VyaXR5T3JpZ2luIHBvaW50ZXIgY29tcGFyaXNvbiB0aGF0IEkgc2hvdWxkIGhhdmUg c3dpdGNoZWQgdG8gYmUgYSBoYXNoIGNvbXBhcmlzb24gaW4gcjU2MjkzIFtidWcgMzQ5OTFdLgor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MzY2NzEKKwor ICAgICAgICBObyBuZXcgdGVzdHMuICBSZXF1aXJlcyB0ZXN0aW5nIG9uIFNhZmFyaSBvbiBXaW5k b3dzLgorCisgICAgICAgICogc3RvcmFnZS9EYXRhYmFzZVRyYWNrZXIuY3BwOgorICAgICAgICAo V2ViQ29yZTo6RGF0YWJhc2VUcmFja2VyOjpmdWxsUGF0aEZvckRhdGFiYXNlTm9Mb2NrKTogQ29u dmVydCBhIHBvaW50ZXIgY29tcGFyaXNvbiB0byB1c2UgU2VjdXJpdHlPcmlnaW5IYXNoOjpoYXNo KCkgaW5zdGVhZCwgYW5kIG1vdmUgaXQgdG8gdGhlIGVuZCBvZiB0aGUgY2xhdXNlIGZvciBzcGVl ZCBpbiB0aGUgZWFzeS1vdXQgY2FzZS4KKwogMjAxMC0wNC0wMiAgQW5kcmV3IFNjaGVya3VzICA8 c2NoZXJrdXNAY2hyb21pdW0ub3JnPgogCiAgICAgICAgIFJldmlld2VkIGJ5IEVyaWMgQ2FybHNv biBhbmQgRXJpYyBTZWlkZWwuCkluZGV4OiBXZWJDb3JlL3N0b3JhZ2UvRGF0YWJhc2VUcmFja2Vy LmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09Ci0tLSBXZWJDb3JlL3N0b3JhZ2UvRGF0YWJhc2VUcmFja2VyLmNwcAko cmV2aXNpb24gNTcwMjQpCisrKyBXZWJDb3JlL3N0b3JhZ2UvRGF0YWJhc2VUcmFja2VyLmNwcAko d29ya2luZyBjb3B5KQpAQCAtMjM0LDcgKzIzNCw3IEBAIFN0cmluZyBEYXRhYmFzZVRyYWNrZXI6 OmZ1bGxQYXRoRm9yRGF0YWIKICAgICBBU1NFUlQoIW9yaWdpblF1b3RhTWFuYWdlck5vTG9jaygp LnRyeUxvY2soKSk7CiAKICAgICBmb3IgKEhhc2hTZXQ8UHJvcG9zZWREYXRhYmFzZSo+OjppdGVy YXRvciBpdGVyID0gbV9wcm9wb3NlZERhdGFiYXNlcy5iZWdpbigpOyBpdGVyICE9IG1fcHJvcG9z ZWREYXRhYmFzZXMuZW5kKCk7ICsraXRlcikKLSAgICAgICAgaWYgKCgqaXRlciktPmZpcnN0ID09 IG9yaWdpbiAmJiAoKml0ZXIpLT5zZWNvbmQubmFtZSgpID09IG5hbWUpCisgICAgICAgIGlmICgo Kml0ZXIpLT5zZWNvbmQubmFtZSgpID09IG5hbWUgJiYgU2VjdXJpdHlPcmlnaW5IYXNoOjpoYXNo KCgqaXRlciktPmZpcnN0KSA9PSBTZWN1cml0eU9yaWdpbkhhc2g6Omhhc2gob3JpZ2luKSkKICAg ICAgICAgICAgIHJldHVybiBTdHJpbmcoKTsKIAogICAgIFN0cmluZyBvcmlnaW5JZGVudGlmaWVy ID0gb3JpZ2luLT5kYXRhYmFzZUlkZW50aWZpZXIoKTsK 52562 2010-04-05 13:11:40 -0700 2010-04-05 23:33:08 -0700 Fix the fix. 36671.patch text/plain 1596 ericu SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA1NzA5MSkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTcgQEAKKzIwMTAtMDQtMDUgIEVyaWMgVWhyaGFuZSAgPGVyaWN1QGNocm9taXVt Lm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICB3 aW5kb3cub3BlbkRhdGFiYXNlKCkgYWx3YXlzIGZhaWxzIGZvciBuZXcgZGF0YWJhc2VzIHdoZW4g dXNpbmcgV2ViS2l0IG5pZ2h0bHkgd2l0aCBTYWZhcmkgNC4wLjUKKyAgICAgICAgaHR0cHM6Ly9i dWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTM2NjcxCisKKyAgICAgICAgVGhlIHByZXZp b3VzICJmaXgiIEkgbWlzdGFrZW5seSBjb21wYXJlZCBoYXNoZXMsIGFuIGltcHJvdmVtZW50IG92 ZXIgY29tcGFyaW5nIHBvaW50ZXJzLCBidXQgc3RpbGwgbm90IHJpZ2h0LgorCisgICAgICAgIE5v IG5ldyB0ZXN0cy4KKworICAgICAgICAqIHN0b3JhZ2UvRGF0YWJhc2VUcmFja2VyLmNwcDoKKyAg ICAgICAgKFdlYkNvcmU6OkRhdGFiYXNlVHJhY2tlcjo6ZnVsbFBhdGhGb3JEYXRhYmFzZU5vTG9j ayk6IFVzZSBTZWN1cml0eU9yaWdpbjo6ZXF1YWwgdG8gY29tcGFyZSBpbnN0ZWFkIG9mIFNlY3Vy aXR5T3JpZ2luSGFzaC4KKwogMjAxMC0wNC0wNSAgQWRhbSBUcmVhdCAgPGF0cmVhdEByaW0uY29t PgogCiAgICAgICAgIFJldmlld2VkIGJ5IERhcmluIEFkbGVyLgpJbmRleDogV2ViQ29yZS9zdG9y YWdlL0RhdGFiYXNlVHJhY2tlci5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9zdG9yYWdlL0Rh dGFiYXNlVHJhY2tlci5jcHAJKHJldmlzaW9uIDU3MDg1KQorKysgV2ViQ29yZS9zdG9yYWdlL0Rh dGFiYXNlVHJhY2tlci5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTIzNCw3ICsyMzQsNyBAQCBTdHJp bmcgRGF0YWJhc2VUcmFja2VyOjpmdWxsUGF0aEZvckRhdGFiCiAgICAgQVNTRVJUKCFvcmlnaW5R dW90YU1hbmFnZXJOb0xvY2soKS50cnlMb2NrKCkpOwogCiAgICAgZm9yIChIYXNoU2V0PFByb3Bv c2VkRGF0YWJhc2UqPjo6aXRlcmF0b3IgaXRlciA9IG1fcHJvcG9zZWREYXRhYmFzZXMuYmVnaW4o KTsgaXRlciAhPSBtX3Byb3Bvc2VkRGF0YWJhc2VzLmVuZCgpOyArK2l0ZXIpCi0gICAgICAgIGlm ICgoKml0ZXIpLT5zZWNvbmQubmFtZSgpID09IG5hbWUgJiYgU2VjdXJpdHlPcmlnaW5IYXNoOjpo YXNoKCgqaXRlciktPmZpcnN0KSA9PSBTZWN1cml0eU9yaWdpbkhhc2g6Omhhc2gob3JpZ2luKSkK KyAgICAgICAgaWYgKCgqaXRlciktPnNlY29uZC5uYW1lKCkgPT0gbmFtZSAmJiAoKml0ZXIpLT5m aXJzdC0+ZXF1YWwob3JpZ2luKSkKICAgICAgICAgICAgIHJldHVybiBTdHJpbmcoKTsKIAogICAg IFN0cmluZyBvcmlnaW5JZGVudGlmaWVyID0gb3JpZ2luLT5kYXRhYmFzZUlkZW50aWZpZXIoKTsK 53376 2010-04-14 15:34:22 -0700 2010-04-16 13:18:06 -0700 another patch patch text/plain 1365 dumi SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA1NzYxMykKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTQgQEAKKzIwMTAtMDQtMTQgIER1bWl0cnUgRGFuaWxpdWMgIDxkdW1pQGNocm9t aXVtLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICBNYWtlIFNhZmFyaSBjb3JyZWN0bHkgYWxsb2NhdGUgNU1CIG9mIERCIHN0b3JhZ2UgdG8gYWxs IG5ldworCW9yaWdpbnMuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVn LmNnaT9pZD0zNjY3MQorCisgICAgICAgICogc3RvcmFnZS9EYXRhYmFzZVRyYWNrZXIuY3BwOgor ICAgICAgICAoV2ViQ29yZTo6RGF0YWJhc2VUcmFja2VyOjpkZXRhaWxzRm9yTmFtZUFuZE9yaWdp bik6CisKIDIwMTAtMDQtMTQgIFN0ZXZlIEZhbGtlbmJ1cmcgIDxzZmFsa2VuQGFwcGxlLmNvbT4K IAogICAgICAgICBSZXZpZXdlZCBieSBEYW4gQmVybnN0ZWluLgpJbmRleDogV2ViQ29yZS9zdG9y YWdlL0RhdGFiYXNlVHJhY2tlci5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9zdG9yYWdlL0Rh dGFiYXNlVHJhY2tlci5jcHAJKHJldmlzaW9uIDU3NjEzKQorKysgV2ViQ29yZS9zdG9yYWdlL0Rh dGFiYXNlVHJhY2tlci5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTM3Nyw3ICszNzcsNyBAQCBEYXRh YmFzZURldGFpbHMgRGF0YWJhc2VUcmFja2VyOjpkZXRhaWxzCiAgICAgICAgIE11dGV4TG9ja2Vy IGxvY2tEYXRhYmFzZShtX2RhdGFiYXNlR3VhcmQpOwogCiAgICAgICAgIGZvciAoSGFzaFNldDxQ cm9wb3NlZERhdGFiYXNlKj46Oml0ZXJhdG9yIGl0ZXIgPSBtX3Byb3Bvc2VkRGF0YWJhc2VzLmJl Z2luKCk7IGl0ZXIgIT0gbV9wcm9wb3NlZERhdGFiYXNlcy5lbmQoKTsgKytpdGVyKQotICAgICAg ICAgICAgaWYgKCgqaXRlciktPmZpcnN0ID09IG9yaWdpbiAmJiAoKml0ZXIpLT5zZWNvbmQubmFt ZSgpID09IG5hbWUpIHsKKyAgICAgICAgICAgIGlmICgoKml0ZXIpLT5zZWNvbmQubmFtZSgpID09 IG5hbWUgJiYgKCppdGVyKS0+Zmlyc3QtPmVxdWFsKG9yaWdpbikpIHsKICAgICAgICAgICAgICAg ICBBU1NFUlQoKCppdGVyKS0+c2Vjb25kLnRocmVhZCgpID09IGN1cnJlbnRUaHJlYWQoKSk7CiAg ICAgICAgICAgICAgICAgcmV0dXJuICgqaXRlciktPnNlY29uZDsKICAgICAgICAgICAgIH0K