locked
Am I supposed to delete pointer to CD2DSolidColorBrush? RRS feed

  • Question

  • In my MFC application I declare in class declaration:

    CD2DSolidColorBrush*    pBrush_{ nullptr };

    Then I initialize it in OnInitialUpdate:

    pBrush_ = new CD2DSolidColorBrush(GetRenderTarget(), D2D1::ColorF(D2D1::ColorF::YellowGreen));

    If I try to delete that pBrush object in destructor of my class I'm getting error:

    Exception thrown: read access violation.
    this->pBrush_-> was 0xDDDDDDDD.

    Any idea what is happening and how to fix that?

    Thank you

    Tuesday, January 19, 2016 10:58 AM

Answers

  • You should be using Release() (or define a SafeRelease) IF you need to release the resource.

    You can also use ComPtr.

    I am wondering if have you looked at any of the Direct2D samples or any of the documentation?

    It appears you are using the MFC wrappers for D2D. There are some very good samples on MSDN.



    • Edited by Expedition2 Tuesday, January 19, 2016 8:57 PM
    • Marked as answer by smallC795 Thursday, January 21, 2016 10:30 AM
    Tuesday, January 19, 2016 7:09 PM

All replies

  • You should always delete anything you create with new, and delete[] anything you create with new[]. It makes no difference what the type is.

    Use the debugger to check the value of pBrush when it is created. If it is a good value, perhaps you are overwriting it somehow in your code.


    David Wilkinson | Visual C++ MVP

    Tuesday, January 19, 2016 12:01 PM
  • According to documentation, the brush will be owned and destroyed by the render target, and you do not have to delete it. But if you use

       pBrush_ = new CD2DSolidColorBrush(GetRenderTarget(), D2D1::ColorF(D2D1::ColorF::YellowGreen), NULL, FALSE);

    then you can delete it, but not too early.

    Tuesday, January 19, 2016 12:03 PM
  • @Viorel_

    Hi, thank you for your answer. So basically you're saying that I do not have to delete this and there will not be any mem leak, right?

    Also you're talking about documentation which says that, would you be so good and provide link to it.

    Thank you.

    • Edited by smallC795 Tuesday, January 19, 2016 1:59 PM
    Tuesday, January 19, 2016 1:58 PM
  • @davewilk

    Hi ;)

    According to @Violet_ I don't have to delete it.

    Tuesday, January 19, 2016 1:59 PM
  • I think that if the render target is managed correctly, then it will also destroy the brush. You can simply execute ‘pbrush_ = NULL’ in your destructor.


    • Edited by Viorel_MVP Tuesday, January 19, 2016 2:03 PM
    Tuesday, January 19, 2016 2:02 PM
  • @davewilk

    Hi ;)

    According to @Violet_ I don't have to delete it.

    I think we have to be sure what we mean by delete.

    If we are talking about using C++ delete to free the memory (and run the destructor) then you must do it.

    But if we are talking about deleting (destroying) the brush then you do not need to do it, because it is done in the CD2DSolidColorBrush destructor (provided it is called!).


    David Wilkinson | Visual C++ MVP

    Tuesday, January 19, 2016 2:48 PM
  • @davewilk

    Hi

    By deleting I mean executing this statement:

    delete pBrush_;

    do I have to do it^^^? As I've mentioned, when I try to do it in destructor of my view class I'm getting read access exception.

    So I'm really confused now.

    If this works as Violet_ think it works, renderTarget simply does the deleting, and what I'm left with is simply pointer to already deleted object and I should not delete it.

    So the question is, do I have to call delete pBrush_; or not?

    Thank you

    Tuesday, January 19, 2016 2:57 PM
  • @Violet_

    Could you please provide link to the documentation you were mentioned in your earlier post?

    Thank you.

    Tuesday, January 19, 2016 2:59 PM
  • One thought:  0xDDDDDDDD is the bit pattern assigned to deallocated memory.  It sounds to me like you are double deleting the pBrush.

    Have you considered using std::unique_ptr<CD2DSolidColorBrush> instead of CD2DSolidColorBrush *?  You don't have to worry about deleting the brush in this case (and can indeed omit your destructor entirely!)

    Tuesday, January 19, 2016 3:34 PM
  • @davewilk

    Hi

    By deleting I mean executing this statement:

    delete pBrush_;

    do I have to do it^^^? As I've mentioned, when I try to do it in destructor of my view class I'm getting read access exception.

    Yes, you have to do it. But only once! If you use the pattern

    delete p;
    p = nullptr;
    

    then you will avoid double deletion errors. But personally, I regard double deletion as a programming error.

    As SimonRev suggests, if you use unique_ptr then these deletion problems will all go away. In fact, in modern C+, you should never be using new and delete (and new[] and delete[]) in your own code. You can use make_unique() to avoid explicit use of new.


    David Wilkinson | Visual C++ MVP

    Tuesday, January 19, 2016 3:59 PM
  • Hi @dave, @SimonRev,

    Thanks for your reply guys. I did try to use unique_ptr for that matter because I also thought that the problem would go away, but alas I'm getting same error, this time from different place:

    In std header <memory> :

    void operator()(_Ty *_Ptr) const _NOEXCEPT
            {    // delete a pointer
            static_assert(0 < sizeof (_Ty),
                "can't delete an incomplete type");
            delete _Ptr;//THIS POINTER HERE IS ALREADY 0XDDDDDDDD
            }

     



    Exception thrown: read access violation.

    _Ptr-> was 0xDDDDDDDD.

    I've changed my pBrush to be of type:

    std::unique_ptr<CD2DSolidColorBrush>    pBrush_{ nullptr };

    there is only one place in entire project where I create that object namely in OnInitialUpdate():

    pBrush_ = std::make_unique<CD2DSolidColorBrush>(GetRenderTarget(), D2D1::ColorF(D2D1::ColorF::YellowGreen));

    and there is only one place in that project where I use this object namely in OnDraw2D:

    pRenderTarget_->DrawText(_T("Hello, World!"), rect, pBrush_.get(), pTextFormat_);

    How is it possible that I'm getting read access violation error?



    • Edited by smallC795 Tuesday, January 19, 2016 6:14 PM
    Tuesday, January 19, 2016 6:11 PM
  • You should be using Release() (or define a SafeRelease) IF you need to release the resource.

    You can also use ComPtr.

    I am wondering if have you looked at any of the Direct2D samples or any of the documentation?

    It appears you are using the MFC wrappers for D2D. There are some very good samples on MSDN.



    • Edited by Expedition2 Tuesday, January 19, 2016 8:57 PM
    • Marked as answer by smallC795 Thursday, January 21, 2016 10:30 AM
    Tuesday, January 19, 2016 7:09 PM
  • Hi @dave, @SimonRev,

    Thanks for your reply guys. I did try to use unique_ptr for that matter because I also thought that the problem would go away, but alas I'm getting same error, this time from different place:

    In std header <memory> :

    void operator()(_Ty *_Ptr) const _NOEXCEPT
            {    // delete a pointer
            static_assert(0 < sizeof (_Ty),
                "can't delete an incomplete type");
            delete _Ptr;//THIS POINTER HERE IS ALREADY 0XDDDDDDDD
            }
    

    Exception thrown: read access violation.

    _Ptr-> was 0xDDDDDDDD.

    I've changed my pBrush to be of type:

    std::unique_ptr<CD2DSolidColorBrush>    pBrush_{ nullptr };
    

    there is only one place in entire project where I create that object namely in OnInitialUpdate():

    pBrush_ = std::make_unique<CD2DSolidColorBrush>(GetRenderTarget(), D2D1::ColorF(D2D1::ColorF::YellowGreen));
    

    and there is only one place in that project where I use this object namely in OnDraw2D:

    pRenderTarget_->DrawText(_T("Hello, World!"), rect, pBrush_.get(), pTextFormat_);
    

    How is it possible that I'm getting read access violation error?

    Hmmm, maybe this is one of these (crazy) MFC classes that does a "delete this" when the wrapped object is destroyed. I know nothing about Direct2D, but in some MFC classes this is done in the PostNcDestroy() method.

    If this is what is happening, I guess you should not delete the pointer (and therefore should not use unique_ptr).


    David Wilkinson | Visual C++ MVP

    Tuesday, January 19, 2016 9:45 PM
  • @Expedition,

    Hi, yes, indeed, I've looked at examples and documentation. Unfortunately neither in examples, nor in documentation I was able to find answer to my question, hence I've asked that here.

    If you could provide link to documentation which would answer that question I'll be very grateful.

    Thank you.

    Wednesday, January 20, 2016 7:50 AM
  • Hi David,

    Thanks for your time.

    To me it looks like this brush is automatically destroyed by the framework (but where and when I cannot figure out for the life of me).

    Would be good to know for sure though.

    Wednesday, January 20, 2016 7:51 AM
  • Hi, thank you very much for your answer. After reading Prosies' book I've stumbled upon: 

    "COM differs from C++ in this respect, because clients create object instances but they don't delete them. Instead, COM objects delete themselves."

    That solves the mystery for me.

    Thanks again

    Thursday, January 21, 2016 10:30 AM
  • Hi, thank you very much for your answer. After reading Prosies' book I've stumbled upon: 

    "COM differs from C++ in this respect, because clients create object instances but they don't delete them. Instead, COM objects delete themselves."

    Maybe so, but COM objects are normally created using some kind of factory function, not by using "new" in your own code. This factory function calls "new" inside the COM DLL, and the corresponding "delete" also runs inside the DLL.

    But I know nothing about these Direct2D classes, so maybe they work differently.


    David Wilkinson | Visual C++ MVP

    Thursday, January 21, 2016 2:36 PM
  • Hello,

    A couple of pointers (this is for the MFC Direct2D wrappers).

    If you temporarily create a resource, release it with Release() when you have finished.

    For example:

    pD2DBrush = new CD2DSolidColorBrush(pRenderTarget, D2D1::ColorF(D2D1::ColorF::Black));

    ...

    pD2DBrush->Release();

    Global resources may be different. If an EndDraw() function returns D2DERR_RECREATE_TARGET, you need to recreate all resources.

    However, since it appears you are using the MFC framework, you may not be having to call BeginDraw() and EndDraw(). In this case, this functionality is handled by the AFX_WM_RECREATED2DRESOURCES message.



    • Edited by Expedition2 Thursday, January 21, 2016 7:27 PM
    Thursday, January 21, 2016 7:26 PM
  • @Expedition

    Yes, I do use strictly MFC framework and with regards to that framework I am/was asking this question. I believe it is correct to say that renderTarget should create any resource and client shouldn't do anything about deletion of it.

    Friday, January 22, 2016 8:30 AM
  • @davewilk, that right, and that's why I state that renderTarget is responsible for creation and deletion of those resources. Client is supposed to only use them. Which is in my opinion fantastic approach.

    Seriously, I really don't understand why people are slagging off MFC. It is really solid framework.

    Friday, January 22, 2016 8:32 AM
  • Could you please provide link to the documentation you were mentioned in your earlier post?

    Here is the documentation: https://msdn.microsoft.com/en-us/library/gg466833(v=vs.140).aspx

    Therefore, if you do not specify the bAutoDestroy parameter, or specify TRUE, then you do not have to delete the brush, since it will be freed by the render target (owner), which is convenient. Otherwise you must free it with delete.

    Friday, January 22, 2016 8:57 AM
  • @Violel_

    Thank you for that link.

    Best Regards

    Friday, January 22, 2016 9:40 AM
  • @Violel_

    Thank you for that link.

    Best Regards

    I had looked at this documentation, but had failed to notice the bAutoDestroy parameter, which is defaulted to TRUE.

    I still use MFC, but I do not like these auto-deletion features. Modern C++ has much more robust ways to control object lifetime.

    When used with the default bAutoDestroy value (TRUE) these Direct2D classes will create run-time errors, both when used on the stack and when placed in a smart pointer. A class should not behave this way.


    David Wilkinson | Visual C++ MVP

    Friday, January 22, 2016 11:41 AM