feat: support async onShouldStartLoad requests #49
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |
| #import <React/RCTConvert.h> | ||
| #import <React/RCTAutoInsetsProtocol.h> | ||
| #import "RNCWKProcessPoolManager.h" | ||
| #import "RNCWebViewDecisionManager.h" | ||
| #if !TARGET_OS_OSX | ||
| #import <UIKit/UIKit.h> | ||
| #else | ||
|
|
@@ -1106,7 +1107,32 @@ - (void) webView:(WKWebView *)webView | |
| NSURLRequest *request = navigationAction.request; | ||
| BOOL isTopFrame = [request.URL isEqual:request.mainDocumentURL]; | ||
|
|
||
| void (^allowNavigation)(void) = ^{ | ||
| if (self->_onLoadingStart) { | ||
| // We have this check to filter out iframe requests and whatnot | ||
| if (isTopFrame) { | ||
| NSMutableDictionary<NSString *, id> *event = [self baseEvent]; | ||
| [event addEntriesFromDictionary: @{ | ||
| @"url": (request.URL).absoluteString, | ||
| @"navigationType": navigationTypes[@(navigationType)] | ||
| }]; | ||
| self->_onLoadingStart(event); | ||
| } | ||
| } | ||
| decisionHandler(WKNavigationActionPolicyAllow); | ||
| }; | ||
|
|
||
| if (_onShouldStartLoadWithRequest) { | ||
| int lockIdentifier = [[RNCWebViewDecisionManager getInstance] setDecisionHandler:^(BOOL shouldStart) { | ||
| dispatch_async(dispatch_get_main_queue(), ^{ | ||
| if (!shouldStart) { | ||
| decisionHandler(WKNavigationActionPolicyCancel); | ||
| return; | ||
| } | ||
| allowNavigation(); | ||
| }); | ||
| }]; | ||
|
Comment on lines
+1126
to
+1134
There was a problem hiding this comment. When the WKWebView is deallocated while there are pending navigation decisions, the stored decision handlers in the singleton RNCWebViewDecisionManager will never be called, leading to a memory leak. The decisionHandler closure from WKWebView's decidePolicyForNavigationAction captures references that won't be released until the handler is invoked. Consider adding a mechanism to cancel pending decisions when the webview is deallocated, or add a timeout mechanism to automatically clean up stale handlers. |
||
|
|
||
|
Comment on lines
+1126
to
+1135
Author
There was a problem hiding this comment. The allowing code is moved into the |
||
| NSMutableDictionary<NSString *, id> *event = [self baseEvent]; | ||
| if (request.mainDocumentURL) { | ||
| [event addEntriesFromDictionary: @{ | ||
|
|
@@ -1116,30 +1142,15 @@ - (void) webView:(WKWebView *)webView | |
| [event addEntriesFromDictionary: @{ | ||
| @"url": (request.URL).absoluteString, | ||
| @"navigationType": navigationTypes[@(navigationType)], | ||
| @"isTopFrame": @(isTopFrame), | ||
| @"lockIdentifier": @(lockIdentifier) | ||
| }]; | ||
| _onShouldStartLoadWithRequest(event); | ||
| return; | ||
| } | ||
|
|
||
| // Allow all navigation by default | ||
| allowNavigation(); | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,18 @@ | ||||||||||||||||||||||||
| #import <Foundation/Foundation.h> | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| typedef void (^DecisionBlock)(BOOL); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| @interface RNCWebViewDecisionManager : NSObject { | ||||||||||||||||||||||||
| int nextLockIdentifier; | ||||||||||||||||||||||||
| NSMutableDictionary *decisionHandlers; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| @property (nonatomic) int nextLockIdentifier; | ||||||||||||||||||||||||
| @property (nonatomic, retain) NSMutableDictionary *decisionHandlers; | ||||||||||||||||||||||||
|
There was a problem hiding this comment. The decisionHandlers property should use 'copy' semantic instead of 'retain' for storing blocks. In Objective-C, blocks need to be copied to move them from the stack to the heap to ensure they remain valid after the scope in which they were created ends. This is a critical memory management issue that could lead to crashes when the block is invoked after the original stack frame is destroyed. |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
Comment on lines
+5
to
+12
There was a problem hiding this comment. The instance variables in the @interface block are redundant when @Property declarations are present, as properties automatically synthesize their backing instance variables. The duplicate declarations of nextLockIdentifier and decisionHandlers in lines 6-7 should be removed to follow modern Objective-C conventions.
Suggested change
|
||||||||||||||||||||||||
| + (id) getInstance; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| - (int)setDecisionHandler:(DecisionBlock)handler; | ||||||||||||||||||||||||
| - (void) setResult:(BOOL)shouldStart | ||||||||||||||||||||||||
| forLockIdentifier:(int)lockIdentifier; | ||||||||||||||||||||||||
| @end | ||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| #import "RNCWebViewDecisionManager.h" | ||
| #import <React/RCTLog.h> | ||
|
|
||
| /** | ||
| * Thread-safe singleton that manages navigation decision handlers for WKWebView. | ||
| * | ||
| * This class bridges async navigation decisions between: | ||
| * - WKWebView delegate (main thread) - stores decision handlers | ||
| * - React Native bridge (background thread) - resolves decisions from JS | ||
| * | ||
| * All public methods use @synchronized for thread safety since they access | ||
| * shared state (nextLockIdentifier and decisionHandlers) from different threads. | ||
| */ | ||
| @implementation RNCWebViewDecisionManager | ||
|
|
||
| @synthesize nextLockIdentifier; | ||
| @synthesize decisionHandlers; | ||
|
|
||
| + (id)getInstance { | ||
| static RNCWebViewDecisionManager *lockManager = nil; | ||
| static dispatch_once_t onceToken; | ||
| dispatch_once(&onceToken, ^{ | ||
| lockManager = [[self alloc] init]; | ||
| }); | ||
| return lockManager; | ||
| } | ||
|
|
||
| /** | ||
| * Stores a decision handler and returns a unique identifier. | ||
| * Called from the main thread (WKNavigationDelegate). | ||
| * @synchronized ensures atomic increment + insertion. | ||
| */ | ||
| - (int)setDecisionHandler:(DecisionBlock)decisionHandler { | ||
| @synchronized (self) { | ||
| int lockIdentifier = self.nextLockIdentifier++; | ||
|
There was a problem hiding this comment. The nextLockIdentifier counter could theoretically overflow after 2^31-1 increments (about 2.1 billion). While unlikely in normal usage, if this happens, the identifier will wrap around to negative values or repeat, potentially causing collisions with pending navigation decisions. Consider using a more robust identifier generation strategy, such as checking for existing keys before assignment, or using a larger integer type. |
||
| [self.decisionHandlers setObject:decisionHandler forKey:@(lockIdentifier)]; | ||
| return lockIdentifier; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Resolves a pending navigation decision. | ||
| * Called from the RN bridge thread (background) when JS responds. | ||
| * | ||
| * The handler is invoked OUTSIDE the @synchronized block to prevent deadlocks, | ||
| * since the handler dispatches to the main queue and could potentially | ||
| * trigger another navigation that re-enters this class. | ||
| */ | ||
| - (void) setResult:(BOOL)shouldStart | ||
| forLockIdentifier:(int)lockIdentifier { | ||
| DecisionBlock handler; | ||
| @synchronized (self) { | ||
| handler = [self.decisionHandlers objectForKey:@(lockIdentifier)]; | ||
| if (handler == nil) { | ||
| RCTLogWarn(@"Lock not found"); | ||
ChALkeR marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return; | ||
| } | ||
| [self.decisionHandlers removeObjectForKey:@(lockIdentifier)]; | ||
| } | ||
| handler(shouldStart); | ||
|
Comment on lines
+49
to
+60
There was a problem hiding this comment. The WKWebView decisionHandler must be called exactly once. If JS calls startLoadWithResult multiple times with the same lockIdentifier (either due to a bug or race condition), the decisionHandler will be invoked multiple times, which will cause a crash with the error "Completion handler passed to -[WKWebView decidePolicyForNavigationAction:decisionHandler:] was called more than once". Consider adding a guard to track whether a decision has already been made for a given lockIdentifier to prevent double invocation. |
||
| } | ||
|
|
||
| - (id)init { | ||
| if (self = [super init]) { | ||
| self.nextLockIdentifier = 1; | ||
| self.decisionHandlers = [[NSMutableDictionary alloc] init]; | ||
| } | ||
| return self; | ||
| } | ||
|
|
||
| - (void)dealloc {} | ||
|
|
||
ChALkeR marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| @end | ||
Uh oh!
There was an error while loading. Please reload this page.