Bug 136888 - IndexedDB onupgradeneeded event has incorrect value for oldVersion
Summary: IndexedDB onupgradeneeded event has incorrect value for oldVersion
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: iPhone / iPad Unspecified
: P2 Major
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-09-17 08:04 PDT by Nolan Lawson
Modified: 2019-07-04 00:35 PDT (History)
10 users (show)

See Also:


Attachments
Reduced test case from Nolan (957 bytes, text/html)
2015-06-11 14:30 PDT, Brady Eidson
no flags Details
Patch v1 (15.79 KB, patch)
2015-06-11 15:13 PDT, Brady Eidson
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nolan Lawson 2014-09-17 08:04:12 PDT
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!
Comment 1 Brady Eidson 2014-09-17 08:55:16 PDT
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"
Comment 2 Radar WebKit Bug Importer 2014-09-17 09:40:34 PDT
<rdar://problem/18366739>
Comment 3 Nolan Lawson 2014-09-17 10:46:44 PDT
(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.
Comment 4 Brady Eidson 2014-09-17 10:49:00 PDT
(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.
Comment 5 Zaid 2014-09-18 07:42:39 PDT
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.
Comment 6 Brady Eidson 2014-09-18 08:55:33 PDT
(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.
Comment 7 Nolan Lawson 2014-09-20 16:23:07 PDT
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/
Comment 8 Brady Eidson 2014-09-20 16:30:38 PDT
(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!
Comment 9 Nolan Lawson 2014-09-20 16:32:31 PDT
Okay, just checking!

Also FYI, this seems to be a duplicate of this bug: https://bugs.webkit.org/show_bug.cgi?id=136937
Comment 10 Robert Knight 2014-10-06 06:18:58 PDT
>  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)
Comment 11 Robert Knight 2014-10-06 06:19:49 PDT
(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)
Comment 12 Maciej Stachowiak 2015-06-11 12:34:33 PDT
New Radar ID is <rdar://problem/18309792>
Comment 13 Brady Eidson 2015-06-11 14:30:08 PDT
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.
Comment 14 Brady Eidson 2015-06-11 14:31:15 PDT
Retitling:
IndexedDB onupgradeneeded event has incorrect value for oldVersion
Comment 15 Brady Eidson 2015-06-11 15:13:41 PDT
Created attachment 254761 [details]
Patch v1
Comment 16 Brady Eidson 2015-06-11 17:18:31 PDT
http://trac.webkit.org/changeset/185480
Comment 17 Nolan Lawson 2015-06-12 07:03:27 PDT
Sorry for causing confusion, and thanks for the fix!
Comment 18 Nolan Lawson 2015-06-14 14:21:09 PDT
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? ;)
Comment 19 Brady Eidson 2015-06-15 09:29:05 PDT
(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.