Skip to content

Commit 984acdc

Browse files
IgorMinarIgorMinar
committed
Reworked the cookie synchronization between cookie service, $browser and document.cookie.
Now we finally correctly handle situations when browser refuses to set a cookie, due to storage quota or other (file:// protocol) limitations.
1 parent 3eec8c1 commit 984acdc

5 files changed

Lines changed: 101 additions & 35 deletions

File tree

‎src/Browser.js‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -138,11 +138,10 @@ function Browser(location, document, head, XHR, $log) {
138138

139139
if (name) {
140140
if (value === _undefined) {
141-
delete lastCookies[name];
142141
rawDocument.cookie = escape(name) + "=;expires=Thu, 01 Jan 1970 00:00:00 GMT";
143142
} else {
144143
if (isString(value)) {
145-
rawDocument.cookie = escape(name) + '=' + escape(lastCookies[name] = value);
144+
rawDocument.cookie = escape(name) + '=' + escape(value);
146145

147146
cookieLength = name.length + value.length + 1;
148147
if (cookieLength > 4096) {

‎src/services.js‎

Lines changed: 42 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -405,52 +405,71 @@ angularService('$resource', function($xhr){
405405
* cookies are created or deleted from the browser at the end of the current eval.
406406
*/
407407
angularService('$cookies', function($browser) {
408-
var cookies = {},
409-
rootScope = this,
410-
lastCookies;
408+
var rootScope = this,
409+
cookies = {},
410+
lastCookies = {},
411+
lastBrowserCookies;
411412

412413
//creates a poller fn that copies all cookies from the $browser to service & inits the service
413414
$browser.addPollFn(function() {
414415
var currentCookies = $browser.cookies();
415-
if (lastCookies != currentCookies) {
416-
lastCookies = currentCookies;
416+
if (lastBrowserCookies != currentCookies) { //relies on browser.cookies() impl
417+
lastBrowserCookies = currentCookies;
418+
copy(currentCookies, lastCookies);
417419
copy(currentCookies, cookies);
418420
rootScope.$eval();
419421
}
420422
})();
421423

422424
//at the end of each eval, push cookies
423-
this.$onEval(PRIORITY_LAST, update);
425+
this.$onEval(PRIORITY_LAST, push);
424426

425427
return cookies;
426428

427-
function update(){
429+
430+
/**
431+
* Pushes all the cookies from the service to the browser and verifies if all cookies were stored.
432+
*/
433+
function push(){
428434
var name,
429-
browserCookies = $browser.cookies();
435+
browserCookies,
436+
updated;
437+
438+
//delete any cookies deleted in $cookies
439+
for (name in lastCookies) {
440+
if (isUndefined(cookies[name])) {
441+
$browser.cookies(name, _undefined);
442+
}
443+
}
430444

431-
//$cookies -> $browser
445+
//update all cookies updated in $cookies
432446
for(name in cookies) {
433-
if (cookies[name] !== browserCookies[name]) {
447+
if (cookies[name] !== lastCookies[name]) {
434448
$browser.cookies(name, cookies[name]);
449+
updated = true;
435450
}
436451
}
437452

438-
//get what was actually stored in the browser
439-
browserCookies = $browser.cookies();
453+
//verify what was actually stored
454+
if (updated){
455+
updated = !updated;
456+
browserCookies = $browser.cookies();
457+
458+
for (name in cookies) {
459+
if (cookies[name] !== browserCookies[name]) {
460+
//delete or reset all cookies that the browser dropped from $cookies
461+
if (isUndefined(browserCookies[name])) {
462+
delete cookies[name];
463+
} else {
464+
cookies[name] = browserCookies[name];
465+
}
466+
updated = true;
467+
}
440468

441-
//$browser -> $cookies
442-
for(name in browserCookies) {
443-
if (isUndefined(cookies[name])) {
444-
$browser.cookies(name, _undefined);
445-
} else {
446-
cookies[name] = browserCookies[name];
447469
}
448-
}
449470

450-
//drop cookies in $cookies for cookies that $browser or real browser dropped
451-
for (name in cookies) {
452-
if (isUndefined(browserCookies[name])) {
453-
delete cookies[name];
471+
if (updated) {
472+
rootScope.$eval();
454473
}
455474
}
456475
}

‎test/BrowserSpecs.js‎

Lines changed: 26 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -152,27 +152,39 @@ describe('browser', function(){
152152
});
153153

154154
it('should log warnings when 4kb per cookie storage limit is reached', function() {
155-
var i, longVal = '', cookieString;
155+
var i, longVal = '', cookieStr;
156156

157157
for(i=0; i<4092; i++) {
158158
longVal += '+';
159159
}
160160

161-
cookieString = document.cookie;
161+
cookieStr = document.cookie;
162162
browser.cookies('x', longVal); //total size 4094-4096, so it should go through
163-
expect(document.cookie).not.toEqual(cookieString);
163+
expect(document.cookie).not.toEqual(cookieStr);
164164
expect(browser.cookies()['x']).toEqual(longVal);
165165
expect(logs.warn).toEqual([]);
166166

167-
browser.cookies('x', longVal + 'xxx') //total size 4097-4099, a warning should be logged
168-
//browser behavior is undefined, so we test for existance of warning logs only
167+
browser.cookies('x', longVal + 'xxx'); //total size 4097-4099, a warning should be logged
169168
expect(logs.warn).toEqual(
170169
[[ "Cookie 'x' possibly not set or overflowed because it was too large (4097 > 4096 " +
171170
"bytes)!" ]]);
171+
172+
//force browser to dropped a cookie and make sure that the cache is not out of sync
173+
browser.cookies('x', 'shortVal');
174+
expect(browser.cookies().x).toEqual('shortVal'); //needed to prime the cache
175+
cookieStr = document.cookie;
176+
browser.cookies('x', longVal + longVal + longVal); //should be too long for all browsers
177+
178+
if (document.cookie !== cookieStr) {
179+
fail("browser didn't drop long cookie when it was expected. make the cookie in this " +
180+
"test longer");
181+
}
182+
183+
expect(browser.cookies().x).toEqual('shortVal');
172184
});
173185

174186
it('should log warnings when 20 cookies per domain storage limit is reached', function() {
175-
var i, str;
187+
var i, str, cookieStr;
176188

177189
for (i=0; i<20; i++) {
178190
str = '' + i;
@@ -185,12 +197,18 @@ describe('browser', function(){
185197
}
186198
expect(i).toEqual(20);
187199
expect(logs.warn).toEqual([]);
200+
cookieStr = document.cookie;
188201

189202
browser.cookies('one', 'more');
190-
//browser behavior is undefined, so we test for existance of warning logs only
191203
expect(logs.warn).toEqual([]);
192-
});
193204

205+
//if browser dropped a cookie (very likely), make sure that the cache is not out of sync
206+
if (document.cookie === cookieStr) {
207+
expect(size(browser.cookies())).toEqual(20);
208+
} else {
209+
expect(size(browser.cookies())).toEqual(21);
210+
}
211+
});
194212
});
195213

196214

‎test/angular-mocks.js‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@ function MockBrowser() {
7575
};
7676

7777
self.cookieHash = {};
78+
self.lastCookieHash = {};
7879
}
7980
MockBrowser.prototype = {
8081

@@ -103,12 +104,17 @@ MockBrowser.prototype = {
103104
if (value == undefined) {
104105
delete this.cookieHash[name];
105106
} else {
106-
if (isString(value)) {
107+
if (isString(value) && //strings only
108+
value.length <= 4096) { //strict cookie storage limits
107109
this.cookieHash[name] = value;
108110
}
109111
}
110112
} else {
111-
return copy(this.cookieHash);
113+
if (!equals(this.cookieHash, this.lastCookieHash)) {
114+
this.lastCookieHash = copy(this.cookieHash);
115+
this.cookieHash = copy(this.cookieHash);
116+
}
117+
return this.cookieHash;
112118
}
113119
}
114120

‎test/servicesSpec.js‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -438,6 +438,7 @@ describe("service", function(){
438438
it('should remove a cookie when a $cookies property is deleted', function() {
439439
scope.$cookies.oatmealCookie = 'nom nom';
440440
scope.$eval();
441+
scope.$browser.poll();
441442
expect(scope.$browser.cookies()).
442443
toEqual({'preexisting': 'oldCookie', 'oatmealCookie':'nom nom'});
443444

@@ -446,6 +447,29 @@ describe("service", function(){
446447

447448
expect(scope.$browser.cookies()).toEqual({'preexisting': 'oldCookie'});
448449
});
450+
451+
452+
it('should drop or reset cookies that browser refused to store', function() {
453+
var i, longVal;
454+
455+
for (i=0; i<5000; i++) {
456+
longVal += '*';
457+
}
458+
459+
//drop if no previous value
460+
scope.$cookies.longCookie = longVal;
461+
scope.$eval();
462+
expect(scope.$cookies).toEqual({'preexisting': 'oldCookie'});
463+
464+
465+
//reset if previous value existed
466+
scope.$cookies.longCookie = 'shortVal';
467+
scope.$eval();
468+
expect(scope.$cookies).toEqual({'preexisting': 'oldCookie', 'longCookie': 'shortVal'});
469+
scope.$cookies.longCookie = longVal;
470+
scope.$eval();
471+
expect(scope.$cookies).toEqual({'preexisting': 'oldCookie', 'longCookie': 'shortVal'});
472+
});
449473
});
450474

451475

0 commit comments

Comments
 (0)