ScottHamper / Cookies

JavaScript Client-Side Cookie Manipulation Library
The Unlicense
1.77k stars 170 forks source link

`_isValidDate` method failing in IE11 #59

Closed elidupuis closed 8 years ago

elidupuis commented 8 years ago

I'm getting the following error in IE 11:

 `expires` parameter cannot be converted to a valid Date instance

I've tracked the error down to the _isValidDate method evaluating to [object Array Iterator] instead of [object Date]. As far as I can tell this is happening when the testKey is removed in the _areEnabled method.

ScottHamper commented 8 years ago

Hey Eli,

I'm unable to reproduce this issue in IE11, and all unit tests successfully pass. If you include Cookies.js in an otherwise empty HTML page, does the error still occur? Can you provide me a simplified test case/code that reproduces the issue "in a vacuum"?

I have a feeling the problem is occurring due to some environmental difference, and not directly because of the combination of Cookies.js and IE11.

ScottHamper commented 8 years ago

Closing due to inactivity and inability to reproduce the issue.

miglesiasEB commented 4 years ago

Hi @ScottHamper,

We are getting this error at Eventbrite all over the place. We are using your project as our main cookie library, and this error is happening around 30K times per month at this moment.

We are loading version 1.2.3 and the issue is happening mainly in Windows machines (from Vista to Win10) and in Chrome (versions 51 and up). I have tried to reproduce it within a Codepen with no luck so far. I will keep on trying these days.

If you could give us any insights about why this could be happening we would appreciate it!

Thanks for your work in this library!

ScottHamper commented 4 years ago

Hey @miglesiasEB,

I think the most likely cause of this error is passing in a string value for the expires option when setting a cookie. The string value ultimately gets passed directly into a Date constructor, and the MDN documentation on the Date constructor states:

Note: Parsing of date strings with the Date constructor (and Date.parse(), which works the same way) is strongly discouraged due to browser differences and inconsistencies. Support for RFC 2822 format strings is by convention only. Support for ISO 8601 formats differs in that date-only strings (e.g. "1970-01-01") are treated as UTC, not local.

This could explain why errors are only seen on specific browsers - one browser may be successful in parsing a specific string format, while another may not.

Unfortunately, looking back on the library now, I see that Cookies.js itself relies on string parsing for its _maxExpireDate "constant" that's used when setting a cookie with an expires of Infinity. While it does use the format described in RFC 2822, it should be changed to use the Date constructor that takes multiple integers. However, I would be surprised if the _maxExpireDate constant is the cause of the exceptions you're seeing.

The only other possible cause for the error that is immediately apparent to me is if expires is a very large number. When expires is a number, it also eventually gets passed into a Date constructor (representing the number of milliseconds since Jan 1, 1970). If the number is too large, then the Date instance it creates will be invalid. For example:

new Date(999999999999999).getTime(); // 999999999999999
new Date(9999999999999999).getTime(); // NaN

Keep in mind that expires doesn't need to be as large as what's shown above before it causes issues, as it's multiplied by 1000 (to convert from seconds to milliseconds), and then added to the current time.

Hopefully this helps you track down the issue!

miglesiasEB commented 4 years ago

Hi @ScottHamper, thanks for your timely reply!

I have been trying different expires values within this Codepen.

Maybe the Codepen is not the best way of debbugging? Do you happen to have a page to do your manual testing for this library?

ScottHamper commented 4 years ago

Hey @miglesiasEB ,

There's an automated unit test suite that you can run by cloning the repo and opening /tests/spec-runner.html in a browser.

The best way to debug it would be to find a page on Eventbrite where the error gets triggered reliably and then use breakpoints in the browser dev tools to figure out what expires value is being used. Alternatively, you could temporarily tweak the Cookies._getExpiresDate method to include more information in the error message that it throws. Unfortunately, I originally wrote the function to mutate the expires parameter (lame), so the original value might be lost by the time an error gets thrown. We could change this so the original value stays intact and update the error message as follows:

Cookies._getExpiresDate = function (expires, now) {
    now = now || new Date();

    var expireDate = expires;

    if (typeof expires === 'number') {
        expireDate = expires === Infinity ?
            Cookies._maxExpireDate : new Date(now.getTime() + expires * 1000);
    } else if (typeof expires === 'string') {
        expireDate = new Date(expires);
    }

    if (expireDate && !Cookies._isValidDate(expireDate)) {
        throw new Error('`expires` parameter cannot be converted to a valid Date instance: ' + expires);
    }

    return expireDate;
};

Definitely not my favorite way to debug a problem, but if nothing else is working out for you, it's an option...

miglesiasEB commented 4 years ago

Thanks a lot @ScottHamper!

I will give it a try and get back to you with the results!

jdelStrother commented 4 years ago

For what it's worth, this crops up a lot in our javascript error reporter, from a variety of user agents. I added some extra debugging information - for us, it's failing straight away in the Cookies._areEnabled() call.

That passes -1 as expires into _getExpiresDate. The resulting expireDate is set to new Date(now.getTime() + expires * 1000), which appears to generate a valid-looking time, except that Object.prototype.toString.call(date) returns [object Object].

Which is clearly crazy. I suspect it's a dodgy extension somewhere, but haven't been able to narrow it down any further.

ScottHamper commented 4 years ago

Interesting. That certainly suggests there's something in the environment that's messing with the Date identifier. You can produce that kind of behavior by doing the following:

Date = function() { };
Object.prototype.toString.call(new Date());

I'm not familiar with extension development, but I could see that being a plausible explanation. It could also just be someone's date library latching onto and extending the default Date function. Isn't JavaScript wonderful?