Bug 55775

Summary: Web Inspector: introduce RuntimeAgent::evaluateAsJSON, implement completion, audits and image hover on its basis.
Product: WebKit Reporter: Pavel Feldman <pfeldman>
Component: Web Inspector (Deprecated)Assignee: Alexander Pavlov (apavlov) <apavlov>
Status: RESOLVED WONTFIX    
Severity: Normal CC: apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
[patch] second version. with test.
yurys: review-
[PATCH] Suggested solution with a cyclic structure check (the depth limit is 1000 levels)
none
[PATCH] Comments addressed
none
[PATCH] Function detection comment addressed pfeldman: review-

Description Pavel Feldman 2011-03-04 08:19:52 PST
Patch to follow.
Comment 1 Pavel Feldman 2011-03-04 08:30:02 PST
Created attachment 84755 [details]
Patch
Comment 2 Ilya Tikhonovsky 2011-03-04 09:18:31 PST
Comment on attachment 84755 [details]
Patch

please implement a test for this function in inspector/protocol/runtime-agent.html
Comment 3 Ilya Tikhonovsky 2011-03-09 01:20:13 PST
Created attachment 85144 [details]
[patch] second version. with test.
Comment 4 Yury Semikhatsky 2011-03-09 01:38:49 PST
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.
Comment 5 Alexander Pavlov (apavlov) 2011-04-11 03:54:12 PDT
Created attachment 88984 [details]
[PATCH] Suggested solution with a cyclic structure check (the depth limit is 1000 levels)
Comment 6 Yury Semikhatsky 2011-04-11 04:39:19 PDT
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.
Comment 7 Alexander Pavlov (apavlov) 2011-04-11 07:20:02 PDT
(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.
Comment 8 Alexander Pavlov (apavlov) 2011-04-11 07:21:57 PDT
Created attachment 89003 [details]
[PATCH] Comments addressed
Comment 9 Yury Semikhatsky 2011-04-11 07:40:18 PDT
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.
Comment 10 Alexander Pavlov (apavlov) 2011-04-11 08:23:52 PDT
Created attachment 89008 [details]
[PATCH] Function detection comment addressed
Comment 11 Alexander Pavlov (apavlov) 2011-04-11 08:24:17 PDT
(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.
Comment 12 Pavel Feldman 2011-04-11 22:16:29 PDT
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.
Comment 13 Alexander Pavlov (apavlov) 2011-05-24 04:14:37 PDT
We've got a workaround, so this change is not needed.