Bug 36671 – window.openDatabase() always fails for new databases when using WebKit nightly with Safari 4.0.5

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dumitru Daniliuc]

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.

[Adam Roben (:aroben)]

<rdar://problem/7798809>

[Brady Eidson]

Still tracing through code now, but this probably broke in http://trac.webkit.org/changeset/56293

[Dmitry Titov]

Just a data point: this issue does not appear in ToT Webkit + Safari 4.0.4

[Brady Eidson]

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.

[Brady Eidson]

"to be created", of course.

[Brady Eidson]

(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.

[Eric U.]

(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.

[Brady Eidson]

(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.

[Eric U.]

Created attachment 52455 [details]
Patch

[Eric U.]

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.

[Brady Eidson]

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.

[Brady Eidson]

Comment on attachment 52455 [details]
Patch

Actually setting r+ this time.  :)

[Eric U.]

I'm actually not a committer yet; could you please cq+ or commit?

[Brady Eidson]

Done.

[WebKit Commit Bot]

Comment on attachment 52455 [details]
Patch

Clearing flags on attachment: 52455

Committed r57036: <http://trac.webkit.org/changeset/57036>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Roben (:aroben)]

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.

[Brady Eidson]

Indeed, Adam is totally correct.  Keen eye, thank you!

[Eric U.]

Created attachment 52562 [details]
Fix the fix.

D'oh!

Here's the right fix.

[Brady Eidson]

Comment on attachment 52562 [details]
Fix the fix.

Yup.

[WebKit Commit Bot]

Comment on attachment 52562 [details]
Fix the fix.

Clearing flags on attachment: 52562

Committed r57128: <http://trac.webkit.org/changeset/57128>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dumitru Daniliuc]

This is still an issue in Safari + ToT WebKit on Windows.

[Brady Eidson]

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.

[Eric U.]

(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.

[Dumitru Daniliuc]

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.

[Adam Roben (:aroben)]

(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.

[Dumitru Daniliuc]

Created attachment 53376 [details]
another patch

Tested on Windows Vista.

[WebKit Review Bot]

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.

[Dumitru Daniliuc]

Will fix the TAB in WebCore/ChangeLog when landing, unless there are other comments.

[Alexey Proskuryakov]

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.

[Dumitru Daniliuc]

Second patch landed as r57754.