Issue 312143003: This fixes bug with trying to get properties of non-object (symbol).

Issue 312143003: This fixes bug with trying to get properties of non-object (symbol). (Closed)

Created:
6 years, 6 months ago by Alexandra Mikhaylova

Modified:
6 years, 6 months ago

Reviewers:
aandrey, yurys

CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, aandrey+blink_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Visibility:
Public.

More Reviews

Description

This fixes bug with trying to get properties of non-object (symbol). BUG=376194 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175709

Patch Set 1 #

Total comments: 6

Patch Set 2 : Address review comments #

Total comments: 6

Patch Set 3 : Address comments #

Total comments: 10

Patch Set 4 : Address comments #

Patch Set 5 : REBASE #

Created: 6 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+41 lines, -13 lines)			Patch
M	LayoutTests/inspector/console/console-dir.html	View	1 2 3	1 chunk	+3 lines, -0 lines	0 comments	Download
M	LayoutTests/inspector/console/console-dir-es6.html	View	1 3	1 chunk	+3 lines, -0 lines	0 comments	Download
M	LayoutTests/inspector/console/console-dir-es6-expected.txt	View	1 3	2 chunks	+2 lines, -0 lines	0 comments	Download
M	LayoutTests/inspector/console/console-dir-expected.txt	View	1 2 3	2 chunks	+9 lines, -0 lines	0 comments	Download
M	Source/core/inspector/InjectedScriptExterns.js	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/inspector/InjectedScriptSource.js	View	1 2 3 4	10 chunks	+23 lines, -12 lines	0 comments	Download

Messages

Total messages: 23 (0 generated)

Expand Messages | Collapse Messages

aandrey

test! https://codereview.chromium.org/312143003/diff/1/Source/core/inspector/InjectedScriptSource.js File Source/core/inspector/InjectedScriptSource.js (right): https://codereview.chromium.org/312143003/diff/1/Source/core/inspector/InjectedScriptSource.js#newcode386 Source/core/inspector/InjectedScriptSource.js:386: if (!this._isDefined(object) || this._isSymbol(object)) typeof object !== "object" ...

6 years, 6 months ago (2014-06-04 13:03:51 UTC) #2

Alexandra Mikhaylova

Added a test as well. https://codereview.chromium.org/312143003/diff/1/Source/core/inspector/InjectedScriptSource.js File Source/core/inspector/InjectedScriptSource.js (right): https://codereview.chromium.org/312143003/diff/1/Source/core/inspector/InjectedScriptSource.js#newcode386 Source/core/inspector/InjectedScriptSource.js:386: if (!this._isDefined(object) || this._isSymbol(object)) ...

6 years, 6 months ago (2014-06-04 14:08:37 UTC) #3

aandrey

https://codereview.chromium.org/312143003/diff/20001/Source/core/inspector/InjectedScriptSource.js File Source/core/inspector/InjectedScriptSource.js (right): https://codereview.chromium.org/312143003/diff/20001/Source/core/inspector/InjectedScriptSource.js#newcode182 Source/core/inspector/InjectedScriptSource.js:182: /** @type {!Object.<number, Object>} */ @type {!Object.<number, (!Object|symbol)>} https://codereview.chromium.org/312143003/diff/20001/Source/core/inspector/InjectedScriptSource.js#newcode315 ...

6 years, 6 months ago (2014-06-04 15:39:07 UTC) #5

Alexandra Mikhaylova

6 years, 6 months ago (2014-06-05 13:27:43 UTC) #6

aandrey

looks good https://codereview.chromium.org/312143003/diff/40001/LayoutTests/inspector/console/console-dir-es6.html File LayoutTests/inspector/console/console-dir-es6.html (right): https://codereview.chromium.org/312143003/diff/40001/LayoutTests/inspector/console/console-dir-es6.html#newcode20 LayoutTests/inspector/console/console-dir-es6.html:20: console.dir(function() {}); this has nothing to do ...

6 years, 6 months ago (2014-06-05 15:13:23 UTC) #7

Alexandra Mikhaylova

https://codereview.chromium.org/312143003/diff/40001/LayoutTests/inspector/console/console-dir-es6.html File LayoutTests/inspector/console/console-dir-es6.html (right): https://codereview.chromium.org/312143003/diff/40001/LayoutTests/inspector/console/console-dir-es6.html#newcode20 LayoutTests/inspector/console/console-dir-es6.html:20: console.dir(function() {}); On 2014/06/05 15:13:23, aandrey wrote: > this ...

6 years, 6 months ago (2014-06-06 09:49:52 UTC) #8

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/amikhaylova@google.com/312143003/60001

6 years, 6 months ago (2014-06-06 10:02:25 UTC) #10

commit-bot: I haz the power

FYI, CQ is re-trying this CL (attempt #1). The failing builders are: blink_presubmit on tryserver.blink ...

6 years, 6 months ago (2014-06-06 11:04:28 UTC) #12

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

6 years, 6 months ago (2014-06-06 11:35:07 UTC) #14

commit-bot: I haz the power

Failed to apply patch for Source/core/inspector/InjectedScriptSource.js: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...

6 years, 6 months ago (2014-06-06 11:35:09 UTC) #15

Failed to apply patch for Source/core/inspector/InjectedScriptSource.js:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/inspector/InjectedScriptSource.js
  Hunk #1 FAILED at 173.
  Hunk #2 FAILED at 312.
  Hunk #3 succeeded at 384 (offset 1 line).
  Hunk #4 succeeded at 417 (offset 1 line).
  Hunk #5 succeeded at 500 (offset 1 line).
  Hunk #6 succeeded at 534 (offset 1 line).
  Hunk #7 FAILED at 875.
  Hunk #8 FAILED at 884.
  Hunk #9 succeeded at 988 (offset 3 lines).
  Hunk #10 succeeded at 1019 (offset 3 lines).
  4 out of 10 hunks FAILED -- saving rejects to file
Source/core/inspector/InjectedScriptSource.js.rej

Patch:       Source/core/inspector/InjectedScriptSource.js
Index: Source/core/inspector/InjectedScriptSource.js
diff --git a/Source/core/inspector/InjectedScriptSource.js
b/Source/core/inspector/InjectedScriptSource.js
index
b60be3e95288e365eddde8188f7fb2b7acba3747..7fcb53012fb1c26b219e8e9f1f887ef677f774b4
100644
--- a/Source/core/inspector/InjectedScriptSource.js
+++ b/Source/core/inspector/InjectedScriptSource.js
@@ -173,13 +173,24 @@ function max(a, b)
 }
 
 /**
+ * FIXME: Remove once ES6 is supported natively by JS compiler.
+ * @param {*} obj
+ * @return {boolean}
+ */
+function isSymbol(obj)
+{
+    var type = typeof obj;
+    return (type === "symbol");
+}
+
+/**
  * @constructor
  */
 var InjectedScript = function()
 {
     /** @type {number} */
     this._lastBoundObjectId = 1;
-    /** @type {!Object.<number, Object>} */
+    /** @type {!Object.<number, (Object|symbol)>} */
     this._idToWrappedObject = { __proto__: null };
     /** @type {!Object.<number, string>} */
     this._idToObjectGroupName = { __proto__: null };
@@ -312,7 +323,7 @@ InjectedScript.prototype = {
     },
 
     /**
-     * @param {Object} object
+     * @param {Object|symbol} object
      * @param {string=} objectGroupName
      * @return {string}
      */
@@ -383,8 +394,9 @@ InjectedScript.prototype = {
         var object = this._objectForId(parsedObjectId);
         var objectGroupName = this._idToObjectGroupName[parsedObjectId.id];
 
-        if (!this._isDefined(object))
+        if (!this._isDefined(object) || isSymbol(object))
             return false;
+        object = /** @type {Object} */ (object);
         var descriptors = this._propertyDescriptors(object, ownProperties,
accessorPropertiesOnly);
 
         // Go over properties, wrap object values.
@@ -415,8 +427,9 @@ InjectedScript.prototype = {
         var parsedObjectId = this._parseObjectId(objectId);
         var object = this._objectForId(parsedObjectId);
         var objectGroupName = this._idToObjectGroupName[parsedObjectId.id];
-        if (!this._isDefined(object))
+        if (!this._isDefined(object) || isSymbol(object))
             return false;
+        object = /** @type {Object} */ (object);
         var descriptors = [];
         var internalProperties =
InjectedScriptHost.getInternalProperties(object);
         if (internalProperties) {
@@ -497,8 +510,7 @@ InjectedScript.prototype = {
                     continue;
 
                 var name = property;
-                var type = typeof property;
-                if (type === "symbol")
+                if (isSymbol(property))
                     name = injectedScript._describe(property);
 
                 try {
@@ -532,7 +544,7 @@ InjectedScript.prototype = {
                 descriptor.name = name;
                 if (o === object)
                     descriptor.isOwn = true;
-                if (type === "symbol")
+                if (isSymbol(property))
                     descriptor.symbol = property;
                 push(descriptors, descriptor);
             }
@@ -874,7 +886,7 @@ InjectedScript.prototype = {
 
     /**
      * @param {Object} objectId
-     * @return {Object}
+     * @return {Object|symbol}
      */
     _objectForId: function(objectId)
     {
@@ -883,7 +895,7 @@ InjectedScript.prototype = {
 
     /**
      * @param {string} objectId
-     * @return {Object}
+     * @return {Object|symbol}
      */
     findObjectById: function(objectId)
     {
@@ -984,7 +996,6 @@ InjectedScript.prototype = {
         if (this.isPrimitiveValue(obj))
             return null;
 
-        var type = typeof obj;
         var subtype = this._subtype(obj);
 
         if (subtype === "regexp")
@@ -1016,10 +1027,10 @@ InjectedScript.prototype = {
         }
 
         // NodeList in JSC is a function, check for array prior to this.
-        if (type === "function")
+        if (typeof obj === "function")
             return toString(obj);
 
-        if (type === "symbol") {
+        if (isSymbol(obj)) {
             try {
                 return Symbol.prototype.toString.call(obj) || "Symbol";
             } catch (e) {

Alexandra Mikhaylova

The CQ bit was checked by amikhaylova@google.com

6 years, 6 months ago (2014-06-06 11:59:45 UTC) #16

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/amikhaylova@google.com/312143003/80001

6 years, 6 months ago (2014-06-06 12:01:24 UTC) #17

commit-bot: I haz the power

FYI, CQ is re-trying this CL (attempt #1). The failing builders are: blink_presubmit on tryserver.blink ...

6 years, 6 months ago (2014-06-06 13:19:23 UTC) #18

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

6 years, 6 months ago (2014-06-06 13:20:34 UTC) #19

commit-bot: I haz the power

Try jobs failed on following builders: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/10591)

6 years, 6 months ago (2014-06-06 13:20:36 UTC) #20

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/amikhaylova@google.com/312143003/80001

6 years, 6 months ago (2014-06-06 21:24:47 UTC) #22

Message was sent while issue was closed.

Change committed as 175709

Expand Messages | Collapse Messages