Summary: | REGRESSION (r72895): console.trace crashes | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexey Proskuryakov <ap> | ||||
Component: | WebCore JavaScript | Assignee: | 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
Alexey Proskuryakov
2011-01-23 12:31:53 PST
Please see <http://webkit.org/coding/RefPtr.html> for information on using RefPtr and PassRefPtr. (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 Created attachment 79902 [details]
Patch
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.
Committed r76493: <http://trac.webkit.org/changeset/76493> (In reply to comment #6) > Committed r76493: <http://trac.webkit.org/changeset/76493> Style errors were fixed before landing. -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?
(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. > 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.
|