Bug 53072 – Web Inspector: evaluate in console may not work when window.console is substituted

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

[Pavel Podivilov]

Web Inspector: evaluate in console may not work when window.console is substituted

Initial bug - http://code.google.com/p/chromium/issues/detail?id=70629.

[Pavel Podivilov]

Created attachment 80039 [details]
Patch.

[Yury Semikhatsky]

Comment on attachment 80039 [details]
Patch.

Can you provide a test for this?

[Yury Semikhatsky]

Comment on attachment 80039 [details]
Patch.

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

> Source/WebCore/inspector/InjectedScriptSource.js:261
> +        expression = "with ((window && window.console && window.console._commandLineAPI) || {}) {\n" + expression + "\n}";

If .console is undefined then this method will fail at line 256 and not get here:

inspectedWindow.console._commandLineAPI = this._commandLineAPI;

[Pavel Podivilov]

(In reply to comment #3)
> (From update of attachment 80039 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80039&action=review
> 
> > Source/WebCore/inspector/InjectedScriptSource.js:261
> > +        expression = "with ((window && window.console && window.console._commandLineAPI) || {}) {\n" + expression + "\n}";
> 
> If .console is undefined then this method will fail at line 256 and not get here:
> 
> inspectedWindow.console._commandLineAPI = this._commandLineAPI;

Not undefined, but substituted. In any case this patch looks safe and fixes the problem.

[Pavel Podivilov]

Created attachment 80063 [details]
Added test.

[Yury Semikhatsky]

Comment on attachment 80063 [details]
Added test.

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

> LayoutTests/inspector/console-substituted.html:21
> +    InspectorTest.evaluateInConsole("setTimeout(deleteConsole, 0)", step1);

Why do you need setTimeout here?

> LayoutTests/inspector/console-substituted.html:31
> +        InspectorTest.evaluateInConsole("setTimeout(substituteConsole, 0)", step3);

The same question for this call.

> Source/WebCore/inspector/InjectedScriptSource.js:261
> +            expression = "with (window || {}) {\n" + expression + "\n} ";

is there a real scenario when there are no window?

[Pavel Podivilov]

(In reply to comment #6)
> (From update of attachment 80063 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80063&action=review
> 
> > LayoutTests/inspector/console-substituted.html:21
> > +    InspectorTest.evaluateInConsole("setTimeout(deleteConsole, 0)", step1);
> 
> Why do you need setTimeout here?
> 
> > LayoutTests/inspector/console-substituted.html:31
> > +        InspectorTest.evaluateInConsole("setTimeout(substituteConsole, 0)", step3);
> 
> The same question for this call.
> 
> > Source/WebCore/inspector/InjectedScriptSource.js:261
> > +            expression = "with (window || {}) {\n" + expression + "\n} ";
> 
> is there a real scenario when there are no window?

Sure, you may be inside a closure created in function with "window" argument, which is not used by that closure. In that case "window" will be undefined (v8 will drop it).

[Pavel Podivilov]

(In reply to comment #6)
> (From update of attachment 80063 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80063&action=review
> 
> > LayoutTests/inspector/console-substituted.html:21
> > +    InspectorTest.evaluateInConsole("setTimeout(deleteConsole, 0)", step1);
> 
> Why do you need setTimeout here?

Replaced with direct call.

> 
> > LayoutTests/inspector/console-substituted.html:31
> > +        InspectorTest.evaluateInConsole("setTimeout(substituteConsole, 0)", step3);
> 
> The same question for this call.

Replaced with direct call.

> 
> > Source/WebCore/inspector/InjectedScriptSource.js:261
> > +            expression = "with (window || {}) {\n" + expression + "\n} ";
> 
> is there a real scenario when there are no window?

Fixed. Since this is behind !isEvalOnCallFrame, there is probably no such scenario.

[Pavel Podivilov]

Created attachment 80176 [details]
Patch.

[Pavel Feldman]

Comment on attachment 80176 [details]
Patch.

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

> Source/WebCore/inspector/InjectedScriptSource.js:257
> +            inspectedWindow.console._commandLineAPI = this._commandLineAPI;

Nit: defining non-enumerable property would be even better.

[Pavel Podivilov]

Committed r76699: <http://trac.webkit.org/changeset/76699>