Bug 52981 – REGRESSION (r72895): console.trace crashes

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

callStack becomes null when passed to addMessage(), so calling callStack->size() just crashes.

PassRefPtr should not be used here at all - it's only needed as an optimization when passing ownership. So, neither trace() nor addMessage() should take a PassRefPtr<ScriptCallStack>, they should just take ScriptCallStack*. Of course, this also applies to PassRefPtr<ScriptArguments> arguments and many other functions in this file, even though they don't crash right now.

Why don't we have any tests for console.trace at all?

[Alexey Proskuryakov]

Please see <http://webkit.org/coding/RefPtr.html> for information on using RefPtr and PassRefPtr.

[Alexey Proskuryakov]

<rdar://problem/8905076>

[Yury Semikhatsky]

(In reply to comment #0)
> 
> Why don't we have any tests for console.trace at all?

There are at lest two tests for console.trace:

LayoutTests/inspector/console-trace.html
LayoutTests/inspector/console-trace-in-eval.html

[Yury Semikhatsky]

Created attachment 79902 [details]
Patch

[WebKit Review Bot]

Attachment 79902 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/inspector/InspectorDatabaseInstrumentation.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yury Semikhatsky]

Committed r76493: <http://trac.webkit.org/changeset/76493>

[Yury Semikhatsky]

(In reply to comment #6)
> Committed r76493: <http://trac.webkit.org/changeset/76493>

Style errors were fixed before landing.

[Alexey Proskuryakov]

-void Console::trace(PassRefPtr<ScriptArguments> arguments, PassRefPtr<ScriptCallStack> callStack)
+void Console::trace(PassRefPtr<ScriptArguments> arguments, PassRefPtr<ScriptCallStack> prpCallStack)

This patch is wrong. Argument types should be ScriptArguments* and ScriptCallStack*.

> There are at lest two tests for console.trace:

How come these don't crash, are they disabled on most platforms? And if these didn't crash somehow, what regression test covers this fix?

[Yury Semikhatsky]

(In reply to comment #8)
> -void Console::trace(PassRefPtr<ScriptArguments> arguments, PassRefPtr<ScriptCallStack> callStack)
> +void Console::trace(PassRefPtr<ScriptArguments> arguments, PassRefPtr<ScriptCallStack> prpCallStack)
> 
> This patch is wrong. Argument types should be ScriptArguments* and ScriptCallStack*.
> 
I disagree with you here. Console::trace passes both callStack and arguments to addMessage which is supposed to create a console message with RefPtrs to these objects.  This code follows the following recommedation from http://webkit.org/coding/RefPtr.html:

" If a function does take ownership of an object, the argument should be a PassRefPtr. This includes most setter functions. Unless the use of the argument is very simple, the argument should be transferred to a RefPtr at the start of the function; the argument can be named with a “prp” prefix in such cases. "


> > There are at lest two tests for console.trace:
> 
> How come these don't crash, are they disabled on most platforms? And if these didn't crash somehow, what regression test covers this fix?

The code that lead to the crashes you described is guarded with Console::shouldPrintExceptions() which is only set from WebKit/mac/Misc/WebCoreStatistics.mm and hence is never executed in layout tests. We should consider moving the code that prints messages to console to the WebKit layer instead of calling printf from WebCore.

[Alexey Proskuryakov]

> I disagree with you here.

There is a very simple and practical explanation of why your reading is wrong. By following it, you introduced a crasher.  That wouldn't have happened if you followed a common PassRepPtr usage pattern.