Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Manage resource and onscreen contexts using separate IOSGLContext objects #12277

Merged
merged 3 commits into from
Sep 16, 2019
Merged

Manage resource and onscreen contexts using separate IOSGLContext objects #12277

merged 3 commits into from
Sep 16, 2019

Conversation

gw280
Copy link
Contributor

@gw280 gw280 commented Sep 13, 2019

Attempt 3. So the issue here was that with the change to WeakPtr for IOSGLContext we ended up with a nullptr deref on simulator where there is no GL context.

I've fixed that and verified it passes LUCI:

https://chromium-swarm.appspot.com/task?id=4744ea5e1a2d3010#
https://chromium-swarm.appspot.com/task?id=4745214d8f1a6610

I am also going to file a follow up issue to clean up the logic surrounding [FlutterView createSurfaceWithResourceGLContext] as it's quite confusing right now with the way it deals with simulator and device.

@gw280
Manage resource and onscreen contexts using separate IOSGLContext obj…
282f968 
…ects.

FlutterView owns the onscreen context, and PlatformViewIOS owns the resource context.
@gw280 gw280 requested a review from dnfield September 13, 2019 21:09
gw280
gw280 commented Sep 13, 2019
View reviewed changes
shell/platform/darwin/ios/platform_view_ios.mm Outdated
fml::WeakPtr<IOSGLContext> weak_gl_context;
if (resource_gl_context_) {
weak_gl_context = resource_gl_context_->GetWeakPtr();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what changed.

dnfield
dnfield approved these changes Sep 13, 2019
View reviewed changes
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

LGTM

dnfield
dnfield approved these changes Sep 16, 2019
View reviewed changes
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

still LGTM - you can ignore the "fail" test, that was put in place to stop the autoroller when the tree was in bad shape. It would also go away if you sync to head

@gw280
Copy link
Contributor Author

gw280 commented Sep 16, 2019

Update to fix the failing unit test that was caught by @xster's new test (thanks!)

So basically what was going on is that this patch released the IOSSurface in order to not retain the IOSurface when backgrounded (a requirement of the issue that this patch was to resolve). This happened in PlatformViewIOS::NotifyDestroyed(). When the app came back into the foreground, though, FlutterViewController::surfaceUpdated would call NotifyCreated which would then try and create a GPU surface but the IOSSurface was gone, thus returning a nullptr and ending up with a nullptr deref down the chain (in the rasterizer).

This patch recreates the IOSSurface correctly when coming into the foreground, providing the owner FlutterViewController is still present.

@xster
Copy link
Member

xster commented Sep 16, 2019

Another scenario we should validate for (tests being added in #12128) is to not unconditionally re-claim resources on foregrounding.

For instance, if I showed a VC, popped it but held reference to it, then background and foregrounded the app, I shouldn't recreate the IOSSurface again until I present that VC again.

@gw280
Copy link
Contributor Author

gw280 commented Sep 16, 2019

I think I'd consider that an optimisation that's out of scope for this PR, let's file a new one.

@gw280 gw280 merged commit 5a8da65 into flutter:master Sep 16, 2019
@xster xster mentioned this pull request Sep 17, 2019
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 17, 2019
@engine-flutter-autoroll
5a8da65 Manage resource and onscreen contexts using separate IOSGLCon…
6fb3f9a 
…text objects (flutter/engine#12277)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Sep 17, 2019
Merged
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Sep 18, 2019
@engine-flutter-autoroll
Roll engine 63873d9..d1692d4 (22 commits) (#40717)
00d50be 
[email protected]:flutter/engine.git/compare/63873d9f421f...d1692d4

git log 63873d9..d1692d4 --no-merges --oneline
2019-09-17 [email protected] Update canvaskit backend (flutter/engine#12318)
2019-09-17 [email protected] README for the felt tool (flutter/engine#12323)
2019-09-17 [email protected] Fix continuous event polling in the GLFW event loop (flutter/engine#12320)
2019-09-17 [email protected] Tests for #11283 (flutter/engine#12322)
2019-09-17 [email protected] Improve check to render (or not) a DRRect when inner falls outside of outer on RecordingCanvas (flutter/engine#12229)
2019-09-17 [email protected] Roll src/third_party/dart dd1969a43a..7505b3a5f0 (39 commits)
2019-09-17 [email protected] Channel buffers (flutter/engine#12167)
2019-09-17 [email protected] Make iOS FlutterViewController stop sending inactive/pause on app lifecycle events when not visible (flutter/engine#12128)
2019-09-17 [email protected] Adds PluginRegistry to the C++ client wrapper API (flutter/engine#12287)
2019-09-17 [email protected] Add "type" to getDisplayRefreshRate protocol (flutter/engine#12319)
2019-09-17 [email protected] Add a build command to felt (flutter/engine#12303)
2019-09-17 [email protected] Roll src/third_party/skia df432d5efb70..d545bfbb94ca (1 commits) (flutter/engine#12316)
2019-09-17 [email protected] Roll fuchsia/sdk/core/linux-amd64 from rwf0-... to RRgw-... (flutter/engine#12315)
2019-09-17 [email protected] Roll src/third_party/skia f8486f2c5fb6..df432d5efb70 (1 commits) (flutter/engine#12313)
2019-09-17 [email protected] Roll src/third_party/skia b47704b0bd34..f8486f2c5fb6 (2 commits) (flutter/engine#12312)
2019-09-16 [email protected] Fix the declaration of setSystemGestureExclusionRects to match the PlatformMessageHandler interface (flutter/engine#12306)
2019-09-16 [email protected] Manage resource and onscreen contexts using separate IOSGLContext objects (flutter/engine#12277)
2019-09-16 [email protected] Cleanup in web_ui (flutter/engine#12307)
2019-09-16 [email protected] Made flutter startup faster by allowing initialization to be parallelized (flutter/engine#10182)
2019-09-16 [email protected] Roll src/third_party/skia c22498502cda..b47704b0bd34 (16 commits) (flutter/engine#12304)
2019-09-16 [email protected] Include firefox in check to quote font families (flutter/engine#12288)
2019-09-16 [email protected] Roll src/third_party/dart 7799f424f4..dd1969a43a (2 commits)


If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected] on the revert to ensure that a human
is aware of the problem.

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
@gw280 gw280 mentioned this pull request Sep 19, 2019
Open
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Sep 30, 2019
@engine-flutter-autoroll @Inconnu08
Roll engine 63873d9..d1692d4 (22 commits) (flutter#40717)
85706af 
[email protected]:flutter/engine.git/compare/63873d9f421f...d1692d4

git log 63873d9..d1692d4 --no-merges --oneline
2019-09-17 [email protected] Update canvaskit backend (flutter/engine#12318)
2019-09-17 [email protected] README for the felt tool (flutter/engine#12323)
2019-09-17 [email protected] Fix continuous event polling in the GLFW event loop (flutter/engine#12320)
2019-09-17 [email protected] Tests for flutter#11283 (flutter/engine#12322)
2019-09-17 [email protected] Improve check to render (or not) a DRRect when inner falls outside of outer on RecordingCanvas (flutter/engine#12229)
2019-09-17 [email protected] Roll src/third_party/dart dd1969a43a..7505b3a5f0 (39 commits)
2019-09-17 [email protected] Channel buffers (flutter/engine#12167)
2019-09-17 [email protected] Make iOS FlutterViewController stop sending inactive/pause on app lifecycle events when not visible (flutter/engine#12128)
2019-09-17 [email protected] Adds PluginRegistry to the C++ client wrapper API (flutter/engine#12287)
2019-09-17 [email protected] Add "type" to getDisplayRefreshRate protocol (flutter/engine#12319)
2019-09-17 [email protected] Add a build command to felt (flutter/engine#12303)
2019-09-17 [email protected] Roll src/third_party/skia df432d5efb70..d545bfbb94ca (1 commits) (flutter/engine#12316)
2019-09-17 [email protected] Roll fuchsia/sdk/core/linux-amd64 from rwf0-... to RRgw-... (flutter/engine#12315)
2019-09-17 [email protected] Roll src/third_party/skia f8486f2c5fb6..df432d5efb70 (1 commits) (flutter/engine#12313)
2019-09-17 [email protected] Roll src/third_party/skia b47704b0bd34..f8486f2c5fb6 (2 commits) (flutter/engine#12312)
2019-09-16 [email protected] Fix the declaration of setSystemGestureExclusionRects to match the PlatformMessageHandler interface (flutter/engine#12306)
2019-09-16 [email protected] Manage resource and onscreen contexts using separate IOSGLContext objects (flutter/engine#12277)
2019-09-16 [email protected] Cleanup in web_ui (flutter/engine#12307)
2019-09-16 [email protected] Made flutter startup faster by allowing initialization to be parallelized (flutter/engine#10182)
2019-09-16 [email protected] Roll src/third_party/skia c22498502cda..b47704b0bd34 (16 commits) (flutter/engine#12304)
2019-09-16 [email protected] Include firefox in check to quote font families (flutter/engine#12288)
2019-09-16 [email protected] Roll src/third_party/dart 7799f424f4..dd1969a43a (2 commits)


If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected] on the revert to ensure that a human
is aware of the problem.

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
gaaclarke added a commit to gaaclarke/engine that referenced this pull request Oct 3, 2019
@gaaclarke
Revert "Manage resource and onscreen contexts using separate IOSGLCon…
65342cb 
…text objects (flutter#12277)"

This reverts commit 5a8da65.
@gaaclarke gaaclarke mentioned this pull request Oct 3, 2019
Merged
chinmaygarde pushed a commit that referenced this pull request Oct 3, 2019
@gaaclarke @chinmaygarde
Revert "Manage resource and onscreen contexts using separate IOSGLCon…
142cf00 
…text objects (#12277)" (#12773)

This reverts commit 5a8da65.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dnfield dnfield dnfield approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gw280 @xster @dnfield @googlebot