Hide the logbox window explicitly. New behavior in iOS SDK appears to…

[paddlefish]

Summary

Fixes #32434: RCTLogBox window is orphaned, covering entire screen.

After this change, the logbox window once again is removed from the screen.

Changelog

Some third-party SDKs may hold references to created UIWindow, UIViewController, or UIView objects. Doing so means that the current code's hide method that releases the reference to the UIWindow in LogBox will not cause the window to be dealloc'd, and thus instead it will remain on the screen. This change explicitly hides the LogBox window when the reference is released, so that even if some other SDK holds onto the window it will still be taken off the screen.

[iOS] [Fixed] - 32434

Test Plan

1. Use console.warn to generate a yellow warning message in log box.  Also install a third-party SDK that holds onto a reference to UIWindow -- for example the Facebook SDK, the Data Dog SDK, or any number of other SDKs that use `swizzling` to intercept calls like `viewDidAppear:`.
   

2. click the log
   

3. tap "dismiss"
   

4. try to tap anywhere
   

5. Use Xcode view debugger to inspect the UI state

Expected

The app still responds to the touch.
In Xcode, there is not an extra UIWindow covering the screen

[pull-bot]

PR build artifact for 1d47f16 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,310,244	+0
android	hermes	armeabi-v7a	7,874,312	+0
android	hermes	x86	8,677,251	+0
android	hermes	x86_64	8,636,466	+0
android	jsc	arm64-v8a	9,812,535	+0
android	jsc	armeabi-v7a	8,773,418	+0
android	jsc	x86	9,761,393	+0
android	jsc	x86_64	10,362,338	+0

Base commit: 0c07500
Branch: main

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 63f66fc
Branch: main

[charlesbdudley]

I'm having difficulty reproducing this issue, can you provide a snack or an example project please?

[paddlefish]

@charlesbdudley I created a new project with the same iOS SDK and was also unable to reproduce this. I spent a while in my real app using Instruments to see if I could figure out where the retain cycle is coming from. I have several third party SDKs installed. As is typical with "no code" marketing SDKs, these SDKs use swizzling to insert their hooks all over the place. It's entirely possible that one of them is holding on to the "keyWindow" for some reason. I've located several places these sdks make calls to get all the applications windows, and iterate them.

However, even if that's true, I don't see why the proposed fix for this is a bad thing. Right now the logbox window only closes because it gets dealloc'd. The intention of the hide method is to hide the window; it seems like it should do that even if some other piece of code has a reference to the window. Or a reference to a window's view, or a a windows view controller ... as it is now, it's a house of cards.

[charlesbdudley]

@paddlefish That makes sense to me! Thanks for digging deeper on this. To avoid confusion for the changelog, do you mind updating the title and description (more of an enhancement now than a bug fix)? Then I'll import it for internal review.

[paddlefish]

@charlesbdudley I updated the title and description.

[facebook-github-bot]

@sshic has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[philIip]

i feel we should think of some way to remove the view from the actual hierarchy, though that is probably the responsibility of whomever decided to retain this window.

i think this is the safest and least invasive way to resolve this edge case, so accepting!

[facebook-github-bot]

@sshic merged this pull request in 72ea0e1.