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

Fix IndexedDB check in Firefox with disabled cookies #1831

Merged
merged 1 commit into from Jan 13, 2016

Conversation

Projects
None yet
4 participants
@sheff146
Copy link

sheff146 commented Jan 13, 2016

This PR is a fix for issue #1825. According to it FF throws a Security Error during IndexedDB check if cookies are disabled.
Simply wrapped these checks in try/catch.

@patrickkettner

This comment has been minimized.

Copy link
Member

patrickkettner commented Jan 13, 2016

lgtm! Could you squash down to a single commit? Thanks!

@ma-jahn

This comment has been minimized.

Copy link

ma-jahn commented Jan 13, 2016

Hi,

I am afraid this PR will not fix the issue.
If I'm not mistaken, Modernizr already uses try catch for local and session storage access. Security Errors will not be caught by a simple try catch otherwise we wouldn't experience any issues with local and session storage.

@patrickkettner

This comment has been minimized.

Copy link
Member

patrickkettner commented Jan 13, 2016

@KK-MarcelJahn did you try the fix? Because I am finding the opposite of your claims

image

@sheff146

This comment has been minimized.

Copy link
Author

sheff146 commented Jan 13, 2016

@KK-MarcelJahn there are no security errors on localStorage and sessionStorage checks in Modernizr v3 unlike v2. The only error is being thrown on IndexedDB check. You can build sources and try.

@sheff146 sheff146 force-pushed the sheff146:firefox-disabled-cookies-fix branch from 4f9c8de to a1fd83a Jan 13, 2016

@sheff146

This comment has been minimized.

Copy link
Author

sheff146 commented Jan 13, 2016

@patrickkettner squashed.

try {
indexeddb = prefixed('indexedDB', window);
} catch (e) {
indexeddb = null;

This comment has been minimized.

@patrickkettner

patrickkettner Jan 13, 2016

Member

sorry for not catching before, but this line is unnecessary. can you remove it in both files and squash one more time?

This comment has been minimized.

@sheff146

sheff146 Jan 13, 2016

Author

Yep. Done.

a.shevchenko
Remove unnecessary assignment
Squashed commits:
[a1fd83a] Fix indexedDB blob check
In Firefox it was throwing Security Error when cookies was disabled
[fea8cb6] Fix indexedDB check
In Firefox it was throwing Security Error when cookies was disabled

@sheff146 sheff146 force-pushed the sheff146:firefox-disabled-cookies-fix branch from a1fd83a to 715d60b Jan 13, 2016

patrickkettner added a commit that referenced this pull request Jan 13, 2016

Merge pull request #1831 from sheff146/firefox-disabled-cookies-fix
Fix IndexedDB check in Firefox with disabled cookies

@patrickkettner patrickkettner merged commit 7152423 into Modernizr:master Jan 13, 2016

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@patrickkettner

This comment has been minimized.

Copy link
Member

patrickkettner commented Jan 13, 2016

cheers - thanks mate

try {
indexeddb = prefixed('indexedDB', window);
} catch (e) {
}

This comment has been minimized.

@ryanseddon

ryanseddon Jan 13, 2016

Member

Put the closing } on the same line

This comment has been minimized.

@ryanseddon

ryanseddon Jan 13, 2016

Member

Actually wouldn't we want to return false here since it won't work and to me that's the same as not supported.

This comment has been minimized.

@patrickkettner

patrickkettner Jan 13, 2016

Member

we could return, but we aren't inside of an addTest, so I don't think return false would be doing what you think it would be doing

This comment has been minimized.

@sheff146

sheff146 Jan 13, 2016

Author

@ryanseddon how to do it technically now when PR is closed? Create new PR?

try {
indexeddb = prefixed('indexedDB', window);
} catch (e) {
}

This comment has been minimized.

@ryanseddon

ryanseddon Jan 13, 2016

Member

Same here return false.

@ma-jahn

This comment has been minimized.

Copy link

ma-jahn commented Jan 13, 2016

Yeah sorry for the (my) confusion!
This fix is working!

Guess i messed up with the versions i used for testing (in prod we are using 2.8).
So we'll update as soon as this fix is merged and a new version is available. Thank you guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.