Bug 55775 – Web Inspector: introduce RuntimeAgent::evaluateAsJSON, implement completion, audits and image hover on its basis.

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Web Inspector (Deprecated)

[Pavel Feldman]

Patch to follow.

[Pavel Feldman]

Created attachment 84755 [details]
Patch

[Ilya Tikhonovsky]

Comment on attachment 84755 [details]
Patch

please implement a test for this function in inspector/protocol/runtime-agent.html

[Ilya Tikhonovsky]

Created attachment 85144 [details]
[patch] second version. with test.

[Yury Semikhatsky]

Comment on attachment 85144 [details]
[patch] second version. with test.

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

> Source/WebCore/inspector/InjectedScriptSource.js:253
> +        // FIXME: validate JSON-stringifiable structure.

If returned structure has cycles it will crash the browser. We should add some check here or use custom jsonificator here. Another option is to use hard-coded limit on the structure nesting level. Something like 10-20 levels should work fine for us. Please fix this.

[Alexander Pavlov (apavlov)]

Created attachment 88984 [details]
[PATCH] Suggested solution with a cyclic structure check (the depth limit is 1000 levels)

[Yury Semikhatsky]

Comment on attachment 88984 [details]
[PATCH] Suggested solution with a cyclic structure check (the depth limit is 1000 levels)

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

> Source/WebCore/bindings/v8/ScriptValue.cpp:84
> +            *errorMessage = "The object property hierarchy is too deep (probably, a cyclic object)";

Would be nice to have this message and the maxDepth constant defined in a common place.

> Source/WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:104
> +    host->inspectImpl(object.toInspectorValue(ScriptState::current(), 0), hints.toInspectorValue(ScriptState::current(), 0));

I'd rather pass error non-0 error message variable and add an ASSERT that toInspectorValue will never fail in this case.

> Source/WebCore/inspector/InjectedScriptSource.js:251
> +            return { value: inspectedWindow.eval(expression) };

Please make sure that in case "expression" evaluates to an object with methods it won't break ScriptValue.toInspectorValue.

[Alexander Pavlov (apavlov)]

(In reply to comment #6)
> (From update of attachment 88984 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88984&action=review
> 
> > Source/WebCore/bindings/v8/ScriptValue.cpp:84
> > +            *errorMessage = "The object property hierarchy is too deep (probably, a cyclic object)";
> 
> Would be nice to have this message and the maxDepth constant defined in a common place.

I suggest that we do it in a separate patch, since this patch scope is creeping rapidly.

> > Source/WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:104
> > +    host->inspectImpl(object.toInspectorValue(ScriptState::current(), 0), hints.toInspectorValue(ScriptState::current(), 0));
> 
> I'd rather pass error non-0 error message variable and add an ASSERT that toInspectorValue will never fail in this case.

Done.

> > Source/WebCore/inspector/InjectedScriptSource.js:251
> > +            return { value: inspectedWindow.eval(expression) };
> 
> Please make sure that in case "expression" evaluates to an object with methods it won't break ScriptValue.toInspectorValue.

Done.

A fixed patch will be up for review shortly.

[Alexander Pavlov (apavlov)]

Created attachment 89003 [details]
[PATCH] Comments addressed

[Yury Semikhatsky]

Comment on attachment 89003 [details]
[PATCH] Comments addressed

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

> Source/WebCore/bindings/v8/ScriptValue.cpp:125
> +    if (value->IsFunction()) {

I'd rather return 0 with a error message no matter what the value is here.

[Alexander Pavlov (apavlov)]

Created attachment 89008 [details]
[PATCH] Function detection comment addressed

[Alexander Pavlov (apavlov)]

(In reply to comment #9)
> (From update of attachment 89003 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89003&action=review
> 
> > Source/WebCore/bindings/v8/ScriptValue.cpp:125
> > +    if (value->IsFunction()) {
> 
> I'd rather return 0 with a error message no matter what the value is here.

Function is always an object, so reworked the patch accordingly.

[Pavel Feldman]

Comment on attachment 89008 [details]
[PATCH] Function detection comment addressed

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

> Source/WebCore/bindings/js/JSInjectedScriptHostCustom.cpp:121
> +        ASSERT_UNUSED(objectError, objectError.isEmpty());

Why ASSERT_UNUSED?

> Source/WebCore/bindings/js/ScriptValue.cpp:150
> +        if (getCallData(value, callData) != CallTypeNone) {

You should mimic JSON behavior and silently skip functions or call the original method other than evaluateAsJSON. Rationale: others can add functions to your prototype.

> Source/WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:107
> +    ASSERT_UNUSED(objectError, objectError.isEmpty());

ditto

> Source/WebCore/inspector/InjectedScript.cpp:212
> +            String resultMessage = "Failed to construct resulting value";

You should already have a valid error message, result should be empty.

> Source/WebCore/inspector/InjectedScript.cpp:218
> +        *result = InspectorString::create("Exception while making a call");

ditto. You should not touch result in case or error.

> Source/WebCore/inspector/InjectedScriptSource.js:253
> +            return false;

Silently ignoring user exceptions is bad, r- is primarily for this. Please wrap the error as it is done in the other evaluates.

> Source/WebCore/inspector/InspectorRuntimeAgent.cpp:62
> +    InjectedScript injectedScript = m_injectedScriptManager->injectedScriptFor(getDefaultInspectedState());

This way it won't work for iframes where you probably want it to work. You might want to move this method to DOM agent and pass context node for injected script lookup.

[Alexander Pavlov (apavlov)]

We've got a workaround, so this change is not needed.