Bug 174495 – [WTF] Use std::unique_ptr for StackTrace

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[WTF] Use std::unique_ptr for StackTrace

[Yusuke Suzuki]

Created attachment 315405 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 315405 [details]
Patch

Clearing flags on attachment: 315405

Committed r219506: <http://trac.webkit.org/changeset/219506>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mark Lam]

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.

[Saam Barati]

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?

[Mark Lam]

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.

[Mark Lam]

(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.

[Caio Lima]

(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.

[Darin Adler]

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.

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

(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