Summary: | window.openDatabase() always fails for new databases when using WebKit nightly with Safari 4.0.5 | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dumitru Daniliuc <dumi> | ||||||||
Component: | New Bugs | Assignee: | Eric U. <ericu> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap, aroben, beidson, commit-queue, dimich, ericu, laszlo.gombos, levin, webkit.review.bot | ||||||||
Priority: | P1 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Dumitru Daniliuc
2010-03-26 12:29:43 PDT
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> All reviewed patches have been landed. Closing bug. 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. |