Bug 165506 – Inspector::InspectorValue class doesn't handle undefined type

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Karim]

InspectorValue class doesn't handle undefined type witch may cause issues with other classes that handle undefined type.

[Karim]

Created attachment 296360 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 296360 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=296360&action=review

> Source/JavaScriptCore/ChangeLog:4
> +        Added undefined type and fixed related issues
> +        https://bugs.webkit.org/show_bug.cgi?id=165506

"undefined" is not a valid JSON value type:
http://www.json.org
http://www.ecma-international.org/publications/files/ECMA-ST/ECMA-404.pdf

>>> JSON.stringify({a:undefined})
{}

>>> JSON.stringify({a:null})
{"a":null}

Is there a specific reason we want to be adding it to InspectorValues?

> Source/JavaScriptCore/ChangeLog:11
> +        * runtime/VM.h:

This looks stale, the diff doesn't show any modifications to VM.h.

[Karim]

There is a FIXME tag at:JavaScriptCore/bindings/ScriptValue.cpp:56
// FIXME: Why is it OK to turn undefined into null?

> Comment on attachment 296360 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=296360&action=review
> 
> > Source/JavaScriptCore/ChangeLog:4
> > +        Added undefined type and fixed related issues
> > +        https://bugs.webkit.org/show_bug.cgi?id=165506
> 
> "undefined" is not a valid JSON value type:
> http://www.json.org
> http://www.ecma-international.org/publications/files/ECMA-ST/ECMA-404.pdf
> 
> >>> JSON.stringify({a:undefined})
> {}
> 
> >>> JSON.stringify({a:null})
> {"a":null}
> 
> Is there a specific reason we want to be adding it to InspectorValues?
> 
> > Source/JavaScriptCore/ChangeLog:11
> > +        * runtime/VM.h:
> 
> This looks stale, the diff doesn't show any modifications to VM.h.

[Karim]

Comment on attachment 296360 [details]
Patch

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 209439)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,15 @@
>[phone]  Karim H  <[e-mail]>
>+
>+        Added undefined type and fixed related issues
>+        https://bugs.webkit.org/show_bug.cgi?id=165506
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * inspector/InspectorValues.cpp:
>+        (Inspector::InspectorValue::undefined):
>+        * inspector/InspectorValues.h:
>+
> 2016-12-06  Alexey Proskuryakov  <[e-mail]>
> 
>         Correct SDKROOT values in xcconfig files
>Index: Source/JavaScriptCore/inspector/InspectorValues.cpp
>===================================================================
>--- Source/JavaScriptCore/inspector/InspectorValues.cpp	(revision 209405)
>+++ Source/JavaScriptCore/inspector/InspectorValues.cpp	(working copy)
>@@ -485,6 +485,11 @@ Ref<InspectorValue> InspectorValue::null
>     return adoptRef(*new InspectorValue);
> }
> 
>+Ref<InspectorValue> InspectorValue::undefined()
>+{
>+    return adoptRef(*new InspectorValue(Type::Undefined));
>+}
>+
> Ref<InspectorValue> InspectorValue::create(bool value)
> {
>     return adoptRef(*new InspectorValue(value));
>Index: Source/JavaScriptCore/inspector/InspectorValues.h
>===================================================================
>--- Source/JavaScriptCore/inspector/InspectorValues.h	(revision 209405)
>+++ Source/JavaScriptCore/inspector/InspectorValues.h	(working copy)
>@@ -64,11 +64,13 @@ public:
>             break;
>         case Type::Object:
>         case Type::Array:
>+        case Type::Undefined:
>             break;
>         }
>     }
> 
>     static Ref<InspectorValue> null();
>+    static Ref<InspectorValue> undefined();
>     static Ref<InspectorValue> create(bool);
>     static Ref<InspectorValue> create(int);
>     static Ref<InspectorValue> create(double);
>@@ -83,10 +85,12 @@ public:
>         String,
>         Object,
>         Array,
>+        Undefined,
>     };
> 
>     Type type() const { return m_type; }
>     bool isNull() const { return m_type == Type::Null; }
>+    bool isUndefined() const { return m_type == Type::Undefined; }
> 
>     bool asBoolean(bool&) const;
>     bool asInteger(int&) const;

[Joseph Pecoraro]

Sounds like we have a reason, we should remove the FIXME comment.

[Blaze Burg]

Please just remove the FIXME. There is nothing to fix.

[Karim]

Created attachment 296567 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 296567 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=296567&action=review

> Source/JavaScriptCore/ChangeLog:9
> +        * bindings/ScriptValue.cpp:
> +        (Inspector::jsToInspectorValue):

We should include a description here about why. Something like:

It is okay to turn undefined into null because we are producing values for a
JSON representation (InspectorValue) and JSON has a `null` value and no
`undefined` value.

[Karim]

Created attachment 296574 [details]
Patch

[Build Bot]

Comment on attachment 296574 [details]
Patch

Attachment 296574 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2656533

New failing tests:
http/tests/xmlhttprequest/access-control-repeated-failed-preflight-crash.html

[Build Bot]

Created attachment 296583 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[WebKit Commit Bot]

Comment on attachment 296574 [details]
Patch

Clearing flags on attachment: 296574

Committed r209650: <http://trac.webkit.org/changeset/209650>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.