Bug 201729 – Uncaught Exception: null is not an object (evaluating 'Object.keys(propertyNames)​')​ (at JavaScriptRuntimeCompletionProvider.js:​244:​57)​

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Not sure exactly how to reproduce yet. Was typing:
1. js> entry.contentView.|

When 

Uncaught Exception:
----------------------
null is not an object (evaluating 'Object.keys(propertyNames)​')​ (at JavaScriptRuntimeCompletionProvider.js:​244:​57)​
    keys @ [native code]​
    receivedObjectPropertyNames @ JavaScriptRuntimeCompletionProvider.js:​244:​57
    receivedObjectPropertyNames @ [native code]​
    mycallback @ RemoteObject.js:​516:​21
    _dispatchResponseToCallback @ Connection.js:​148:​27
    _dispatchResponse @ Connection.js:​118:​45
    dispatch @ Connection.js:​70:​35
    dispatchMessageFromTarget @ TargetManager.js:​143:​35
    dispatchMessageFromTarget @ TargetObserver.js:​42:​51
    dispatchEvent @ InspectorBackend.js:​340:​42
    _dispatchEvent @ Connection.js:​195:​32
    dispatch @ Connection.js:​72:​32
    dispatch @ InspectorBackend.js:​178:​52
    ?​ @ MessageDispatcher.js:​42:​34
----------------------

[Joseph Pecoraro]

This seems like a race when typing very fast.

Calls to:

    RuntimeAgent.releaseObjectGroup("completion")

From a prior evaluation will wipe out an ongoing request.

---

Typing: `entry.contentView` quickly in the quick console when paused:

[Log] request: {
        "id": 675,
        "method": "Debugger.evaluateOnCallFrame",
        "params": {
            "callFrameId": "{\"ordinal\":2,\"injectedScriptId\":1}",
            "expression": "\n//# sourceURL=__WebInspectorInternal__\nentry",
            "objectGroup": "completion",
            "includeCommandLineAPI": true,
            "doNotPauseOnExceptionsAndMuteConsole": true,
            "returnByValue": false,
            "generatePreview": false,
            "saveResult": false,
            "emulateUserGesture": false
        }
    }

[Log] response: {
        "result": {
            "result": {
                "type": "object",
                "objectId": "{\"injectedScriptId\":1,\"id\":4003}",
                "className": "BackForwardEntry",
                "description": "BackForwardEntry"
            }
        },
        "id": 675

[Log] request: {
        "id": 701,
        "method": "Runtime.callFunctionOn",
        "params": {
            "objectId": "{\"injectedScriptId\":1,\"id\":4003}",
            "functionDeclaration": "\n//# sourceURL=__WebInspectorInternal__\nfunction inspectedPage_evalResult_getCompletions(primitiveType)\n            {\n                var object;\n                if (primitiveType === \"string\")\n                    object = new String(\"\");\n                else if (primitiveType === \"number\")\n                    object = new Number(0);\n                else if (primitiveType === \"boolean\")\n                    object = new Boolean(false);\n                else if (primitiveType === \"symbol\")\n                    object = Symbol();\n                else\n                    object = this;\n\n                var resultSet = {};\n                for (var o = object; o; o = o.__proto__) {\n                    try {\n                        var names = Object.getOwnPropertyNames(o);\n                        for (var i = 0; i < names.length; ++i)\n                            resultSet[names[i]] = true;\n                    } catch (e) { }\n                }\n\n                return resultSet;\n            }",
            "doNotPauseOnExceptionsAndMuteConsole": true,
            "returnByValue": true
        }
    }

...

[Log] request: {
        "id": 703,
        "method": "Runtime.releaseObjectGroup",
        "params": {
            "objectGroup": "completion"
        }
    }

...

[Log] response: {
        "error": {
            "code": -32000,
            "message": "Could not find object with given id",
            "data": [{
                "code": -32000,
                "message": "Could not find object with given id"
            }]
        },
        "id": 701
    }

[Joseph Pecoraro]

Created attachment 378667 [details]
[PATCH] Proposed Fix

[Devin Rousso]

Comment on attachment 378667 [details]
[PATCH] Proposed Fix

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

r=me, nice!  I've encountered this a handful of times but could never reproduce.  Great catch! =D

> Source/WebInspectorUI/UserInterface/Controllers/JavaScriptRuntimeCompletionProvider.js:378
> +        this._ongoingCompletionRequests++;

Style: `++this._ongoingCompletionRequests;`

> Source/WebInspectorUI/UserInterface/Controllers/JavaScriptRuntimeCompletionProvider.js:381
> +    _decrementOngoingCompletionRequests()

Style: `--this._ongoingCompletionRequests;`

> Source/WebInspectorUI/UserInterface/Controllers/JavaScriptRuntimeCompletionProvider.js:385
> +        if (!this._ongoingCompletionRequests)

We should assert that we haven't gone below `0` (and maybe even change the `if` condition to reflect that as well, given how many different branches this code has).

[Joseph Pecoraro]

Comment on attachment 378667 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Controllers/JavaScriptRuntimeCompletionProvider.js:378
>> +        this._ongoingCompletionRequests++;
> 
> Style: `++this._ongoingCompletionRequests;`

There is no reason to prefer pre or post in JavaScript. We have hundreds of examples of each. I think readability should matter more and post is more readable in `this._member` cases, and our existing code seems to agree.

>> Source/WebInspectorUI/UserInterface/Controllers/JavaScriptRuntimeCompletionProvider.js:385
>> +        if (!this._ongoingCompletionRequests)
> 
> We should assert that we haven't gone below `0` (and maybe even change the `if` condition to reflect that as well, given how many different branches this code has).

Okay. It seems more likely that a decrement will be missed, in which case the counter ever increases, which is not easy to catch with assertions.

[Joseph Pecoraro]

Source/WebInspectorUI/UserInterface/Controllers/JavaScriptRuntimeCompletionProvider.js:385
> >> +        if (!this._ongoingCompletionRequests)
> > 
> > We should assert that we haven't gone below `0` (and maybe even change the `if` condition to reflect that as well, given how many different branches this code has).
> 
> Okay. It seems more likely that a decrement will be missed, in which case
> the counter ever increases, which is not easy to catch with assertions.

I think I'll add an assertion that we never get above 50. If so something has probably gone wrong and it would be useful to alert us of that in development!

[Joseph Pecoraro]

https://trac.webkit.org/changeset/249846/webkit

[Radar WebKit Bug Importer]

<rdar://problem/55347422>