Skip to content
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

RCTImageLoader crashes when canceling image loads (RN 0.36) #10433

Closed
SunnyGurnani opened this issue Oct 18, 2016 · 106 comments
Closed

RCTImageLoader crashes when canceling image loads (RN 0.36) #10433

SunnyGurnani opened this issue Oct 18, 2016 · 106 comments
Labels
Platform: iOS iOS applications. Resolution: Locked This issue was locked by the bot.

Comments

@SunnyGurnani
Copy link
Contributor

SunnyGurnani commented Oct 18, 2016

Image Loader Crashing at OSAtomicOr32Barrier (EXC_BAD_ACCESS)

Start of Stack is

- (void)didMoveToWindow
{
  [super didMoveToWindow];

  if (!self.window) {
    // Cancel loading the image if we've moved offscreen. In addition to helping
    // prioritise image requests that are actually on-screen, this removes
    // requests that have gotten "stuck" from the queue, unblocking other images
    // from loading.
    **[self cancelImageLoad];**
  } else if ([self shouldChangeImageSource]) {
    [self reloadImage];
  }
}

image

image

Steps to Reproduce / Code Snippets

Put some Network Images on the Page (Specially in a List View) then scroll or change tabs so Images which are not loaded yet are out of the View Port and go back to same tab/viewport and you will see this error.

Expected Results

No Error

Additional Information

  • React Native version: Latest 0.36.rc.0
  • Platform(s) (iOS, Android, or both?): iOS
  • Operating System (macOS, Linux, or Windows?): macOS
@hramos
Copy link
Contributor

hramos commented Oct 18, 2016

Thanks for submitting an issue. Can you update your original post and ensure all the fields required by the template are filled in?

@SunnyGurnani SunnyGurnani changed the title RCTImageLoader Crash RN-0.36.01 Beta RCTImageLoader Crash 0.30.rc.0 Oct 19, 2016
@SunnyGurnani
Copy link
Contributor Author

@hramos update the issue. Let me know if you need any more information.

@anttimo
Copy link
Contributor

anttimo commented Oct 20, 2016

@SunnyGurnani you probably mean to have 0.36.rc.0 in the title instead of 0.30.rc.0?

@SunnyGurnani SunnyGurnani changed the title RCTImageLoader Crash 0.30.rc.0 RCTImageLoader Crash 0.36.rc.0 Oct 20, 2016
@SunnyGurnani
Copy link
Contributor Author

@anttimo thats correct I have changed the title

@SunnyGurnani
Copy link
Contributor Author

This is also similar #10473

This was referenced Oct 24, 2016
@ide ide changed the title RCTImageLoader Crash 0.36.rc.0 RCTImageLoader crashes when canceling image loads (RN 0.36) Oct 26, 2016
@chandlervdw
Copy link

This issue has us still using 0.32. I'm not sure why this hasn't become a more pressing issue to fix.

@pietropizzi
Copy link

Same as @chandlervdw here. We were constantly updating to the newest version to try if it is fixed. Now that we need to release an update to our app, we downgraded back to React Native 0.31 (for us 0.32 crashes as well with a related network issue) to be able to release an update. That was a sad moment since we also had to downgrade a couple of other libraries.

@mrspeaker
Copy link
Contributor

mrspeaker commented Oct 27, 2016

Same boat - this is a huge issue for us, getting hundreds of crash reports each week from it. I was realllly hoping not to have to got back to 0.31 because there's so many other good bug fixes in there - so I come here daily hoping for a miracle patch ;)

@seanadkinson
Copy link

Similar story here. We upgraded to 0.33 awhile back, and found this issue during testing. I assumed it would be addressed within the month or two before we were ready to release, but now that date it quickly approaching, and I really don't want to downgrade.

Can this issue not be patched somehow, even something dirty? The entire app crashing is really not good... even if the images just didn't load, or didn't get cleaned up would be better.

I'll have to learn some more objective-c to see how I can help. Maybe there's someone who can do a quick technical writeup for why this is actually happening. For example, what is trying to be achieved with the line: OSAtomicOr32Barrier(1, &cancelled). Which part is the "BAD_ACCESS"? I assume the issue is the &cancelled pointer is no longer valid, but how or why does that happen? I'd love to help, but this language isn't my strong suit.

@hramos hramos added the Platform: iOS iOS applications. label Oct 27, 2016
@hramos
Copy link
Contributor

hramos commented Oct 27, 2016

Is there a rnplay snippet that reproduces the crash?

@gitim
Copy link
Contributor

gitim commented Oct 28, 2016

@hramos I created an app to reproduce the bug https://github.com/gitim/ListViewApp. To reproduce you need scroll intensively, sometimes the bug happens quickly, sometimes you need to scroll more, I think it depends on your network connection.

@Aaang
Copy link

Aaang commented Oct 31, 2016

I can also confirm that this is still crashing, even though the crash rate got much better after upgrading to version 0.36.0.rc.1. We upgraded our production app from 0.28.0 to 0.34.0 two weeks ago. After that, we experienced an enormous increase of crashes, so we decided to release a new app update with 0.36.rc.1 as soon as possible (hence the release candidate and not the stable version) as there were two promising commits in 0.35.0. and 0.36.0,rc.1.

After evaluating the crash rate of this crash after a week on production, the crash rate was reduced by 88%, so it really helped a lot. But still, a proper fix would be amazing.

@gitim
Copy link
Contributor

gitim commented Oct 31, 2016

Did anybody try to disable removeClippedSubviews of ListView, does it reduce crashes?

@SunnyGurnani
Copy link
Contributor Author

@gitim yes it does.

I also found that if the server has error or Not found for any image url in the list it increases these crashes even when removeClippedSubviews is disabled.

So I would suggest to set one of the URL which responds as 4xx or 5xx

@gitim
Copy link
Contributor

gitim commented Nov 3, 2016

I created a product pain. Please, vote to pay attention of the core team members to the bug.

@SunnyGurnani
Copy link
Contributor Author

@gitim voted.

@javache
Copy link
Member

javache commented Nov 7, 2016

Thanks @gitim, I'm trying to reproduce with your app right now. With Xcode's memory analyzer I can see a number of zombie NSCFURLSessionTask but I can't reliably repro the crash.

@gitim
Copy link
Contributor

gitim commented Nov 7, 2016

I didn't find reliable way to reproduce, but it is definitely reproducible in this app. After about 3 minutes of attempts it crashed http://www.giphy.com/gifs/l0MYw9QKCyg2ESvsI.

@javache
Copy link
Member

javache commented Nov 7, 2016

@gitim: one theory I'm trying to verify now. Could you try removing the [_delegates removeObjectForKey:task]; in -[RCTHTTPRequestHandler cancel]? Since the task is still running at that point (and will still call the completion callback), this may lead to an early release.

The other thing I'm investigating is that when running your app in Instruments, it looks like all of the __NSCFLocalDataTask are actually being leaked.

@njafei
Copy link

njafei commented Dec 30, 2016

still crash in 0.39 :(

@gitim
Copy link
Contributor

gitim commented Dec 30, 2016

@njafei as far as I know the last commit from @rigdern will be released with version 0.40

@aterribili
Copy link

aterribili commented Jan 4, 2017

It's still crash on 0.38 and 0.39

@brenordr
Copy link

brenordr commented Jan 4, 2017

@aterribili Last fixes goes in 0.40 or 0.41, 0.39 and earlier will have the issue.

DanielMSchmidt pushed a commit to DanielMSchmidt/react-native that referenced this issue Jan 4, 2017
Summary:
Fixes facebook#10433

The code didn't account for the fact that cancelLoad is set by two different threads. It gets set on the URL request queue when the request completes successfully and it gets set on the UI queue during cancelation. This oversight lead to a couple of different kinds of crashes.

1. Attempt to invoke a nil function -- We check that cancelLoad is non-nil and on the next line we call it. However, cancelLoad could have been set to nil by the other thread between the time it was verified to be non-nil and the time it was called. Consequently, the program will attempt to call nil and crash.

2. Block deallocated while it's executing -- Suppose cancelLoad points to a block, it is verified to be non-nil, and it is successfully invoked. In the middle of executing the block, cancelLoad, the last reference to the block, is set to nil. This causes the block to be deallocated and its captured values to be freed. However, the block continues executing and the next time it attempts to use a captured value
Closes facebook#11145

Differential Revision: D4261499

Pulled By: javache

fbshipit-source-id: 46424c6dd8cfa085cef32d945308de07798040bc
DanielMSchmidt pushed a commit to DanielMSchmidt/react-native that referenced this issue Jan 4, 2017
Summary:
This fixes a crash occurring [on this line](https://github.com/facebook/react-native/blob/f9f32eb426b5385e199ef6e1d2b610dfe20e60e7/Libraries/Image/RCTImageLoader.m#L253). It was reported in a comment in facebook#10433.

The problem is that `task` is deallocated at this point and is unsafe to use. Removing it from `_pendingTasks` dropped its ref count to 0. [The ARC docs](http://clang.llvm.org/docs/AutomaticReferenceCounting.html#fast-enumeration-iteration-variables) state that, by default, loop variables in fast enumeration loops are not retained. That's why `task`'s ref count is 0.

It's likely we ran into this bug because the code disobeyed the [reverseObjectEnumerator docs](https://developer.apple.com/reference/foundation/nsarray/1415734-reverseobjectenumerator) which state that "you must not modify the array during enumeration". The default retention policy for fast enumeration seems to assume you follow this.

To fix this bug and avoid other potential pitfalls, the code now follows the docs and `_pendingTa
Closes facebook#11296

Differential Revision: D4277167

Pulled By: javache

fbshipit-source-id: 1211b32935bab7f4080dc00b88d85348786e859a
@ide
Copy link
Contributor

ide commented Jan 4, 2017

The fixes were merged before 0.40. My understanding is that there are still some crashes but they should be a lot less frequent.

liskiew pushed a commit to goodylabs/react-native that referenced this issue Mar 16, 2017
Summary:
Fixes facebook#10433

The code didn't account for the fact that cancelLoad is set by two different threads. It gets set on the URL request queue when the request completes successfully and it gets set on the UI queue during cancelation. This oversight lead to a couple of different kinds of crashes.

1. Attempt to invoke a nil function -- We check that cancelLoad is non-nil and on the next line we call it. However, cancelLoad could have been set to nil by the other thread between the time it was verified to be non-nil and the time it was called. Consequently, the program will attempt to call nil and crash.

2. Block deallocated while it's executing -- Suppose cancelLoad points to a block, it is verified to be non-nil, and it is successfully invoked. In the middle of executing the block, cancelLoad, the last reference to the block, is set to nil. This causes the block to be deallocated and its captured values to be freed. However, the block continues executing and the next time it attempts to use a captured value
Closes facebook#11145

Differential Revision: D4261499

Pulled By: javache

fbshipit-source-id: 46424c6dd8cfa085cef32d945308de07798040bc
liskiew pushed a commit to goodylabs/react-native that referenced this issue Mar 17, 2017
Summary:
Fixes facebook#10433

The code didn't account for the fact that cancelLoad is set by two different threads. It gets set on the URL request queue when the request completes successfully and it gets set on the UI queue during cancelation. This oversight lead to a couple of different kinds of crashes.

1. Attempt to invoke a nil function -- We check that cancelLoad is non-nil and on the next line we call it. However, cancelLoad could have been set to nil by the other thread between the time it was verified to be non-nil and the time it was called. Consequently, the program will attempt to call nil and crash.

2. Block deallocated while it's executing -- Suppose cancelLoad points to a block, it is verified to be non-nil, and it is successfully invoked. In the middle of executing the block, cancelLoad, the last reference to the block, is set to nil. This causes the block to be deallocated and its captured values to be freed. However, the block continues executing and the next time it attempts to use a captured value
Closes facebook#11145

Differential Revision: D4261499

Pulled By: javache

fbshipit-source-id: 46424c6dd8cfa085cef32d945308de07798040bc
@atticoos
Copy link
Contributor

atticoos commented Mar 21, 2017

Linking a crash report incase this can provide further details

http://crashes.to/s/ee7279ad0b8

ReactNative version 0.41.2

@myusuf3
Copy link
Contributor

myusuf3 commented Apr 3, 2017

This is happening frequently for many users any ideas what would be main cause for this.

@rigdern
Copy link
Contributor

rigdern commented Apr 3, 2017

@myusuf3 What version of React Native are you seeing the crashes on? What callstack are you seeing? This information is important in order to be able to give you relevant suggestions.

A couple of fixes (2342377 and 2aca021) went into React Native 0.41 for fixing image loader crashes. If you are using an older version of React Native, you can backport these 2 fixes into your version of React Native. If you are seeing these crashes on React Native 0.41 or newer then I don't have any tips for you. Somebody needs to investigate the remaining image loader crash more deeply.

@myusuf3
Copy link
Contributor

myusuf3 commented Apr 4, 2017

@rigdern we are on ~0.35.0 but have been for awhile and the bug just exposed itself in the latest release. Not sure what triggered it but looking for a workaround atm. That being said has '0.41.0' been released? I will try backporting those fixes but can't get it to consistently reproduce this error atm.

@rigdern
Copy link
Contributor

rigdern commented Apr 4, 2017

@myusuf3 Yes, 0.41 was released at the beginning of February. As described in this blog post, a new version of React Native is released at the beginning of each month. 0.43 should be released sometime near the beginning of this month. You can see the full list of releases at https://github.com/facebook/react-native/releases.

The image loader crashes are race conditions and it can be challenging to get the conditions just right to reproduce them consistently. In general, they trigger when an image is canceled while it's in the middle of being loaded.

@myusuf3
Copy link
Contributor

myusuf3 commented Apr 4, 2017

@rigdern so do those commits fix the issue? We aren't able to reproduce but a significant chunk of users are hitting the issue.

@rigdern
Copy link
Contributor

rigdern commented Apr 4, 2017

@myusuf3 Probably, those 2 commits fixed the most common image loader crashes. What's the callstack at the time of the crash?

@myusuf3
Copy link
Contributor

myusuf3 commented Apr 4, 2017

@rigdern

#0. Crashed: com.apple.main-thread
0  libobjc.A.dylib                0x18c55ef30 objc_msgSend + 16
1  App                      0x1002f8fec -[RCTNetworkTask cancel] + 4297707500
2  App                      0x1002f24d8 __64-[RCTImageLoader _loadURLRequest:progressBlock:completionBlock:]_block_invoke.235 + 4297680088
3  App                      0x1002f1c4c __118-[RCTImageLoader _loadImageOrDataWithURLRequest:size:scale:resizeMode:progressBlock:partialLoadBlock:completionBlock:]_block_invoke.186 + 4297677900
4  App                      0x1002f2840 __119-[RCTImageLoader loadImageWithURLRequest:size:scale:clipped:resizeMode:progressBlock:partialLoadBlock:completionBlock:]_block_invoke + 4297680960
5  App                      0x1002f5790 -[RCTImageView cancelImageLoad] + 4297693072
6  App                      0x1002f6e84 -[RCTImageView didMoveToWindow] + 4297698948
7  UIKit                          0x193958e08 -[UIView(Internal) _didMoveFromWindow:toWindow:] + 1544
8  UIKit                          0x193cf6df4 -[UIImageView _didMoveFromWindow:toWindow:] + 80
9  UIKit                          0x193958ad0 -[UIView(Internal) _didMoveFromWindow:toWindow:] + 720
10 UIKit                          0x193958ad0 -[UIView(Internal) _didMoveFromWindow:toWindow:] + 720
11 UIKit                          0x193958ad0 -[UIView(Internal) _didMoveFromWindow:toWindow:] + 720
12 UIKit                          0x193958ad0 -[UIView(Internal) _didMoveFromWindow:toWindow:] + 720
13 UIKit                          0x193958ad0 -[UIView(Internal) _didMoveFromWindow:toWindow:] + 720
14 UIKit                          0x193958ad0 -[UIView(Internal) _didMoveFromWindow:toWindow:] + 720
15 UIKit                          0x193958ad0 -[UIView(Internal) _didMoveFromWindow:toWindow:] + 720
16 UIKit                          0x193958ad0 -[UIView(Internal) _didMoveFromWindow:toWindow:] + 720
17 UIKit                          0x193957f7c __45-[UIView(Hierarchy) _postMovedFromSuperview:]_block_invoke + 156
18 Foundation                     0x18e540510 -[NSISEngine withBehaviors:performModifications:] + 168
19 UIKit                          0x193957df0 -[UIView(Hierarchy) _postMovedFromSuperview:] + 820
20 UIKit                          0x193c55268 __UIViewWasRemovedFromSuperview + 172
21 UIKit                          0x193956e6c -[UIView(Hierarchy) removeFromSuperview] + 512
22 App                      0x1001a9524 -[UIView(React) removeReactSubview:] + 4296332580
23 App                      0x1001d309c -[RCTUIManager _removeChildren:fromContainer:] + 4296503452
24 App                      0x1001d4610 -[RCTUIManager _manageChildren:moveFromIndices:moveToIndices:addChildReactTags:addAtIndices:removeAtIndices:registry:] + 4296508944
25 App                      0x1001d43ac __108-[RCTUIManager manageChildren:moveFromIndices:moveToIndices:addChildReactTags:addAtIndices:removeAtIndices:]_block_invoke + 4296508332
26 App                      0x1001d6110 __29-[RCTUIManager flushUIBlocks]_block_invoke + 4296515856
27 libdispatch.dylib              0x18c99d200 _dispatch_call_block_and_release + 24
28 libdispatch.dylib              0x18c99d1c0 _dispatch_client_callout + 16
29 libdispatch.dylib              0x18c9a1d6c _dispatch_main_queue_callback_4CF + 1000
30 CoreFoundation                 0x18dac1f2c __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ + 12
31 CoreFoundation                 0x18dabfb18 __CFRunLoopRun + 1660
32 CoreFoundation                 0x18d9ee048 CFRunLoopRunSpecific + 444
33 GraphicsServices               0x18f471198 GSEventRunModal + 180
34 UIKit                          0x1939c7818 -[UIApplication _run] + 684
35 UIKit                          0x1939c2550 UIApplicationMain + 208
36 App                      0x100149714 main (main.m:16)
37 libdispatch.dylib              0x18c9d05b8 (Missing)

@rigdern
Copy link
Contributor

rigdern commented Apr 4, 2017

@myusuf3 I'm not sure if that particular crash is fixed by the 2 commits. Based on this comment, somebody else crashed with this stack as well but never reported back whether my fixes resolved his issue.

@mikelambert
Copy link
Contributor

mikelambert commented Apr 12, 2017

@tianxind Your stacktrace is unrelated to the issue in this thread. Yours appears to be a crash in decodeImageData, and has nothing to do with NSURLSession or cancelImageLoad. I suggest filing a different bug about your crash, so as to not pollute/derail this already-long bug here.

@tianxind
Copy link

@mikelambert ok I will delete my post

@booboothefool
Copy link

booboothefool commented May 4, 2017

Oh man, this thing has been plaguing me for months now, since I started using React Native, and I finally found this thread!

(Just posting to make sure this is what everyone else is getting? The crash seems to appear almost random/happens if you're unlucky when loading/scrolling down a list with images.)

I'm on RN 0.37. Pulling in @rigdern 's two commits should do the trick?

screen shot 2017-05-04 at 2 23 19 am

Can someone tell me if these stack traces are related? I do not know how to read them. Instabug/Instabug-iOS#150 My first stack trace (in the screenshot) looks similar to #10433 (comment), but the other two (copy pasted) don't look like the ones posted in this thread. Wouldn't want to be bugging the good people over there for a React Native image/list problem.

UPDATE: Posting screenshot here for convenience:
crash
Big thanks to @rigdern. 2aca021 solves this crash.

@rigdern
Copy link
Contributor

rigdern commented May 4, 2017

@booboothefool The crash in your screenshot was fixed by 2aca021 which is included in React Native 0.41 and higher.

@hramos
Copy link
Contributor

hramos commented Jul 20, 2017

Hi there! This issue is being closed because it has been inactive for a while. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. Either way, we're automatically closing issues after a period of inactivity. Please do not take it personally!

If you think this issue should definitely remain open, please let us know. The following information is helpful when it comes to determining if the issue should be re-opened:

  • Does the issue still reproduce on the latest release candidate? Post a comment with the version you tested.
  • If so, is there any information missing from the bug report? Post a comment with all the information required by the issue template.
  • Is there a pull request that addresses this issue? Post a comment with the PR number so we can follow up.

If you would like to work on a patch to fix the issue, contributions are very welcome! Read through the contribution guide, and feel free to hop into #react-native if you need help planning your contribution.

@hramos hramos added the Icebox label Jul 20, 2017
@hramos hramos closed this as completed Jul 20, 2017
@bitfused
Copy link

We are still getting multiple crashes in production on v0.41:

screen shot 2017-11-28 at 11 58 32 am
screen shot 2017-11-28 at 11 58 23 am

@facebook facebook locked as resolved and limited conversation to collaborators Jul 20, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jul 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Platform: iOS iOS applications. Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests