Bug 57557 – [Crash] Web Inspector: inspected page crashes on attempt to log object with broken .toString

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

[Yury Semikhatsky]

Steps to reproduce:

1. Navigate to a page with the following content:
<script> 
console.log({toString:{}});
</script> 
2. Open inspector.
3. Reload the inspected page.

Result:
Page crashes.

Original Chromium bug report:
http://code.google.com/p/chromium/issues/detail?id=74411

[Yury Semikhatsky]

Clearing [V8] prefix since it's reproducible with JSC as well.

[Yury Semikhatsky]

Created attachment 87765 [details]
Patch

[Yury Semikhatsky]

A big thanks to Anton Muhin for his help in finding the real cause of this crash.

[anton muhin]

Comment on attachment 87765 [details]
Patch

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

LGTM

> LayoutTests/inspector/console/console-log-toString-object.html:20
> +Tests passing object with broken toString attribute into console.log won't crash

nit: I am having some troubles parsing this sentence, maybe Test <that> passing object...  And maybe you should generalize the case: passing an object which throws on string conversion or something of this kind

> Source/WebCore/ChangeLog:8
> +        If ScriptValue.toString causes a JavaScript exception the exception is cleared

nit: exception<,> the exception (add a comma)

> Source/WebCore/bindings/v8/ScriptValue.cpp:72
> +    return v8StringToWebCoreString<String>(s, DoNotExternalize);

Another option would be to teach V8Parameter not to rethrow an exception.  The benefit would be your code would be shorter and others can use the same logic too.  Up to you though,

[Yury Semikhatsky]

Created attachment 87829 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #4)
> (From update of attachment 87765 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87765&action=review
> > LayoutTests/inspector/console/console-log-toString-object.html:20
> > +Tests passing object with broken toString attribute into console.log won't crash
> 
> nit: I am having some troubles parsing this sentence, maybe Test <that> passing object...  And maybe you should generalize the case: passing an object which throws on string conversion or something of this kind
> 
Done.

> > Source/WebCore/ChangeLog:8
> > +        If ScriptValue.toString causes a JavaScript exception the exception is cleared
> 
> nit: exception<,> the exception (add a comma)
> 
Done.

> > Source/WebCore/bindings/v8/ScriptValue.cpp:72
> > +    return v8StringToWebCoreString<String>(s, DoNotExternalize);
> 
> Another option would be to teach V8Parameter not to rethrow an exception.  The benefit would be your code would be shorter and others can use the same logic too.  Up to you though,

I'd rather do this in a separate change.

[Yury Semikhatsky]

Committed r82664: <http://trac.webkit.org/changeset/82664>