Bug 174495

Summary: [WTF] Use std::unique_ptr for StackTrace
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, benjamin, buildbot, cdumez, cmarcelo, commit-queue, darin, dbates, keith_miller, mark.lam, msaboff, saam, ticaiolima
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Yusuke Suzuki 2017-07-13 23:26:48 PDT
[WTF] Use std::unique_ptr for StackTrace
Comment 1 Yusuke Suzuki 2017-07-13 23:30:38 PDT
Created attachment 315405 [details]
Patch
Comment 2 WebKit Commit Bot 2017-07-14 09:03:51 PDT
Comment on attachment 315405 [details]
Patch

Clearing flags on attachment: 315405

Committed r219506: <http://trac.webkit.org/changeset/219506>
Comment 3 WebKit Commit Bot 2017-07-14 09:03:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 4 Mark Lam 2017-07-14 09:42:09 PDT
Caio had brought up this issue before, and I told him at the time that I original chose to have captureStackTrace() return a raw pointer instead of a std::unique_ptr for a reason.

I looked and found the discussion I was thinking of: https://bugs.webkit.org/show_bug.cgi?id=169454#c7.  Michael had suggested using RefPtr which I think is not needed.  A std::unique_ptr is indeed the right solution.  The only reason that captureStackTrace() didn't return a std::unique_ptr instead of a raw pointer was because I didn't think to do so at the time and was writing bad C++ code.

Thanks for the fix.
Comment 5 Saam Barati 2017-07-14 10:03:40 PDT
Comment on attachment 315405 [details]
Patch

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

> Source/WTF/wtf/StackTrace.cpp:69
> +    std::unique_ptr<StackTrace> trace(new (NotNull, fastMalloc(sizeToAllocate)) StackTrace());

How does the fastMalloc memory here get freed when the unique_ptr’s destructor is run? Maybe I’m missing something?
Comment 6 Mark Lam 2017-07-14 10:07:32 PDT
Comment on attachment 315405 [details]
Patch

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

>> Source/WTF/wtf/StackTrace.cpp:69
>> +    std::unique_ptr<StackTrace> trace(new (NotNull, fastMalloc(sizeToAllocate)) StackTrace());
> 
> How does the fastMalloc memory here get freed when the unique_ptr’s destructor is run? Maybe I’m missing something?

Because class StackTrace is WTF_MAKE_FAST_ALLOCATED.
Comment 7 Mark Lam 2017-07-14 10:08:48 PDT
(In reply to Mark Lam from comment #6)
> Comment on attachment 315405 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315405&action=review
> 
> >> Source/WTF/wtf/StackTrace.cpp:69
> >> +    std::unique_ptr<StackTrace> trace(new (NotNull, fastMalloc(sizeToAllocate)) StackTrace());
> > 
> > How does the fastMalloc memory here get freed when the unique_ptr’s destructor is run? Maybe I’m missing something?
> 
> Because class StackTrace is WTF_MAKE_FAST_ALLOCATED.

In case you missed it, the purpose of the explicit call to fastMalloc() is not to choose fastMalloc vs system malloc, but to customize the allocation size.
Comment 8 Caio Lima 2017-07-15 07:39:10 PDT
(In reply to Mark Lam from comment #4)
> Caio had brought up this issue before, and I told him at the time that I
> original chose to have captureStackTrace() return a raw pointer instead of a
> std::unique_ptr for a reason.
> 
> I looked and found the discussion I was thinking of:
> https://bugs.webkit.org/show_bug.cgi?id=169454#c7.  Michael had suggested
> using RefPtr which I think is not needed.  A std::unique_ptr is indeed the
> right solution.  The only reason that captureStackTrace() didn't return a
> std::unique_ptr instead of a raw pointer was because I didn't think to do so
> at the time and was writing bad C++ code.
> 
> Thanks for the fix.

It was Yusuke's suggestion BTW.
Comment 9 Darin Adler 2017-07-15 09:58:51 PDT
Comment on attachment 315405 [details]
Patch

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

> Source/WTF/ChangeLog:12
> +        And we also move WTFGetBackTrace from Assertions.cpp to
> +        StackTrace.cpp to consolidate stack trace related operations
> +        into StackTrace.cpp.

OK, but it’s not great that this function is declared in Assertions.h but defined in StackTrace.cpp.
Comment 10 Yusuke Suzuki 2017-07-16 07:39:22 PDT
(In reply to Darin Adler from comment #9)
> Comment on attachment 315405 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315405&action=review
> 
> > Source/WTF/ChangeLog:12
> > +        And we also move WTFGetBackTrace from Assertions.cpp to
> > +        StackTrace.cpp to consolidate stack trace related operations
> > +        into StackTrace.cpp.
> 
> OK, but it’s not great that this function is declared in Assertions.h but
> defined in StackTrace.cpp.

Let's remove WTFGetBacktrace and WTFPrintBacktrace!
And move these functionalities to StackTrace.
Comment 11 Yusuke Suzuki 2017-07-16 08:25:37 PDT
(In reply to Yusuke Suzuki from comment #10)
> (In reply to Darin Adler from comment #9)
> > Comment on attachment 315405 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=315405&action=review
> > 
> > > Source/WTF/ChangeLog:12
> > > +        And we also move WTFGetBackTrace from Assertions.cpp to
> > > +        StackTrace.cpp to consolidate stack trace related operations
> > > +        into StackTrace.cpp.
> > 
> > OK, but it’s not great that this function is declared in Assertions.h but
> > defined in StackTrace.cpp.
> 
> Let's remove WTFGetBacktrace and WTFPrintBacktrace!
> And move these functionalities to StackTrace.

https://bugs.webkit.org/show_bug.cgi?id=174566