Buggy code in JsonUtils.java

[GoogleCodeExporter]

Google Chrome version: n/a

SDK + Eclipse Debugger version(s): 0.1.3

OS + version: n/a

I believe there is buggy code in the JsonUtils.java

Go to:

http://code.google.com/p/chromedevtools/source/browse/trunk/plugins/org.chr
omium.sdk/src/org/chromium/sdk/internal/JsonUtil.java

Examine the function: getAsLong

The line:

 if (v instanceof Long || v == null) {

should be:

 if (v instanceof Long || v != null) {

There are more examples of this mistake in the file.

Original issue reported on code.google.com by alistair.rutherford on 18 Dec 2009 at 9:56

[GoogleCodeExporter]

Note that line you are proposing
"if (v instanceof Long || v != null) {"

is equivalent to

"if (v != null) {"

Doesn't seem like a good fix to me.

Original comment by peter.ry...@gmail.com on 18 Dec 2009 at 10:08

[GoogleCodeExporter]

Quite right. Then it should be

if (v != null && v instanceof Long) {"

This will short-cut on the first test.

The original line is still incorrect.

Original comment by alistair.rutherford on 19 Dec 2009 at 2:19

[GoogleCodeExporter]

I don't think 
"if (v != null && v instanceof Long) {"
is really much faster than
"if (v instanceof Long) {"

However, why incorrect?
Have you read javadoc for this method? It says the method returns null or Long.

The only improvement your fix offers is additional logging, in other aspects 
method's 
behavior stays unchanged. Was logging the thing you wanted to correct?

Original comment by peter.ry...@gmail.com on 19 Dec 2009 at 2:35

[GoogleCodeExporter]

If you examine the function below from the file JsonUntil.java:

public static Long getAsLong(JSONObject obj, CharSequence key) {
  String keyString = key.toString();
  Object v = obj.get(keyString);
  if (v instanceof Long || v == null) {
    return (Long) v;
  }

  LOGGER.log(Level.SEVERE, "Key: {0}, found value: {1}", new Object[] {keyString, v});
  return null;
}

If the line

  Object v = obj.get(keyString);

returns null as in "key not found"

The the line

  if (v instanceof Long || v == null) {

will fall into the cast and you will get an exception when the return attempts 
to cast (Long)v. The 
javadoc for the function clearly states "@return null if key not found or bad 
type" so yes I have read 
what the javadoc says. The only way to match what the "javadoc" states is to 
change the test to:

  if (v!=null && v instanceof Long)

which will shortcut on the first test and return null (correct) or give you the 
value.

The line 

  if (v instanceof Long || v == null) {

is therefore incorrect and this mistake is repeated in the following functions.

getAsLong
getAsDouble
getAsString
getAsJSON
getAsJSONArray

I know because I was running the code in debug mode and I passed in an object 
which did not contain the 
key. I traced the exception down to the function getAsLong.

Original comment by alistair.rutherford on 20 Dec 2009 at 6:11

[GoogleCodeExporter]

In my version of Java people may cast null to any (reference) type.

Perhaps you might have run into auto-boxing-related problem. Like this:

Object v = null;
Long l1 = (Long)v;
long l2 = (long)(Long)v;

The third line fails with NPE.

Original comment by peter.ry...@gmail.com on 20 Dec 2009 at 6:39

[GoogleCodeExporter]

The version of java has got nothing to do with the fact that this line:

    if (v instanceof Long || v == null)

Doesn't make any sense. If (v==null) why would you want to attempt this cast? 

Original comment by alistair.rutherford on 20 Dec 2009 at 7:03

[GoogleCodeExporter]

Probably we should discuss issues with the project here, not Java language.

What we have here is a correct, well-readable, reasonably short and reasonably 
fast 
Java code. I don't see how you may write better.
We do not have a goal of never casting null to other reference types.

Original comment by peter.ry...@gmail.com on 20 Dec 2009 at 7:14

[GoogleCodeExporter]

Original comment by peter.ry...@gmail.com on 22 Dec 2009 at 11:17

• Changed state: WontFix