IndexedDB onupgradeneeded event has incorrect value for oldVersion

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Nolan Lawson]

The Safari/iOS 8 implementation of IndexedDB does not pass the first unit test in the PouchDB test suite. The database fails to open.

The test fails with the error message "An operation failed because the requested database object could not be found." This appears to be because the objectStore wasn't created in the onupgradeneeded method.

There is a GitHub issue full of testimonials and details here: https://github.com/pouchdb/pouchdb/issues/2533

You can run the relevant subset of the test suite online here: http://pouchtest.com/tests/test.html?grep=test.basics.js-local

Notice that that test suite passes perfectly in IE10, Chrome, and Firefox, but not Safari 8 (12A365). (It passes in Safari <=7 only because we fall back to Web SQL.)

Let me know if you need any more help. We are usually very receptive in the #pouchdb Freenode IRC channel. Thanks!

[Brady Eidson]

It would be helpful if you could attach a reduced, standalone test case to this bugzilla.

Makes it much easier to explore a bug compared to "Run this large, complex test suite"

[Radar WebKit Bug Importer]

<rdar://problem/18366739>

[Nolan Lawson]

(In reply to comment #1)
> It would be helpful if you could attach a reduced, standalone test case to this bugzilla.
> 
> Makes it much easier to explore a bug compared to "Run this large, complex test suite"

Sorry about that. Tried to write an isolated test, but haven't succeeded yet. I figured it was better to just notify y'all earlier rather than later.

[Brady Eidson]

(In reply to comment #3)
> (In reply to comment #1)
> > It would be helpful if you could attach a reduced, standalone test case to this bugzilla.
> > 
> > Makes it much easier to explore a bug compared to "Run this large, complex test suite"
> 
> Sorry about that. Tried to write an isolated test, but haven't succeeded yet. I figured it was better to just notify y'all earlier rather than later.

And don't get me wrong, that is appreciated :)

Just more likely to get eyes on it sooner if there was a reduction.

[Zaid]

There are two issues with the Safari implementation:

1. The initial version of the database reported in onupgradeneeded is HUGE and positive instead of zero/undefined. This creates issues when trying to determine which code path to run for the migration. I currently have a workaround of assuming current_version = 0 when current_version > 1000

2. Safari seems to run into problems when the database name contains "-" in it. Workaround is easy here, just don't use it.

[Brady Eidson]

(In reply to comment #5)
> There are two issues with the Safari implementation:
> 
> 1. The initial version of the database reported in onupgradeneeded is HUGE and positive instead of zero/undefined. This creates issues when trying to determine which code path to run for the migration. I currently have a workaround of assuming current_version = 0 when current_version > 1000
> 
> 2. Safari seems to run into problems when the database name contains "-" in it. Workaround is easy here, just don't use it.

Those are two separate bugs from this one.  Could you please file each as a separate bug?

We know about the initial version problem, but I don't think we have a bugzilla for it yet.

[Nolan Lawson]

I'm sorry, do you want me to file the separate issue for the issue of "-" being in the database name? (Also FWIW I cannot seem to reproduce that, and in PouchDB our db names don't have "-" in them...)

In any case, for this error, we managed to write a small isolated test that demonstrates the issue. Just go here, and if you see "everything is A-OK", then the test passed: http://bl.ocks.org/nolanlawson/raw/c83e9039edf2278047e9/

[Brady Eidson]

(In reply to comment #7)
> I'm sorry, do you want me to file the separate issue for the issue of "-" being in the database name? (Also FWIW I cannot seem to reproduce that, and in PouchDB our db names don't have "-" in them...)
> 
> In any case, for this error, we managed to write a small isolated test that demonstrates the issue. Just go here, and if you see "everything is A-OK", then the test passed: http://bl.ocks.org/nolanlawson/raw/c83e9039edf2278047e9/

Nolan, I wasn't asking anything of you.

Zaid was the one who jumped into this bug and reported two unrelated issues.  I was suggesting he file bugs for the unrelated issues he sees.

Thanks for the test case!

[Nolan Lawson]

Okay, just checking!

Also FYI, this seems to be a duplicate of this bug: https://bugs.webkit.org/show_bug.cgi?id=136937

[Robert Knight]

>  The initial version of the database reported in onupgradeneeded is HUGE and positive instead of zero/undefined.

Specifically, the initial value I see on Safari 7.1 is 9223372036854776000 (2^63 - 192)

[Robert Knight]

(In reply to comment #10)
> >  The initial version of the database reported in onupgradeneeded is HUGE and positive instead of zero/undefined.
> 
> Specifically, the initial value I see on Safari 7.1 is 9223372036854776000 (2^63 - 192)

Gah, I meant (2^63 + 192)

[Maciej Stachowiak]

New Radar ID is <rdar://problem/18309792>

[Brady Eidson]

Created attachment 254759 [details]
Reduced test case from Nolan

This is basically Nolan's blocks.org test case, but just reduced, code-styled, and in one file.

It fails not because of a problem with onupgradeneeded, but because it opens a transaction to multiple object stores.

That's already covered in https://bugs.webkit.org/show_bug.cgi?id=136937

So we'll clarify this bug is about one issue, which is that when onupgradeneeded does fire on a new database that the oldversion property is wrong.

[Brady Eidson]

Retitling:
IndexedDB onupgradeneeded event has incorrect value for oldVersion

[Brady Eidson]

Created attachment 254761 [details]
Patch v1

[Brady Eidson]

http://trac.webkit.org/changeset/185480

[Nolan Lawson]

Sorry for causing confusion, and thanks for the fix!

[Nolan Lawson]

I created a new bl.ocks based on the Webkit patch. This one actually reproduces 136888, as opposed to the other bug: http://bl.ocks.org/nolanlawson/5e397897633f9e16eb42. (I know it's already fixed, but I like having a link I can just click and open to test.)

And thanks for your tireless efforts with these IDB bugs, Brady! Any chance they'll make it into the iOS 9 release? ;)

[Brady Eidson]

(In reply to comment #18)
> I created a new bl.ocks based on the Webkit patch. This one actually
> reproduces 136888, as opposed to the other bug:
> http://bl.ocks.org/nolanlawson/5e397897633f9e16eb42. (I know it's already
> fixed, but I like having a link I can just click and open to test.)
> 
> And thanks for your tireless efforts with these IDB bugs, Brady! Any chance
> they'll make it into the iOS 9 release? ;)

This is the WebKit project, not responsible for product releases like iOS or OS X, etc etc.

Apple generally does not comment on future product releases, etc etc.

I believe iOS 9 has been released as a first developer beta, and Apple has announced future updates to that beta as the year progresses. These facts might suggest that iOS 9 is not complete yet.