Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unable to exceed 5MBs on Safari desktop 7.0.2 #115

Closed
andygup opened this issue Mar 22, 2014 · 15 comments
Closed

Unable to exceed 5MBs on Safari desktop 7.0.2 #115

andygup opened this issue Mar 22, 2014 · 15 comments

Comments

@andygup
Copy link

andygup commented Mar 22, 2014

Is this repo being maintained?? I see months old issues that have gone unanswered.

In regards to my error, I get the prompt from Safari to allow 10 additional megabytes and then almost immediately get an "undefined" error back.

@axemclion
Copy link
Collaborator

@DickvdBrink has been wonderful and responding to pull requests.

The problem with additional quota on safari could be due to a recent quota bug.

@andygup
Copy link
Author

andygup commented Mar 24, 2014

@axemclion thank you for the quick response. This could be a show-stopper level bug for us since Safari users represents a significant and important part of our total user base.

@DickvdBrink is this a bug you are already planning to look into? Do you have any additional thoughts or guidance on where in the library this problem may be occurring? Or, is this a bug within the Safari browser, itself?

@axemclion
Copy link
Collaborator

@andygup Do you have a sample that can repro this scenario? I am not sure if we can really help it if this is a Safari bug.

@andygup
Copy link
Author

andygup commented Mar 25, 2014

@axemclion Yes, you can clone the following repo. Run index.html then select the "Get Tiles" button on the top right of the app. It will download more than enough data to trigger the initial approve-extra-storage message: https://github.com/andygup/appcache-test-js/tree/no_manifest

@DickvdBrink
Copy link
Collaborator

I hope I have some time to look into it maybe tonight :)
I haven't noticed this issue yet because I'm using cordova (phonegap) for my projects which doesn't have this limit.

Btw: you might want to update the library because you are missing commit: f417998#diff-5ed5058b24e9f5f1400c8ef330fd540cR109 so the cursor.update doesn't work.

@DickvdBrink
Copy link
Collaborator

I could not reproduce the issue on Chrome (I forced it to use the shim) so I need some more time :(
I do not own a Mac so I couldn't test Safari, but maybe I can do some test on my iPad tomorrow

@andygup
Copy link
Author

andygup commented Mar 26, 2014

@DickvdBrink thanks for heads up on the missing commit. I'll give that a try.

@andygup
Copy link
Author

andygup commented Mar 26, 2014

I locally tested with the latest version including cursor.update commit. No go. Still fails after 5MBs.

@andygup
Copy link
Author

andygup commented Mar 27, 2014

@DickvdBrink At IndexedDBShim.js#L1467 I was able to hard code a new DEFAULT_DB_SIZE to one that's more appropriate for the size of our data and was able to bypass the bug (for now). Credits go to https://github.com/odoe

I used:

var DEFAULT_DB_SIZE = 25 * 1024 * 1024;

@axemclion
Copy link
Collaborator

@andygup Can you send this in as a pull request? Btw, does this size have any problems on other databases? Of not, could we use this as the default?

@andygup
Copy link
Author

andygup commented Mar 27, 2014

@axemclion I only tested this using Safari on a Mac and an iPad so I'm not sure about other databases. So far, I have only run into the problem on Safari.

I propose I can add a note in the README for now rather than hard code change in IndexedDBShim.js. Does this sounds okay to you to just change the README and do a pull request?

I believe there needs to be more research to find out if this is a known Safari bug and if there are any plans for Apple to fix it. That will take more time than simply updating the README for now.

@andygup
Copy link
Author

andygup commented Mar 27, 2014

I have submitted Bug 16450805 to Apple. I'll post back when/if I get an update.

@DickvdBrink
Copy link
Collaborator

Thanks andygup for the info. Changing this default results in a popup asking if you want to increase the storage on startup right?
btw: I was able to reproduce the error on iPad. But in my case it asks it on calling window.openDatabase. This call fails, because I already have another database which is causing go over the 5MB.

@andygup
Copy link
Author

andygup commented Mar 28, 2014

@DickvdBrink No, changing the default creates a workflow where the user will never see the add-more-storage popup. The reason for this is the only time the error shows up is immediately after you accept the message to allocate more storage space. So bypassing that popup for now seems like the best approach.

Note, using the modification I mentioned above, I was successfully able to open a database as large as 100MBs and write over 50MBs to it no problem, and the user will never see the popup.

As I wrote in my bug submission to Apple, this is obviously less-than-ideal coding practices. But, until we get more information (from Apple?) this appears to be one way to work around a potential show-stopper bug.

@brettz9
Copy link
Collaborator

brettz9 commented Oct 19, 2016

Thank you all for your support on this issue. I have added detection of Safari to set the default to the 25 * 1024 * 1025 suggested by @andygup in an earlier comment. I also allow one to import the CFG.js module (just now upper-cased from cfg.js to reflect the variable names in use internally in case you recently began importing it) and set to an alternate value. However, as mentioned we have already set up the default for you based on whether it is Safari or not, so you will probably not need to change this (and btw, if you were wondering, the lower amount isn't a problem for Node since node-websql doesn't use the size parameter).

Perhaps this need for more memory in Safari is related to the file size being larger in Safari as per issue #41 (an issue which is still open for any who might wish to tackle it).

Anyways, I'll close this issue as it should now be handled, but feel free to report back with any additional info. In particular, @andygup , if you can find a link to your bug report, that would be most appreciated. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants