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

localStorage does not work in some configurations of IE #1291

Closed
wants to merge 132 commits into
base: master
from

Conversation

Projects
None yet
@mizzao
Contributor

mizzao commented Jun 2, 2014

Hi guys,

I recently started noticing that IE was not working at all on the devel branch. I thought it would get fixed, but the problem has persisted for a while. The errors are different on local development and meteor hosting, but both result in a blank page.

For example, on http://crowdmapper.meteor.com which is on 0.6.5-rc12:

SCRIPT5: Access is denied.

91404edf5fdd1cb531c72e6ca922a8d703eafd3b.js, line 10 character 3646
SEC7118: XMLHttpRequest for https://ddp--2731-crowdmapper.meteor.com/sockjs/info?cb=ftn6uad6w3&t=1375913299050 required Cross Origin Resource Sharing (CORS). 
crowdmapper.meteor.com

On local hosting:

SCRIPT5: Access is denied.

localstorage.js, line 35 character 1
SCRIPT5007: Unable to get property 'getItem' of undefined or null reference 
accounts-base.js, line 470 character 3
SCRIPT5007: Unable to get property 'Accounts' of undefined or null reference 
accounts-ui-unstyled.js, line 24 character 1
SCRIPT5007: Unable to get property 'Accounts' of undefined or null reference 
accounts-ui.js, line 22 character 1
SCRIPT5007: Unable to get property 'Accounts' of undefined or null reference 
user-status.js, line 22 character 1
SCRIPT5007: Unable to get property 'Accounts' of undefined or null reference 
accounts-testing.js, line 22 character 1
SCRIPT5007: Unable to get property 'Accounts' of undefined or null reference 
waiting-lobby.js, line 25 character 1
SCRIPT5007: Unable to get property 'Accounts' of undefined or null reference 
turkserver.js, line 25 character 1
SCRIPT5007: Unable to get property 'Accounts' of undefined or null reference 
... a billion more errors

It seems like this is caused by the following line of (https://github.com/meteor/meteor/blob/devel/packages/localstorage/localstorage.js : 3) of which kills the rest of the app:

if (window.localStorage) { 
  Meteor._localStorage = {
    ...
  }
}

According to this issue the error will go away if running as admin or disabling protected mode.

The machine I'm using has IE10 on Windows 8.

My app indeed worked when I unchecked the following. However, it is unreasonable to expect users to do this and I wonder if there is a way to make Meteor fail more gracefully (such as reverting to the in-memory storage.) Maybe try wrapping the window.localStorage call in a try block?

image

Other references (seems like the default IE security config is just a big fail, sorry):

@n1mmy

This comment has been minimized.

Member

n1mmy commented Aug 8, 2013

Hey @mizzao, thanks for the report. Yeah, it looks like IE localstorage security is kinda a mess. I think it's possible all our testing environments (VMs, browserstack) have it running as a privileged user =(

Looking at the code, I don't see much difference between devel and 0.6.4. Can you check to see if you see the same error in the released meteor, so we know if this is a regression on devel?

I will try to replicate this in my VM.

@n1mmy

This comment has been minimized.

Member

n1mmy commented Aug 8, 2013

I could not replicate this in my IE10/Win7 VM. I tried both a stock parties example and crowdmapper.meteor.com. In both cases I was able to login, close the browser and restart it, and see I was logged in as the same user as before.

I made a non-admin user and checked the Internet Options had protected mode enabled.

Maybe this is Win8 only? I'll see about getting a Win8 VM.

@n1mmy

This comment has been minimized.

Member

n1mmy commented Aug 8, 2013

Tested on IE10/Win8 on Browserstack. It worked fine, but protected mode was not enabled, and I couldn't enable it (requires system administrator)

@mizzao

This comment has been minimized.

Contributor

mizzao commented Aug 8, 2013

Hi @n1mmy,

I see this error with the default Meteor project on 0.6.4.1 as well, although I had to include the localstorage package manually due to the absence of anything requiring it. However, the page loads as normal because nothing cares about the script crashing.

image

Parties on 0.6.4.1 crashes similarly:

image

Creating an account or logging in is impossible:

SCRIPT5: Access is denied.

localstorage.js, line 1 character 14

SCRIPT5007: Unable to get property 'getItem' of undefined or null reference 
localstorage_token.js, line 57 character 3

 Exception while delivering result of invoking 'createUser'TypeError: Unable to get property 'setItem' of undefined or null referenceTypeError: Unable to get property 'setItem' of undefined or null reference
   at _storeLoginToken (http://192.168.56.101:3000/packages/accounts-base/localstorage_token.js?a92ca700ad45e39e766f8b6ea611c73be490495a:39:3)
   at _makeClientLoggedIn (http://192.168.56.101:3000/packages/accounts-base/accounts_client.js?0d7d17fbdbd2ea470eccef67b144c23646ee853c:161:3)
   at loggedInAndDataReadyCallback (http://192.168.56.101:3000/packages/accounts-base/accounts_client.js?0d7d17fbdbd2ea470eccef67b144c23646ee853c:141:5)
   at Anonymous function (http://192.168.56.101:3000/packages/meteor/dynamics_browser.js?1e9d974c929e38d6d8ea8263bd877ccd5899f5e8:40:7)
   at _maybeInvokeCallback (http://192.168.56.101:3000/packages/livedata/livedata_connection.js?5d09753571656c685bb10c7970eebfbf23d35ef8:349:7)
   at receiveResult (http://192.168.56.101:3000/packages/livedata/livedata_connection.js?5d09753571656c685bb10c7970eebfbf23d35ef8:369:5)
   at _livedata_result (http://192.168.56.101:3000/packages/livedata/livedata_connection.js?5d09753571656c685bb10c7970eebfbf23d35ef8:1232:7)
   at onMessage (http://192.168.56.101:3000/packages/livedata/livedata_connection.js?5d09753571656c685bb10c7970eebfbf23d35ef8:212:7)
   at Anonymous function (http://192.168.56.101:3000/packages/livedata/stream_client_sockjs.js?6ef3fd3e8e39c1c357993f6936512726288ad127:179:11)
   at forEach (http://192.168.56.101:3000/packages/underscore/underscore.js?ed2d2b960c0e746b3e4f9282d5de66ef7b1a2b4d:78:7) 

 Exception while delivering result of invoking 'login'TypeError: Unable to get property 'setItem' of undefined or null referenceTypeError: Unable to get property 'setItem' of undefined or null reference
   at _storeLoginToken (http://192.168.56.101:3000/packages/accounts-base/localstorage_token.js?a92ca700ad45e39e766f8b6ea611c73be490495a:39:3)
   at _makeClientLoggedIn (http://192.168.56.101:3000/packages/accounts-base/accounts_client.js?0d7d17fbdbd2ea470eccef67b144c23646ee853c:161:3)
   at loggedInAndDataReadyCallback (http://192.168.56.101:3000/packages/accounts-base/accounts_client.js?0d7d17fbdbd2ea470eccef67b144c23646ee853c:141:5)
   at Anonymous function (http://192.168.56.101:3000/packages/meteor/dynamics_browser.js?1e9d974c929e38d6d8ea8263bd877ccd5899f5e8:40:7)
   at _maybeInvokeCallback (http://192.168.56.101:3000/packages/livedata/livedata_connection.js?5d09753571656c685bb10c7970eebfbf23d35ef8:349:7)
   at receiveResult (http://192.168.56.101:3000/packages/livedata/livedata_connection.js?5d09753571656c685bb10c7970eebfbf23d35ef8:369:5)
   at _livedata_result (http://192.168.56.101:3000/packages/livedata/livedata_connection.js?5d09753571656c685bb10c7970eebfbf23d35ef8:1232:7)
   at onMessage (http://192.168.56.101:3000/packages/livedata/livedata_connection.js?5d09753571656c685bb10c7970eebfbf23d35ef8:212:7)
   at Anonymous function (http://192.168.56.101:3000/packages/livedata/stream_client_sockjs.js?6ef3fd3e8e39c1c357993f6936512726288ad127:179:11)
   at forEach (http://192.168.56.101:3000/packages/underscore/underscore.js?ed2d2b960c0e746b3e4f9282d5de66ef7b1a2b4d:78:7) 

This is clearly a Win8/IE10 problem when protected mode is enabled by default due to system or network administrator settings.

@raix

This comment has been minimized.

Contributor

raix commented Aug 8, 2013

I think this might allways been an issue, we have to test the actual storage - think this applies to ff etc. The localstorage is there but we are not allowed to use it.

I've made a solution but didn't make it a pr. since theres allready another working on it - so i've put it as inspiration.

have a look at: comment issue 1221

@raix

This comment has been minimized.

Contributor

raix commented Aug 8, 2013

@mizzao

This comment has been minimized.

Contributor

mizzao commented Aug 8, 2013

You got rid of the $.browser.msie part too. Nice! Core needs to work that in.

As an aside, I'd really like it if we had the option to start the Meteor client with local storage turned off. Sometimes it's nice to be able to log in as different users from different tabs, for various reasons including testing.

@raix

This comment has been minimized.

Contributor

raix commented Aug 8, 2013

for now would this work for your test usage?:

window.addEventListener('storage', function(e) {
  if (e.key === 'Meteor.loginToken' && e.newValue !== '') {
    localStorage.setItem(e.key, '');
    localStorage.setItem('Meteor.userId', '');
  }
}, false);
@mizzao

This comment has been minimized.

Contributor

mizzao commented Aug 8, 2013

@raix isn't that going to log out the existing user as well? I think some code for allowing multiple logins would need to just force Meteor to use the shimmed storage instead of local storage. I would probably just do monkey patching...

@raix

This comment has been minimized.

Contributor

raix commented Aug 8, 2013

@mizzao true, I'm thinking that it could be nice to be able to set options for packages in general. Could be pr. package/global at bundle/runtime.

@mizzao

This comment has been minimized.

Contributor

mizzao commented Aug 8, 2013

@raix thinking the same thing. That's why I just wrote #1292. Maybe you could chime in on that thread if you support it.

@raix

This comment has been minimized.

Contributor

raix commented Aug 8, 2013

@mizzao sounds great! 👍

@n1mmy

This comment has been minimized.

Member

n1mmy commented Aug 20, 2013

Just to check in: this is still on my plate. I haven't managed to get a Win8 VM with a non-admin user yet. The free VMs from microsoft (https://github.com/xdissent/ievms) don't seem to let me create a second user. I'll see if I can scrounge up a physical machine with Win8, or find a different VM solution.

@n1mmy

This comment has been minimized.

Member

n1mmy commented Aug 20, 2013

@mizzao I tried this on a physical windows 8 machine, and was still unable to reproduce it.

I made sure I was logged in as a non-admin user and "enable protected mode" was checked. This is a pretty fresh install, so just about all the settings should be at their default values.

Any idea what could be different on my setup that means I don't see the issue. Or ideas on what to change to be able to reproduce it?

@n1mmy

This comment has been minimized.

Member

n1mmy commented Aug 22, 2013

Ok, I made a fresh install of Windows 8 on a machine. Stock install, took windows updates, made a new non-admin user, and confirmed that "enable protected mode" was checked (it was). Everything worked fine, did not see the error. Tested in both desktop and tiled mode (as suggested in one of the threads posted). Any other ideas on how to trigger this? Is it possible microsoft fixed it in an update?

Looking at the code, I don't think there has been any relevant change since auth was launched. That is, if the failure is accessing local storage to test if it exists throws, the same error would happen in every release since 0.5.0. Judging from the text in the first link you provided (http://community.spiceworks.com/topic/357825-ie10-script5-access-is-denied), it sounds like this issue affects lots of sites, not just Meteor. Still, there is probably something clever we can do like try/catch or typeof window.localStorage if we can figure out what IE is actually doing.

@mizzao

This comment has been minimized.

Contributor

mizzao commented Aug 22, 2013

Hi @n1mmy,

I was observing this on a corporate laptop that was set up for me already by the IT department. Unfortunately, I returned that laptop at the end of my internship and I don't see this on Windows 7's IE10 on my personal laptop.

I'm in China right now where it's very hard to connect to a lot of U.S. sites. Please feel free to leave this issue open or closed as you see fit. If other users have this problem as well, I'm sure we'll see them chime in.

@n1mmy

This comment has been minimized.

Member

n1mmy commented Aug 22, 2013

@mizzao Cool beans. I'll leave this open for now in case someone else has a similar problem and can help narrow it down. Have fun in China =)

@raix

This comment has been minimized.

Contributor

raix commented Aug 23, 2013

@n1mmy would you take a pr on this, maybe to a test branch for verifying/test the code? (I could do some test to make sure that theres no issues, but only Got an ie 7 or 8 and ie9, otherwise mac)

@raix

This comment has been minimized.

Contributor

raix commented Aug 23, 2013

Btw. Has to be try catch, if the localstorage is disabled, the localstorage object would still exist and the data would on some browsers just go into the air, so we have to do a "physical" test to make sure the data is stored

@n1mmy

This comment has been minimized.

Member

n1mmy commented Aug 23, 2013

@raix I'm reluctant to do anything on this until we can figure out how to reproduce it. We could do something with try/catch that might fix the problem, but if we don't have a way to test that it actually makes a difference we won't know if we've really fixed the problem or just added unnecessary complexity.

@raix

This comment has been minimized.

Contributor

raix commented Aug 24, 2013

I'll see if i can reproduce the error - it's not an ie only issue...

@raix

This comment has been minimized.

Contributor

raix commented Dec 13, 2013

@n1mmy seems @piffie also made a pr on this with try/catch trying to solve: #1590 #1531 #1512 and this issue?

@jvanveen

This comment has been minimized.

jvanveen commented Dec 16, 2013

Thanks for the heads up. Looks like you got a clean fix for that already. Is that in Meteor core yet?

@glasser glasser closed this in 3f12f77 Dec 20, 2013

@mizzao

This comment has been minimized.

Contributor

mizzao commented Jun 2, 2014

Hi guys,

This is still broken in IE11 with certain security policies turned on. See the following:

image

The problem arises right at https://github.com/meteor/meteor/blob/devel/packages/localstorage/localstorage.js#L3. Just trying to grab window.localStorage throws the error, which causes the rest of the app not to load. It seems like that would need to be encased in a try block in order to work on IE with these restrictions on.

I have access to a corporate computer that can definitively reproduce this, for the summer, so I'd be happy to help find something that works. I'll be in SF for the week of June 8th through June 14th, planning to drop by June 9th, and would be happy to hack further in person.

@raix

This comment has been minimized.

Contributor

raix commented Jun 2, 2014

@mizzao agree it should be tested via try/catch - btw. It would be wise to make the local storage api async this imply changes in accounts etc.

estark37 and others added some commits Jun 2, 2014

Notice changes to npm-shrinkwrap.json
There were two separate issues here:

 - npm-shrinkwrap.json wasn't actually being used as part of the
   watchset/buildinfo, so changes to it might not cause the package to
   be considered for rebuilding

 - meteor-npm only compared top-level changes when deciding whether to
   update, not all changes

Fixes #1648
update sockjs to 0.3.9, websocket-driver to 0.3.4
The websocket-driver update includes our PR to relinquish some memory
faster. faye/websocket-driver-node#6

The sockjs update mostly consists of aligning the version of
faye-websocket it uses internally with the version that we use for our
client. It also contains this Vary: Origin change:
 sockjs/sockjs-node#130
Emily Stark
Remove spacebars-compiler -> spacebars test dependency.
This fixes a circular build-time dependency when building test slices.
@glasser

This comment has been minimized.

Member

glasser commented Jun 2, 2014

@mizzao can you send a PR for this which is tested in your corporate context?

@mizzao

This comment has been minimized.

Contributor

mizzao commented on 65b76b2 Jun 2, 2014

Nice.

Use a try/catch block to check for localStorage - fixes #1291, fixes #…
…1688.

Accessing `window.localStorage` can immediately throw an exception, which needs to be caught.
For more information, see http://www.w3.org/TR/webstorage/#dom-localstorage.
@mizzao

This comment has been minimized.

Contributor

mizzao commented Jun 2, 2014

I certainly can: #2204.

Please ignore the files attached to this issue. I tried using the GitHub issue to PR conversion tool and it created a PR against master.

mizzao added a commit to scienceathome/meteor that referenced this pull request Jun 2, 2014

Check for localStorage, catching exceptions - fixes meteor#1291, fixes
…meteor#1688.

Accessing `window.localStorage` can immediately throw an exception, which needs to be caught.
For more information, see http://www.w3.org/TR/webstorage/#dom-localstorage.

Slava added a commit that referenced this pull request Jun 9, 2014

Check for localStorage, catching exceptions - fixes #1291, fixes #1688.
Accessing `window.localStorage` can immediately throw an exception, which needs to be caught.
For more information, see http://www.w3.org/TR/webstorage/#dom-localstorage.
@yuwiggin

This comment has been minimized.

yuwiggin commented on 875274d Dec 5, 2014

I integrated email verification in the sign up process. When the verification url was opened in browser, an exception occurred in the server-side as following:

I20141205-00:13:18.274(8)? Exception from sub moRyCX49pYKjcivYH Error: Did not check() all arguments during publisher 'filteredUsers'
I20141205-00:13:18.274(8)?     at _.extend.throwUnlessAllArgumentsHaveBeenChecked (packages/check/match.js:352)
I20141205-00:13:18.275(8)?     at Object.Match._failIfArgumentsAreNotAllChecked (packages/check/match.js:108)
I20141205-00:13:18.275(8)?     at maybeAuditArgumentChecks (packages/ddp/livedata_server.js:1596)
I20141205-00:13:18.275(8)?     at _.extend._runHandler (packages/ddp/livedata_server.js:943)
I20141205-00:13:18.276(8)?     at _.extend._startSubscription (packages/ddp/livedata_server.js:769)
I20141205-00:13:18.276(8)?     at _.extend.protocol_handlers.sub (packages/ddp/livedata_server.js:582)
I20141205-00:13:18.277(8)?     at packages/ddp/livedata_server.js:546

The following packages was added in my app.

accounts-password                  1.0.4 
alanning:roles                     1.2.13
audit-argument-checks              1.0.1 
email                              1.0.4 
ian:accounts-ui-bootstrap-3        1.1.26
iron:router                        1.0.3 
meteor-platform                    1.2.0 
mizzao:bootstrap-3                 3.3.1 
mrt:accounts-admin-ui-bootstrap-3  0.2.7 
sacha:spin                         2.0.4 
underscore                         1.0.1 

Is it an issue? how to fix it? Thank you for your advice.

This comment has been minimized.

Member

glasser replied Dec 9, 2014

This means that the Meteor.publish('filteredUsers') call does not check all its arguments and is thus incompatible with the audit-argument-checks package. You should change it to check all its arguments!

jamiter referenced this pull request May 17, 2017

Polyfill Meteor._localStorage on the server too.
Although Meteor._localStorage has historically been used only in browsers,
it can be helpful on the server for writing isomorphic code.

jamiter added a commit to jamiter/meteor that referenced this pull request May 17, 2017

Move localStorage access back into try statement
Accessing window.localStorage can immediately throw an error in IE (meteor#1291) and other/older webkit versions.

benjamn added a commit that referenced this pull request May 17, 2017

Move localStorage access back into try statement (#8703)
Accessing window.localStorage can immediately throw an error in IE (#1291) and other/older webkit versions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment