Skip to content

Commit

Permalink
iOS: when the bridge has been invalidated, NativeModule lookup should…
Browse files Browse the repository at this point in the history
… just return nil

Summary:
There's a corner case where:
* The bridge gets invalidated, and native modules are cleaning up themselves (but not done yet)
* Something asks for a NativeModule instance - ideally it should just get nil at this point
* If TM Manager is invalidating, we get nil correctly, but we continue thru the old NativeModule lookup logic
* The latter will fail anyway, and would throw a redbox (RCTLogError).

So, if the bridge is invalidated, if TM Manager returns nil, we should just return nil for old NativeModule lookup.

The module of interest is RCTImageLoader, which was requested by RCTImageView on deallocation. The problem is RCTImageView got dealloc'ed **after** the bridge has been invalidated, so the lookup would always fail...

Bonus: RCTImageView should just keep a weak ref to the RCTImageLoader, so that:
* if the imageLoader is still alive on image dealloc, it can still access them (last minute "meaningless" cleanup)
* if the imageLoader is gone, then the image deallocation doesn't do anything

Changelog: [iOS] [Fixed] - Fix module lookup race condition on bridge invalidation.

Reviewed By: p-sun, sammy-SC

Differential Revision: D21371845

fbshipit-source-id: 862dc07de18ddbfb90e87e24b8dbd001147ddce4
  • Loading branch information
fkgozali authored and facebook-github-bot committed May 3, 2020
1 parent 1787c16 commit 8ad8107
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 8 deletions.
19 changes: 11 additions & 8 deletions Libraries/Image/RCTImageView.mm
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ @implementation RCTImageView
// Weak reference back to the bridge, for image loading
__weak RCTBridge *_bridge;

// Weak reference back to the active image loader.
__weak id<RCTImageLoaderWithAttributionProtocol> _imageLoader;

// The image source that's currently displayed
RCTImageSource *_imageSource;

Expand Down Expand Up @@ -327,11 +330,13 @@ - (void)reloadImage
[weakSelf imageLoaderLoadedImage:loadedImage error:error forImageSource:source partial:NO];
};

id<RCTImageLoaderWithAttributionProtocol> imageLoader = [_bridge moduleForName:@"ImageLoader"
lazilyLoadIfNecessary:YES];
RCTImageURLLoaderRequest *loaderRequest = [imageLoader loadImageWithURLRequest:source.request
size:imageSize
scale:imageScale
if (!_imageLoader) {
_imageLoader = [_bridge moduleForName:@"ImageLoader" lazilyLoadIfNecessary:YES];
}

RCTImageURLLoaderRequest *loaderRequest = [_imageLoader loadImageWithURLRequest:source.request
size:imageSize
scale:imageScale
clipped:NO
resizeMode:_resizeMode
attribution:{
Expand Down Expand Up @@ -470,9 +475,7 @@ - (void)didMoveToWindow
}

- (void)dealloc {
id<RCTImageLoaderWithAttributionProtocol> imageLoader = [_bridge moduleForName:@"ImageLoader"
lazilyLoadIfNecessary:YES];
[imageLoader trackURLImageDidDestroy:_loaderRequest];
[_imageLoader trackURLImageDidDestroy:_loaderRequest];
}

@end
5 changes: 5 additions & 0 deletions React/CxxBridge/RCTCxxBridge.mm
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,11 @@ - (id)moduleForName:(NSString *)moduleName lazilyLoadIfNecessary:(BOOL)lazilyLoa
}

// Module may not be loaded yet, so attempt to force load it here.
// Do this only if the bridge is still valid.
if (_didInvalidate) {
return nil;
}

const BOOL result = [self.delegate respondsToSelector:@selector(bridge:didNotFindModule:)] &&
[self.delegate bridge:self didNotFindModule:moduleName];
if (result) {
Expand Down

0 comments on commit 8ad8107

Please sign in to comment.