Skip to content

Commit

Permalink
Don't crash if we can't use localStorage
Browse files Browse the repository at this point in the history
Our settings are not a fatal requirement, we can fall back on the
default values if they can't be accessed. A scenario where we've seen
this happen is when cookies are disabled in the browser. It seems
localStorage is disabled along with cookies in these settings.

So, lets log a message about the failure and otherwise silently
continue in this case.

Fixes issue #1577.
  • Loading branch information
samhed committed Jul 13, 2023
1 parent ca6527c commit a30f609
Show file tree
Hide file tree
Showing 2 changed files with 84 additions and 5 deletions.
74 changes: 69 additions & 5 deletions app/webutil.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,16 @@
* See README.md for usage and integration instructions.
*/

import { initLogging as mainInitLogging } from '../core/util/logging.js';
import * as Log from '../core/util/logging.js';

// init log level reading the logging HTTP param
export function initLogging(level) {
"use strict";
if (typeof level !== "undefined") {
mainInitLogging(level);
Log.initLogging(level);
} else {
const param = document.location.href.match(/logging=([A-Za-z0-9._-]*)/);
mainInitLogging(param || undefined);
Log.initLogging(param || undefined);
}
}

Expand Down Expand Up @@ -146,7 +146,7 @@ export function writeSetting(name, value) {
if (window.chrome && window.chrome.storage) {
window.chrome.storage.sync.set(settings);
} else {
localStorage.setItem(name, value);
localStorageSet(name, value);
}
}

Expand All @@ -156,7 +156,7 @@ export function readSetting(name, defaultValue) {
if ((name in settings) || (window.chrome && window.chrome.storage)) {
value = settings[name];
} else {
value = localStorage.getItem(name);
value = localStorageGet(name);
settings[name] = value;
}
if (typeof value === "undefined") {
Expand All @@ -181,6 +181,70 @@ export function eraseSetting(name) {
if (window.chrome && window.chrome.storage) {
window.chrome.storage.sync.remove(name);
} else {
localStorageRemove(name);
}
}

let loggedMsgs = [];
function logOnce(msg, level = "warn") {
if (!loggedMsgs.includes(msg)) {
switch (level) {
case "error":
Log.Error(msg);
break;
case "warn":
Log.Warn(msg);
break;
case "debug":
Log.Debug(msg);
break;
default:
Log.Info(msg);
}
loggedMsgs.push(msg);
}
}

let cookiesMsg = "Couldn't access noVNC settings, are cookies disabled?";

function localStorageGet(name) {
let r;
try {
r = localStorage.getItem(name);
} catch (e) {
if (e instanceof DOMException) {
logOnce(cookiesMsg);
logOnce("'localStorage.getItem(" + name + ")' failed: " + e,
"debug");
} else {
throw e;
}
}
return r;
}
function localStorageSet(name, value) {
try {
localStorage.setItem(name, value);
} catch (e) {
if (e instanceof DOMException) {
logOnce(cookiesMsg);
logOnce("'localStorage.setItem(" + name + "," + value +
")' failed: " + e, "debug");
} else {
throw e;
}
}
}
function localStorageRemove(name) {
try {
localStorage.removeItem(name);
} catch (e) {
if (e instanceof DOMException) {
logOnce(cookiesMsg);
logOnce("'localStorage.removeItem(" + name + ")' failed: " + e,
"debug");
} else {
throw e;
}
}
}
15 changes: 15 additions & 0 deletions tests/test.webutil.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,11 @@ describe('WebUtil', function () {
expect(window.localStorage.setItem).to.have.been.calledWithExactly('test', 'value');
expect(WebUtil.readSetting('test')).to.equal('value');
});

it('should not crash when local storage save fails', function () {
localStorage.setItem.throws(new DOMException());
expect(WebUtil.writeSetting('test', 'value')).to.not.throw;
});
});

describe('setSetting', function () {
Expand Down Expand Up @@ -137,6 +142,11 @@ describe('WebUtil', function () {
WebUtil.writeSetting('test', 'something else');
expect(WebUtil.readSetting('test')).to.equal('something else');
});

it('should not crash when local storage read fails', function () {
localStorage.getItem.throws(new DOMException());
expect(WebUtil.readSetting('test')).to.not.throw;
});
});

// this doesn't appear to be used anywhere
Expand All @@ -145,6 +155,11 @@ describe('WebUtil', function () {
WebUtil.eraseSetting('test');
expect(window.localStorage.removeItem).to.have.been.calledWithExactly('test');
});

it('should not crash when local storage remove fails', function () {
localStorage.removeItem.throws(new DOMException());
expect(WebUtil.eraseSetting('test')).to.not.throw;
});
});
});

Expand Down

0 comments on commit a30f609

Please sign in to comment.