New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EXC_BAD_ACCESS Random crashes on iOS context presentRenderbuffer #15439

Closed
Obg1 opened this Issue Apr 16, 2016 · 30 comments

Comments

Projects
None yet
@Obg1

Obg1 commented Apr 16, 2016

I'm getting random crashes at:

if(![context_ presentRenderbuffer:GL_RENDERBUFFER])
File: CCEAGLView-ios.mm

EXC_BAD_ACCESS (code=1, address 0x1)

OK, I was finally able to identify the cause for it.
The bug is because of the new static __indices in QuadCommand:

void QuadCommand::reIndex(int indicesCount)
{
    if (indicesCount > __indexCapacity)
    {
        CCLOG("cocos2d: QuadCommand: resizing index size from [%d] to [%d]", __indexCapacity, indicesCount);
        __indices = (GLushort*) realloc(__indices, indicesCount * sizeof(__indices[0]));
        __indexCapacity = indicesCount;
    }

When calling realloc, it moves the memory that is being used by OpenGl, thus causing the BAD ACCESS error.
I was able to "Fix" it by calling realloc with null (will work like malloc):

__indices = (GLushort*) realloc(nullptr, indicesCount * sizeof(__indices[0]));

@felixjingga

This comment has been minimized.

Show comment
Hide comment
@felixjingga

felixjingga Apr 17, 2016

Contributor

the more important question is, how do we replicate this bug reliably..? I got this bug too, and can't ever seem to replicate it reliably.. is there any simple way to do so?

Contributor

felixjingga commented Apr 17, 2016

the more important question is, how do we replicate this bug reliably..? I got this bug too, and can't ever seem to replicate it reliably.. is there any simple way to do so?

@Obg1

This comment has been minimized.

Show comment
Hide comment
@Obg1

Obg1 Apr 19, 2016

I think this bug is related to threading so it is not easy to reproduce it.
But by looking at the code your can notice the realloc function is deleting a pointer that is still being use by previous render commands.

Obg1 commented Apr 19, 2016

I think this bug is related to threading so it is not easy to reproduce it.
But by looking at the code your can notice the realloc function is deleting a pointer that is still being use by previous render commands.

@minggo

This comment has been minimized.

Show comment
Hide comment
@minggo

minggo Apr 26, 2016

Contributor

When calling realloc, it moves the memory that is being used by OpenGl, thus causing the BAD ACCESS error.

@Obg1 Good catch. But i am not sure if it is caused by the codes. How did you make sure? But i think we should use malloc here since we don't care about the value of __indices.

@ricardoquesada Did you know why use realloc here?

Contributor

minggo commented Apr 26, 2016

When calling realloc, it moves the memory that is being used by OpenGl, thus causing the BAD ACCESS error.

@Obg1 Good catch. But i am not sure if it is caused by the codes. How did you make sure? But i think we should use malloc here since we don't care about the value of __indices.

@ricardoquesada Did you know why use realloc here?

@minggo minggo self-assigned this Apr 26, 2016

@minggo minggo added the type:bug label Apr 26, 2016

@minggo

This comment has been minimized.

Show comment
Hide comment
@minggo

minggo Apr 26, 2016

Contributor

And i think it is a potential issue here as @Obg1 mentioned.

Contributor

minggo commented Apr 26, 2016

And i think it is a potential issue here as @Obg1 mentioned.

@Obg1

This comment has been minimized.

Show comment
Hide comment
@Obg1

Obg1 Apr 26, 2016

@minggo my temp fix for it was when allocating a new __indices
to save the previous allocated __indices without releasing it, it is only release on the next "reIndex" call.
This is not 100% fix, but it did stop the crash from happening. Crash only appears on iOS, but it may cause unknown errors on other platform, it all depends on what the system does with the free memory.

realloc = causes the memory address to be changed, thus causing the pointers to be invalid.

Obg1 commented Apr 26, 2016

@minggo my temp fix for it was when allocating a new __indices
to save the previous allocated __indices without releasing it, it is only release on the next "reIndex" call.
This is not 100% fix, but it did stop the crash from happening. Crash only appears on iOS, but it may cause unknown errors on other platform, it all depends on what the system does with the free memory.

realloc = causes the memory address to be changed, thus causing the pointers to be invalid.

@minggo

This comment has been minimized.

Show comment
Hide comment
@minggo

minggo Apr 27, 2016

Contributor

@Obg1 I think it is better to release previous __indices is in next frame, for example

void QuadCommand::reIndex(int indicesCount)
{
    if (indicesCount > __indexCapacity)
    {
        CCLOG("cocos2d: QuadCommand: resizing index size from [%d] to [%d]", __indexCapacity, indicesCount);
        tempIndecs = __indices
        __indices = (GLushort*) malloc( indicesCount * sizeof(__indices[0]));
        __indexCapacity = indicesCount;
    }
   ...
}

free tempIndecs in next frame
Contributor

minggo commented Apr 27, 2016

@Obg1 I think it is better to release previous __indices is in next frame, for example

void QuadCommand::reIndex(int indicesCount)
{
    if (indicesCount > __indexCapacity)
    {
        CCLOG("cocos2d: QuadCommand: resizing index size from [%d] to [%d]", __indexCapacity, indicesCount);
        tempIndecs = __indices
        __indices = (GLushort*) malloc( indicesCount * sizeof(__indices[0]));
        __indexCapacity = indicesCount;
    }
   ...
}

free tempIndecs in next frame
@minggo

This comment has been minimized.

Show comment
Hide comment
@minggo

minggo Apr 27, 2016

Contributor

After reading codes more carefully, i think the reason is that

  1. realloc modified __indices that used in previous triangles
  2. renderer will copy indices in Renderer::fillVerticesAndIndices(), because the __indices contents is changed so it may copy wrong value into Renderer::_indices
  3. OpenGL get wrong indices which causes the problem

In fact, there may be problem in step 2 too.

@ricardoquesada Please take a look.

Contributor

minggo commented Apr 27, 2016

After reading codes more carefully, i think the reason is that

  1. realloc modified __indices that used in previous triangles
  2. renderer will copy indices in Renderer::fillVerticesAndIndices(), because the __indices contents is changed so it may copy wrong value into Renderer::_indices
  3. OpenGL get wrong indices which causes the problem

In fact, there may be problem in step 2 too.

@ricardoquesada Please take a look.

@minggo minggo added the critical label Apr 27, 2016

@Obg1

This comment has been minimized.

Show comment
Hide comment
@Obg1

Obg1 Apr 27, 2016

@minggo we also need to check what happens if QuadCommand::reIndex is called more than once on one frame. If that happens having one "tmp" pointer will no be enough, so we need to save them on a list or something and than release them on the next frame.

Obg1 commented Apr 27, 2016

@minggo we also need to check what happens if QuadCommand::reIndex is called more than once on one frame. If that happens having one "tmp" pointer will no be enough, so we need to save them on a list or something and than release them on the next frame.

@minggo

This comment has been minimized.

Show comment
Hide comment
@minggo

minggo Apr 27, 2016

Contributor

@Obg1 Sorry, i didn't express it clearly. We can listen the event of Director::EVENT_AFTER_DRAW sent after invoking renderer(), and in the call back do it like

auto tmpindices = __indeces;
Director::getInstance()->getEventDispatcher()->addCustomEventListener(Director::EVENT_AFTER_DRAW, [](EventCustom *event) {
        free tmpindices;
        unregister event listener
    });

__indices = (GLushort*) malloc( indicesCount * sizeof(__indices[0]));

Then no matter how many times reIndex() is invoked, there is not problem.

Of course there may be other method, such as not share indices for all quad commands.

Contributor

minggo commented Apr 27, 2016

@Obg1 Sorry, i didn't express it clearly. We can listen the event of Director::EVENT_AFTER_DRAW sent after invoking renderer(), and in the call back do it like

auto tmpindices = __indeces;
Director::getInstance()->getEventDispatcher()->addCustomEventListener(Director::EVENT_AFTER_DRAW, [](EventCustom *event) {
        free tmpindices;
        unregister event listener
    });

__indices = (GLushort*) malloc( indicesCount * sizeof(__indices[0]));

Then no matter how many times reIndex() is invoked, there is not problem.

Of course there may be other method, such as not share indices for all quad commands.

@Obg1

This comment has been minimized.

Show comment
Hide comment
@Obg1

Obg1 May 7, 2016

@minggo , based on your suggestion I've made the following change to my code and it works well:

void QuadCommand::reIndex(int indicesCount)
{
    if (indicesCount > __indexCapacity)
    {
        CCLOG("cocos2d: QuadCommand: resizing index size from [%d] to [%d]", __indexCapacity, indicesCount);
        //__indices = (GLushort*) realloc(__indices, indicesCount * sizeof(__indices[0]));
        // TODO - Oren Hack fix for this bug - Will cause some leak but won't crash
        auto tmpindices = __indices;
        __indices = (GLushort*)malloc(indicesCount * sizeof(__indices[0]));

        void** listenerHolder = new void*();

        EventListenerCustom* listener = cocos2d::Director::getInstance()->getEventDispatcher()->addCustomEventListener(cocos2d::Director::EVENT_AFTER_DRAW, [=](cocos2d::EventCustom *event) {

            if (tmpindices)
            {
                free(tmpindices);
            }

            // unregister event listener
            cocos2d::Director::getInstance()->getEventDispatcher()->removeEventListener((EventListener*)*listenerHolder);

            if (listenerHolder)
            {
                delete listenerHolder;
            }

        });

        *listenerHolder = listener;

        __indexCapacity = indicesCount;
    }

    for( int i=0; i < __indexCapacity/6; i++)
    {
        __indices[i*6+0] = (GLushort) (i*4+0);
        __indices[i*6+1] = (GLushort) (i*4+1);
        __indices[i*6+2] = (GLushort) (i*4+2);
        __indices[i*6+3] = (GLushort) (i*4+3);
        __indices[i*6+4] = (GLushort) (i*4+2);
        __indices[i*6+5] = (GLushort) (i*4+1);
    }

    _indexSize = indicesCount;
}

Thanks

Obg1 commented May 7, 2016

@minggo , based on your suggestion I've made the following change to my code and it works well:

void QuadCommand::reIndex(int indicesCount)
{
    if (indicesCount > __indexCapacity)
    {
        CCLOG("cocos2d: QuadCommand: resizing index size from [%d] to [%d]", __indexCapacity, indicesCount);
        //__indices = (GLushort*) realloc(__indices, indicesCount * sizeof(__indices[0]));
        // TODO - Oren Hack fix for this bug - Will cause some leak but won't crash
        auto tmpindices = __indices;
        __indices = (GLushort*)malloc(indicesCount * sizeof(__indices[0]));

        void** listenerHolder = new void*();

        EventListenerCustom* listener = cocos2d::Director::getInstance()->getEventDispatcher()->addCustomEventListener(cocos2d::Director::EVENT_AFTER_DRAW, [=](cocos2d::EventCustom *event) {

            if (tmpindices)
            {
                free(tmpindices);
            }

            // unregister event listener
            cocos2d::Director::getInstance()->getEventDispatcher()->removeEventListener((EventListener*)*listenerHolder);

            if (listenerHolder)
            {
                delete listenerHolder;
            }

        });

        *listenerHolder = listener;

        __indexCapacity = indicesCount;
    }

    for( int i=0; i < __indexCapacity/6; i++)
    {
        __indices[i*6+0] = (GLushort) (i*4+0);
        __indices[i*6+1] = (GLushort) (i*4+1);
        __indices[i*6+2] = (GLushort) (i*4+2);
        __indices[i*6+3] = (GLushort) (i*4+3);
        __indices[i*6+4] = (GLushort) (i*4+2);
        __indices[i*6+5] = (GLushort) (i*4+1);
    }

    _indexSize = indicesCount;
}

Thanks

@minggo

This comment has been minimized.

Show comment
Hide comment
@minggo

minggo May 7, 2016

Contributor

@Obg1 thanks. Let's listen @ricardoquesada 's idea too.

Contributor

minggo commented May 7, 2016

@Obg1 thanks. Let's listen @ricardoquesada 's idea too.

@ricardoquesada

This comment has been minimized.

Show comment
Hide comment
@ricardoquesada

ricardoquesada May 9, 2016

Contributor

I'll take a look at it a few days.

Contributor

ricardoquesada commented May 9, 2016

I'll take a look at it a few days.

@drakon-ag

This comment has been minimized.

Show comment
Hide comment
@drakon-ag

drakon-ag May 13, 2016

Contributor

If it can be useful my game crashes the 90% of times when a moving Particle Effect enters in the Scene.
I think this can be used to reproduce the crash for further inspections.

Bye

Contributor

drakon-ag commented May 13, 2016

If it can be useful my game crashes the 90% of times when a moving Particle Effect enters in the Scene.
I think this can be used to reproduce the crash for further inspections.

Bye

@whitegfx

This comment has been minimized.

Show comment
Hide comment
@whitegfx

whitegfx May 13, 2016

Contributor

Hi @minggo @ricardoquesada @Obg1

  • today after update from v. 3.10 to 3.11. (Xcode 7.3.1)
  • Our project have random crashes When I initiated ParticleSystemQuad.
  • (one system with little coins, max items 200), PSQ started, shown, crash with
    cocos2d: QuadCommand: resizing index size from..
  • crashed in 80% on iPad A1395, 20% on iPhone6.
  • tested many times Obg1 fix and no crash.

Any other solution/test for this issue?
Thx

Contributor

whitegfx commented May 13, 2016

Hi @minggo @ricardoquesada @Obg1

  • today after update from v. 3.10 to 3.11. (Xcode 7.3.1)
  • Our project have random crashes When I initiated ParticleSystemQuad.
  • (one system with little coins, max items 200), PSQ started, shown, crash with
    cocos2d: QuadCommand: resizing index size from..
  • crashed in 80% on iPad A1395, 20% on iPhone6.
  • tested many times Obg1 fix and no crash.

Any other solution/test for this issue?
Thx

@Rypac

This comment has been minimized.

Show comment
Hide comment
@Rypac

Rypac May 15, 2016

Contributor

I've submitted a PR to fix the issue.

The EventDispatcher option seems kind of heavy handed and I don't feel it belongs in the QuadCommand.

In my proposal whenever __indices[] needs to expand, the QuadCommand becomes the owner of the current shared memory buffer for the duration of its lifecycle (as many times as required). This should prevent the Renderer from gaining access to any invalid Triangles indices when copying to the GL buffer.

Feedback and alternatives are very welcome!

Contributor

Rypac commented May 15, 2016

I've submitted a PR to fix the issue.

The EventDispatcher option seems kind of heavy handed and I don't feel it belongs in the QuadCommand.

In my proposal whenever __indices[] needs to expand, the QuadCommand becomes the owner of the current shared memory buffer for the duration of its lifecycle (as many times as required). This should prevent the Renderer from gaining access to any invalid Triangles indices when copying to the GL buffer.

Feedback and alternatives are very welcome!

@marah-sh

This comment has been minimized.

Show comment
Hide comment
@marah-sh

marah-sh May 15, 2016

How can I apply this solution on Cocod2dx 2.2.6??

marah-sh commented May 15, 2016

How can I apply this solution on Cocod2dx 2.2.6??

@whitegfx

This comment has been minimized.

Show comment
Hide comment
@whitegfx

whitegfx May 15, 2016

Contributor

@marah-sh 2.2.6 doesn't have this issue.

Contributor

whitegfx commented May 15, 2016

@marah-sh 2.2.6 doesn't have this issue.

@marah-sh

This comment has been minimized.

Show comment
Hide comment
@marah-sh

marah-sh May 15, 2016

@whitegfx
I'm facing random crashes in EAGLView.mm swapBuffers method at the same line
if(![_context presentRenderbuffer:GL_RENDERBUFFER])

marah-sh commented May 15, 2016

@whitegfx
I'm facing random crashes in EAGLView.mm swapBuffers method at the same line
if(![_context presentRenderbuffer:GL_RENDERBUFFER])

@whitegfx

This comment has been minimized.

Show comment
Hide comment
@whitegfx

whitegfx May 15, 2016

Contributor

@Rypac #15646 tested on iPad A1395 with iOS 8.3 and iPhone6 iOS 9.3.1, no crash Thx.

Contributor

whitegfx commented May 15, 2016

@Rypac #15646 tested on iPad A1395 with iOS 8.3 and iPhone6 iOS 9.3.1, no crash Thx.

@minggo

This comment has been minimized.

Show comment
Hide comment
@minggo

minggo May 16, 2016

Contributor

@marah-sh any opengl error can cause problem in if(![_context presentRenderbuffer:GL_RENDERBUFFER]). 2.26 doesn't use render command, so it is not the same issue.

Contributor

minggo commented May 16, 2016

@marah-sh any opengl error can cause problem in if(![_context presentRenderbuffer:GL_RENDERBUFFER]). 2.26 doesn't use render command, so it is not the same issue.

@QuangBang

This comment has been minimized.

Show comment
Hide comment
@QuangBang

QuangBang May 17, 2016

Just upgrade to cocos2d-x 3.11 and the crash appear more frequently just like @whitegfx . Almost 100% on iPhone 5. I noticed that when change scene using transition fade, there are some glitch appear, after change scene 2-3 times, then the app crash.

Edit: just apply @Rypac and no crash. Thanks 👯

QuangBang commented May 17, 2016

Just upgrade to cocos2d-x 3.11 and the crash appear more frequently just like @whitegfx . Almost 100% on iPhone 5. I noticed that when change scene using transition fade, there are some glitch appear, after change scene 2-3 times, then the app crash.

Edit: just apply @Rypac and no crash. Thanks 👯

@ricardoquesada

This comment has been minimized.

Show comment
Hide comment
@ricardoquesada

ricardoquesada May 24, 2016

Contributor

I took a look at it. @Rypac 's approach is the correct one. No dependencies with the Events should be added for this fix. Merged rypac's PR

Contributor

ricardoquesada commented May 24, 2016

I took a look at it. @Rypac 's approach is the correct one. No dependencies with the Events should be added for this fix. Merged rypac's PR

@Huwell

This comment has been minimized.

Show comment
Hide comment
@Huwell

Huwell Jul 27, 2017

I met the same crash on cocos2d-x 3.15 & xcode 9beta & iOS 11
@Obg1 's codes cannot resolve it. How to fix?

Huwell commented Jul 27, 2017

I met the same crash on cocos2d-x 3.15 & xcode 9beta & iOS 11
@Obg1 's codes cannot resolve it. How to fix?

@minggo

This comment has been minimized.

Show comment
Hide comment
@minggo

minggo Jul 27, 2017

Contributor

@Huwell i think it is a different issue.

Contributor

minggo commented Jul 27, 2017

@Huwell i think it is a different issue.

@Huwell

This comment has been minimized.

Show comment
Hide comment
@Huwell

Huwell Jul 27, 2017

I have tried 3.15 & 3.15.1. You will met this crash on test example of official sdk.
(iOS 11 + xcode 9 beta + cocos2d-x 3.15.x)
screen shot 2017-07-26 at 4 15 04 pm

Huwell commented Jul 27, 2017

I have tried 3.15 & 3.15.1. You will met this crash on test example of official sdk.
(iOS 11 + xcode 9 beta + cocos2d-x 3.15.x)
screen shot 2017-07-26 at 4 15 04 pm

@faresbh

This comment has been minimized.

Show comment
Hide comment
@faresbh

faresbh Jul 27, 2017

@Huwell I had the same issue, with IOS 11 Beta 4. It'a always happening now.

faresbh commented Jul 27, 2017

@Huwell I had the same issue, with IOS 11 Beta 4. It'a always happening now.

@minggo

This comment has been minimized.

Show comment
Hide comment
@minggo

minggo Jul 28, 2017

Contributor

May be it is a bug of iOS itself, as the thread said, it is ok with iOS 11 Beta 3. I think beta version is not stable enough.

Contributor

minggo commented Jul 28, 2017

May be it is a bug of iOS itself, as the thread said, it is ok with iOS 11 Beta 3. I think beta version is not stable enough.

@matt-loflin

This comment has been minimized.

Show comment
Hide comment
@matt-loflin

matt-loflin commented Aug 4, 2017

If you are using iOS 11 Beta 4 someone posted a workaround here: https://stackoverflow.com/questions/45319215/ios-11-beta-4-presentrenderbuffer-crash/45375569#45375569

@DylanXing

This comment has been minimized.

Show comment
Hide comment
@DylanXing

DylanXing Aug 4, 2017

close 'GPU Frame Capture' works fine. thanks ,@matt-loflin

DylanXing commented Aug 4, 2017

close 'GPU Frame Capture' works fine. thanks ,@matt-loflin

@Huwell

This comment has been minimized.

Show comment
Hide comment
@Huwell

Huwell Aug 4, 2017

You save my life, @matt-loflin

Huwell commented Aug 4, 2017

You save my life, @matt-loflin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment