Keep rendering to screen in iOS inactive state - #25196

@gaaclarke This line might be why it's still not working.
Also the diff that I currently have is slightly different to the one you had so perhaps the conflict resolution went wrong before?

Let me know if there is anything I can do :)

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

This line might be why it's still not working.

@Kavantix What isn't working? I tested it and it addressed flutter/flutter#71051

Ah that's even better, you mentioned in 71051 that the PR did not solve it.
So probably fixed then by the correct merge :)

But just for my sanity, what's the current plan @gaaclarke?
Do we simply change it to work like this for both openGL and metal or should we make something to differentiate between the two?

Yep, as for that line you point out, it might be more correct to be foreground instead of active, but let's error on the side of caution and only change what we have to in order to fix the issue.

what's the current plan

I think we should land this. There is no need to differentiate between OpenGL and Metal. Just because Metal doesn't crash in the background doesn't mean it doesn't create problems. I just fixed a bug where we'd do image uploading when launching the app in the Background state via the GPU. It didn't crash but would result in pink images being drawn.

I just want to test this a bit more manually and try to think if there is some way we could test it automatically (not likely).

Yep, as for that line you point out, it might be more correct to be foreground instead of active, but let's error on the side of caution and only change what we have to in order to fix the issue.

Agreed, when starting the shell it should not matter that much anyways.

I think we should land this. There is no need to differentiate between OpenGL and Metal. Just because Metal doesn't crash in the background doesn't mean it doesn't create problems. I just fixed a bug where we'd do image uploading when launching the app in the Background state via the GPU. It didn't crash but would result in pink images being drawn.
I just want to test this a bit more manually and try to think if there is some way we could test it automatically (not likely).

Alright, the crash isn't that big either right since from the user's point of view the app seemed to just be closed by the os which already happens quite a lot on the older devices that were affected

I'd recommend also testing with background updates disabled in the system settings, should not make much of a difference but you never know ;)

As for automatically testing, perhaps you could test that a spinner keeps spinning while a native alert is being shown?
Or do you specifically mean to test whether or not it crashes?

Alright, the crash isn't that big either right since from the user's point of view the app seemed to just be closed by the os which already happens quite a lot on the older devices that were affected

Based on the documentation you pointed to, we shouldn't have to worry about the race condition crash when backgrounding an app. That's why I'm comfortable with the change.

While it's true that it might not be the biggest deal for end users if we crashed when backgrounding, Flutter's customers are actually the developers, not the end-users. If the developers are using something like Crashlytics it will show up if 10% of the time the app crashes when it's backgrounded, but the developers won't know under what conditions it crashes. They just know it crashes. That's how the original problem was reported.

I'm investigating the test failures. I'm having a hard time figuring out what is going on since there are a few infra bugs happening at the same time. I can reproduce the failures locally, but if I revert the PR I still get the failures. I've also seen tests that fail from the command-line pass from Xcode, too.

I'll give it another try tomorrow, it's very frustrating and I'm not 100% sure the PR is at fault. There is some hacks in the scenario integration tests that might have to be updated. I'm pretty sure this failure isn't indicative of a problem with the PR, but is a problem with the tests.

@Kavantix you were right about the initialization of the shell with the gpu disabled if the app is backgrounded. You beautiful random internet person, why didn't I listen to you?

Without that change the platform views controller would initiate a CATransaction, then the frame would get rejected since the GPU was disabled, then the next time we tried to render a frame it would crash because there was an open CATransaction. Listening for the foreground notifications isn't enough because we are already foregrounded by the time we create the shell, we won't receive another notification. I'm surprised it worked with manual testing.

I'll just test this manually with background apps then we should be good.

@Kavantix you were right about the initialization of the shell with the gpu disabled if the app is backgrounded. You beautiful random internet person, why didn't I listen to you?

Haha I don't know, it was only a lucky guess XD.

Without that change the platform views controller would initiate a CATransaction, then the frame would get rejected since the GPU was disabled, then the next time we tried to render a frame it would crash because there was an open CATransaction. Listening for the foreground notifications isn't enough because we are already foregrounded by the time we create the shell, we won't receive another notification. I'm surprised it worked with manual testing.

Makes a lot of sense!

Thanks for taking the time to tackle this!, it has been bugging me ever since I first noticed it (about half a year before I filed the original PR 🙈)

(this is running into flakey tests, I'm fixing them here: flutter/flutter#79011)

I'm not sure what's going on here. The tests run fine if executed from Xcode, but they seem to fail when run from the commandline:

I'll try a bit longer, this is taking way more work than I originally expected.

I tested the change manually with apps that launch in the background, just to double check everything is good there. It looks good as I would have expected from the code. I used https://github.com/pawlowskim/headless_mode_images_issue and walked around my block.