Bug 52981

Summary: REGRESSION (r72895): console.trace crashes
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: WebCore JavaScriptAssignee: Yury Semikhatsky <yurys>
Status: RESOLVED FIXED    
Severity: Normal CC: caseq, loislo, pfeldman, webkit.review.bot, yurys
Priority: P1 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch pfeldman: review+

Description Alexey Proskuryakov 2011-01-23 12:31:53 PST
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?
Comment 1 Alexey Proskuryakov 2011-01-23 12:32:38 PST
Please see <http://webkit.org/coding/RefPtr.html> for information on using RefPtr and PassRefPtr.
Comment 2 Alexey Proskuryakov 2011-01-23 12:33:22 PST
<rdar://problem/8905076>
Comment 3 Yury Semikhatsky 2011-01-24 00:00:22 PST
(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
Comment 4 Yury Semikhatsky 2011-01-24 02:00:51 PST
Created attachment 79902 [details]
Patch
Comment 5 WebKit Review Bot 2011-01-24 02:04:22 PST
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.
Comment 6 Yury Semikhatsky 2011-01-24 02:23:23 PST
Committed r76493: <http://trac.webkit.org/changeset/76493>
Comment 7 Yury Semikhatsky 2011-01-24 02:24:04 PST
(In reply to comment #6)
> Committed r76493: <http://trac.webkit.org/changeset/76493>

Style errors were fixed before landing.
Comment 8 Alexey Proskuryakov 2011-01-24 08:39:27 PST
-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?
Comment 9 Yury Semikhatsky 2011-01-25 02:28:13 PST
(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.
Comment 10 Alexey Proskuryakov 2011-01-25 09:05:25 PST
> 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.