Protect all uses of JSON.parse against exceptions.

Protect all uses of JSON.parse against exceptions.

BUG=None
TEST=Manual

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=134407

[Jamie, 2012-04-27 20:30:29]

ptal

[Sergey Ulanov, 2012-04-27 21:10:18]

In most of these cases failure to parse JSON would indicate a bug in our code. I
think we should at least log something when we encounter such problems instead
of just ignoring them.

[Jamie, 2012-04-27 21:36:46]

Good point. I've added console.error calls for all failures that didn't already
have them.

[Wez, 2012-04-28 00:00:42]

https://chromiumcodereview.appspot.com/10221021/diff/3001/remoting/webapp/cli...
File remoting/webapp/client_plugin_async.js (right):

https://chromiumcodereview.appspot.com/10221021/diff/3001/remoting/webapp/cli...
remoting/webapp/client_plugin_async.js:90:
remoting.ClientPluginAsync.prototype.handleMessage_ = function(message_str) {
cleanup: message_str -> messageStr

https://chromiumcodereview.appspot.com/10221021/diff/3001/remoting/webapp/hos...
File remoting/webapp/host_list.js (right):

https://chromiumcodereview.appspot.com/10221021/diff/3001/remoting/webapp/hos...
remoting/webapp/host_list.js:55: var cached_parsed = jsonParseSafe(cached);
nit: cached -> cachedStr, cached_parsed -> cached? Otherwise, cached_parsed ->
cachedParsed.

https://chromiumcodereview.appspot.com/10221021/diff/3001/remoting/webapp/hos...
remoting/webapp/host_list.js:125: var parsed_response =
nit: parsed_response -> parsedResponse, or just response.

https://chromiumcodereview.appspot.com/10221021/diff/3001/remoting/webapp/hos...
remoting/webapp/host_list.js:145: console.error('Invalid "hosts" response from
server.');
nit: Suggest "Invalid host list from server."

https://chromiumcodereview.appspot.com/10221021/diff/3001/remoting/webapp/oau...
File remoting/webapp/oauth2.js (right):

https://chromiumcodereview.appspot.com/10221021/diff/3001/remoting/webapp/oau...
remoting/webapp/oauth2.js:225: console.error('Invalid "token" response from
server.');
nit: Lose the quotes around token.

https://chromiumcodereview.appspot.com/10221021/diff/3001/remoting/webapp/rem...
File remoting/webapp/remoting.js (right):

https://chromiumcodereview.appspot.com/10221021/diff/3001/remoting/webapp/rem...
remoting/webapp/remoting.js:130: jsonParseSafe(xhr.responseText);
I'd suggest leaving this one JSON.parse(), so that the web-app will fail to work
if we manage to break it in such a fundamental way.

https://chromiumcodereview.appspot.com/10221021/diff/3001/remoting/webapp/rem...
remoting/webapp/remoting.js:288: function jsonParseSafe(jsonString) {
nit: parseJsonOrNull?

https://chromiumcodereview.appspot.com/10221021/diff/3001/remoting/webapp/rem...
remoting/webapp/remoting.js:292: return undefined;
Is the aim in returning undefined that if the caller uses the value without
testing then we'll see errors about using undefined values?

[Jamie, 2012-04-28 00:15:41]

https://chromiumcodereview.appspot.com/10221021/diff/3001/remoting/webapp/cli...
File remoting/webapp/client_plugin_async.js (right):

https://chromiumcodereview.appspot.com/10221021/diff/3001/remoting/webapp/cli...
remoting/webapp/client_plugin_async.js:90:
remoting.ClientPluginAsync.prototype.handleMessage_ = function(message_str) {
On 2012/04/28 00:00:42, Wez wrote:
> cleanup: message_str -> messageStr

Done.

https://chromiumcodereview.appspot.com/10221021/diff/3001/remoting/webapp/hos...
File remoting/webapp/host_list.js (right):

https://chromiumcodereview.appspot.com/10221021/diff/3001/remoting/webapp/hos...
remoting/webapp/host_list.js:55: var cached_parsed = jsonParseSafe(cached);
On 2012/04/28 00:00:42, Wez wrote:
> nit: cached -> cachedStr, cached_parsed -> cached? Otherwise, cached_parsed ->
> cachedParsed.

Done.

https://chromiumcodereview.appspot.com/10221021/diff/3001/remoting/webapp/hos...
remoting/webapp/host_list.js:125: var parsed_response =
On 2012/04/28 00:00:42, Wez wrote:
> nit: parsed_response -> parsedResponse, or just response.

Done.

https://chromiumcodereview.appspot.com/10221021/diff/3001/remoting/webapp/hos...
remoting/webapp/host_list.js:145: console.error('Invalid "hosts" response from
server.');
On 2012/04/28 00:00:42, Wez wrote:
> nit: Suggest "Invalid host list from server."

I'd prefer to keep the API mentioned explicitly in the message.

https://chromiumcodereview.appspot.com/10221021/diff/3001/remoting/webapp/oau...
File remoting/webapp/oauth2.js (right):

https://chromiumcodereview.appspot.com/10221021/diff/3001/remoting/webapp/oau...
remoting/webapp/oauth2.js:225: console.error('Invalid "token" response from
server.');
On 2012/04/28 00:00:42, Wez wrote:
> nit: Lose the quotes around token.

Ditto.

https://chromiumcodereview.appspot.com/10221021/diff/3001/remoting/webapp/rem...
File remoting/webapp/remoting.js (right):

https://chromiumcodereview.appspot.com/10221021/diff/3001/remoting/webapp/rem...
remoting/webapp/remoting.js:130: jsonParseSafe(xhr.responseText);
On 2012/04/28 00:00:42, Wez wrote:
> I'd suggest leaving this one JSON.parse(), so that the web-app will fail to
work
> if we manage to break it in such a fundamental way.

TBH, I'm pretty sure it won't even load if the manifest is invalid. I changed it
just so that there is only one place where we call JSON.parse. If it does manage
to load, then we just lose the version logging, which is not the end of the
world.

https://chromiumcodereview.appspot.com/10221021/diff/3001/remoting/webapp/rem...
remoting/webapp/remoting.js:288: function jsonParseSafe(jsonString) {
On 2012/04/28 00:00:42, Wez wrote:
> nit: parseJsonOrNull?

That was what I called it originally, but it doesn't return null (see below) and
jsonParseOrUndefined just didn't sound right.

https://chromiumcodereview.appspot.com/10221021/diff/3001/remoting/webapp/rem...
remoting/webapp/remoting.js:292: return undefined;
On 2012/04/28 00:00:42, Wez wrote:
> Is the aim in returning undefined that if the caller uses the value without
> testing then we'll see errors about using undefined values?

No. JSON.parse("null") legitimately returns null and I wanted to make sure we
can distinguish that case if we ever want to. AFAICT, it can never return
undefined (JSON.stringify(undefined) = undefined, not "undefined"). Trying to
access a property or using the 'in' operator on either null or undefined is
illegal, so there's no difference in that respect.

[Wez, 2012-04-28 00:32:03]

lgtm

https://chromiumcodereview.appspot.com/10221021/diff/3001/remoting/webapp/rem...
File remoting/webapp/remoting.js (right):

https://chromiumcodereview.appspot.com/10221021/diff/3001/remoting/webapp/rem...
remoting/webapp/remoting.js:288: function jsonParseSafe(jsonString) {
On 2012/04/28 00:15:41, Jamie wrote:
> On 2012/04/28 00:00:42, Wez wrote:
> > nit: parseJsonOrNull?
> 
> That was what I called it originally, but it doesn't return null (see below)
and
> jsonParseOrUndefined just didn't sound right.

In that case safeParseJson(), then? :)