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

Re-entrant loadView / viewDidLoad #3753

Closed
jleandroperez opened this issue Mar 15, 2024 · 4 comments · Fixed by #3754
Closed

Re-entrant loadView / viewDidLoad #3753

jleandroperez opened this issue Mar 15, 2024 · 4 comments · Fixed by #3754

Comments

@jleandroperez
Copy link
Contributor

jleandroperez commented Mar 15, 2024

Platform

iOS

Installed

Swift Package Manager

Version

8.21.0

Steps to Reproduce

  1. Setup Sentry with PerformanceTracking and a profilingRate of 0.5
  2. Launch the app

50% of the times we launch our app, Sentry's swizzled loadView method invokes the .view property of the relevant ViewController.

This ends up triggering two viewDidLoad invocations in our ViewController, and is causing duplication of UI objects (+ performance degradation).

Expected Result

No loadView / viewDidLoad duplication

Actual Result

Our main viewController is getting x2 viewDidLoad invocations. Please review the screenshots below:



Are you willing to submit a PR?

Of course, PR will be up shortly. Thank you =)

@bjhomer
Copy link
Contributor

bjhomer commented Mar 15, 2024

Specifically, the issue can be seen in the first screenshot, where loadViewIfNeeded is being called, and that is recursively calling into loadViewIfNeeded again before the view was fully loaded the first time.

@brustolin
Copy link
Contributor

Hello @jleandroperez, thanks for reaching out.
I don't see the double viewDidLoad in your screenshots.
I couldn't reproduce it either.
Can you explain a little bit more about your view controller setup?

@bjhomer
Copy link
Contributor

bjhomer commented Mar 19, 2024

Hi, I'm @jleandroperez's co-worker. Don't look for duplicate viewDidLoad; look for duplicate loadViewIfNeeded. The issue can be seen in the stack traces on the left side of the screenshot. Inside -[DOIMainContainerViewController setChildViewController:], we are accessing the .view of a view controller for the first time. This is triggering loadViewIfNeeded on the view controller that was passed in (as seen in frames 30-32). That's going down into some Sentry code (-[SentryUIApplication relevantViewControllerFromWindow:]) which eventually ends up calling .view on that same view controller that's already loading its view.

Since the view has not finished loading yet, .view is apparently still nil. So accessing the view results in it loading the view again (seen in frames 12-14 on the stack). After that view finishes loading, it will call viewDidLoad (as seen in frame 1). And then it will unwind back to frames 30-32, UIKit will finish loading the view, and then call viewDidLoad from there. This means that the view got loaded into memory twice.

The duplicate viewDidLoad is a problem, but the bigger problem is that the entire UI is being loaded twice. (In our case, this is happening near the root level of the entire app hierarchy, so loading the UI twice involves loading a bunch of other views and view controllers as well.)

@kahest
Copy link
Member

kahest commented Mar 21, 2024

Hi @bjhomer, thanks for the clarification and @jleandroperez for the PR! As noted on the PR, we'd love to find a way to automatically test this, other than we think it's ready

philipphofmann added a commit that referenced this issue Mar 27, 2024
When performance tracking is enabled, loadView's swizzled implementation may result in a duplicate chain of events, that result in multiple viewDidLoad invocations, in our main app's VC.

Fixes GH-3753

Co-authored-by: BJ Homer <bjhomer@gmail.com>
Co-authored-by: Philipp Hofmann <ph.hofmann@pm.me>
philipphofmann added a commit that referenced this issue Mar 27, 2024
philipphofmann added a commit that referenced this issue Mar 27, 2024
threema-matteo pushed a commit to threema-ch/sentry-cocoa that referenced this issue May 21, 2024
When performance tracking is enabled, loadView's swizzled implementation may result in a duplicate chain of events, that result in multiple viewDidLoad invocations, in our main app's VC.

Fixes getsentryGH-3753

Co-authored-by: BJ Homer <bjhomer@gmail.com>
Co-authored-by: Philipp Hofmann <ph.hofmann@pm.me>
threema-matteo pushed a commit to threema-ch/sentry-cocoa that referenced this issue May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants