Skip to content

[iOS] Fabric: Fixes assert failure when surface stop before we start surface #48213

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.

Sign up for GitHub

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

Closed
zhongwuzw wants to merge 2 commits into from
Closed

[iOS] Fabric: Fixes assert failure when surface stop before we start surface #48213

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
80e31b3
[iOS] Fabric: Fixes assert failure when surface stop before we start …
zhongwuzw Dec 11, 2024
fcdc14f
Add the comment
zhongwuzw Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTHost.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,8 @@ - (RCTFabricSurface *)createSurfaceWithModuleName:(NSString *)moduleName
surface.surfaceHandler.setDisplayMode(displayMode);
[self _attachSurface:surface];

[_instance callFunctionOnBufferedRuntimeExecutor:[surface](facebook::jsi::Runtime &_) { [surface start]; }];
__weak RCTFabricSurface *weakSurface = surface;
[_instance callFunctionOnBufferedRuntimeExecutor:[weakSurface](facebook::jsi::Runtime &_) { [weakSurface start]; }];
Comment on lines +257 to +259

@javache javache Dec 11, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not actually sure why this needs go through the JS thread, since start already dispatches async internally anyway and is threadsafe. It seems this was added in #45486 ?

Can we make this just [surface start] and remove the weak ref? It not, can you add a comment on why this requires synchronization on the JS thread?

@zhongwuzw zhongwuzw Dec 11, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@javache Hi, I added the comment, [surface start] needs to wait for the main bundle to be loaded, so we dispatched it on the buffered runtime executor.

@javache javache Dec 12, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit surprised by that. ReactInstance should be able to deal with the queuing required to delay surface start until the bundle is ready.

@zhongwuzw zhongwuzw Dec 12, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The surface start seems just schedule to the js thread :
https://github.com/facebook/react-native/blob/f4a17fbb93d6cdba29a340f0dde4d155af972439/packages/react-native/ReactCommon/react/renderer/uimanager/UIManager.cpp#L238-L242

return surface;
}

Expand Down