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

Changing Firefox settings causes uncatchable exceptions #647

Closed
AshleyScirra opened this issue Jan 19, 2017 · 10 comments
Closed

Changing Firefox settings causes uncatchable exceptions #647

AshleyScirra opened this issue Jan 19, 2017 · 10 comments

Comments

@AshleyScirra
Copy link

AshleyScirra commented Jan 19, 2017

In Firefox, go to Options -> Privacy -> History and set Firefox to "Never remember history".

Now all localforage calls throw uncatchable exceptions. Demo URL: https://www.scirra.com/labs/bugs/localforage-crash/

This calls localforage.getItem() inside a try-catch block and it still throws an unhandled exception. Presumably there's an internal callback that ends up throwing an exception.

IMO this behavior of Firefox is really annoying, since it breaks pages and libraries that haven't been tested specifically with this particular Firefox setting. It also results in bug reports in our software (https://www.scirra.com/forum/localstorage-crash-with-firefox-no-history_t186255). It's not clear what we can do about this simply as a consumer of the library, because there's no way to catch the exception that is thrown. So I think libraries like localforage ought to handle this to hide these quirks from library users.

This is also precisely why I think there ought to be a memory fallback as proposed in PR #555.

@tofumatt
Copy link
Member

tofumatt commented Jan 19, 2017 via email

@thgreasi
Copy link
Member

It seems browser weird magic to me as well. I can confirm that I couldn't find a way to catch the error and reject properly. The error appears on FF private browsing, localforage fall backs to using localstorage and data are properly stored and retrieved.

@thgreasi
Copy link
Member

I could open a new PR with the memory storage driver currently available under the localforage organization.
BUT

  • as you can see on the provided example, localforage continues to work and properly stores and retrieved data from localstorage ... so I guess the error message logged isn't actually "breaking the page" (tested on Private Browsing + Never Remember History).
  • others have mentioned in other issues that are in favor of making localforage pluggable and as slim as possible

Waiting for your feedback.

@thgreasi
Copy link
Member

An other statement against embedign a memorystorage driver was that:
in order to be sure that the data are saved you will have to check both whether the promise actually resolved but also check the driver that is currently used.

@AshleyScirra
Copy link
Author

AshleyScirra commented Jan 20, 2017

Checking the driver, or waiting for a promise to resolve, doesn't actually tell you if storage was really permanently saved. In Chrome Incognito mode, all IndexedDB calls will succeed, even though it's effectively using memory storage. Even in normal browsing mode, there's no guarantee the browser won't suddenly decide to do a cleanup and trim its disk space usage, and delete all your site's saved content. (Or the user could do it.)

The only way to be sure that storage will persist is to call navigator.storage.persist(), which is not yet widely supported but is the only way to reliably find out the answer to the question "will my storage actually be kept?"

Also given the minified + compressed size of localforage is about 8kb, I'm not sure why anybody is worried about the size of the library. You could double it and nobody would notice. The kind of architecture it uses is another matter, but I don't think that should be the sole reason for not providing memory fallback.

@thgreasi
Copy link
Member

Adding a e.preventDefault(); call inside the indexedDB.open().onerror() handler seems to prevent the error from reaching window.onerror(). Sample codepen. Any chance this might have other side effects?

@thgreasi
Copy link
Member

@AshleyScirra thanks for your commitment on this so far.
I just opened PR #648 that possibly resolves this.
As soon as you can confirm that this works for you, we can merge it in for v1.5 .

@thgreasi
Copy link
Member

thgreasi commented Jan 23, 2017

Here is a fork of the original codepen using the code from PR 648.
Still works on Chrome (+ Private Browsing) and the Error no longer shows up on FF Privare Browsing. Also works in Edge Private and IE11 after replacing the ES6 parts.

@AshleyScirra
Copy link
Author

Tested PR #648 in Construct 2 in Firefox stable and nightly with the "never remember history" setting. It no longer throws the uncatchable error. Looks like it's sorted.

@thgreasi
Copy link
Member

Great!

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

No branches or pull requests

3 participants