Manage resource and onscreen contexts using separate IOSGLContext objects #12277

gw280 · 2019-09-13T21:09:38Z

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.

…ects. FlutterView owns the onscreen context, and PlatformViewIOS owns the resource context.

gw280 · 2019-09-13T21:16:32Z

shell/platform/darwin/ios/platform_view_ios.mm

+    fml::WeakPtr<IOSGLContext> weak_gl_context;
+    if (resource_gl_context_) {
+      weak_gl_context = resource_gl_context_->GetWeakPtr();
+    }


This is what changed.

dnfield

LGTM

dnfield

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 · 2019-09-16T22:08:58Z

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 · 2019-09-16T22:46:12Z

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 · 2019-09-16T22:47:25Z

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

…text objects (flutter/engine#12277)

git@github.com:flutter/engine.git/compare/63873d9f421f...d1692d4 git log 63873d9..d1692d4 --no-merges --oneline 2019-09-17 hterkelsen@users.noreply.github.com Update canvaskit backend (flutter/engine#12318) 2019-09-17 mouad.debbar@gmail.com README for the felt tool (flutter/engine#12323) 2019-09-17 jason-simmons@users.noreply.github.com Fix continuous event polling in the GLFW event loop (flutter/engine#12320) 2019-09-17 15365765+rafern@users.noreply.github.com Tests for #11283 (flutter/engine#12322) 2019-09-17 ditman@gmail.com Improve check to render (or not) a DRRect when inner falls outside of outer on RecordingCanvas (flutter/engine#12229) 2019-09-17 bkonyi@google.com Roll src/third_party/dart dd1969a43a..7505b3a5f0 (39 commits) 2019-09-17 30870216+gaaclarke@users.noreply.github.com Channel buffers (flutter/engine#12167) 2019-09-17 xster@google.com Make iOS FlutterViewController stop sending inactive/pause on app lifecycle events when not visible (flutter/engine#12128) 2019-09-17 stuartmorgan@google.com Adds PluginRegistry to the C++ client wrapper API (flutter/engine#12287) 2019-09-17 liyuqian@google.com Add "type" to getDisplayRefreshRate protocol (flutter/engine#12319) 2019-09-17 mouad.debbar@gmail.com Add a build command to felt (flutter/engine#12303) 2019-09-17 skia-flutter-autoroll@skia.org Roll src/third_party/skia df432d5efb70..d545bfbb94ca (1 commits) (flutter/engine#12316) 2019-09-17 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from rwf0-... to RRgw-... (flutter/engine#12315) 2019-09-17 skia-flutter-autoroll@skia.org Roll src/third_party/skia f8486f2c5fb6..df432d5efb70 (1 commits) (flutter/engine#12313) 2019-09-17 skia-flutter-autoroll@skia.org Roll src/third_party/skia b47704b0bd34..f8486f2c5fb6 (2 commits) (flutter/engine#12312) 2019-09-16 jason-simmons@users.noreply.github.com Fix the declaration of setSystemGestureExclusionRects to match the PlatformMessageHandler interface (flutter/engine#12306) 2019-09-16 gw280@google.com Manage resource and onscreen contexts using separate IOSGLContext objects (flutter/engine#12277) 2019-09-16 goderbauer@google.com Cleanup in web_ui (flutter/engine#12307) 2019-09-16 30870216+gaaclarke@users.noreply.github.com Made flutter startup faster by allowing initialization to be parallelized (flutter/engine#10182) 2019-09-16 skia-flutter-autoroll@skia.org Roll src/third_party/skia c22498502cda..b47704b0bd34 (16 commits) (flutter/engine#12304) 2019-09-16 jonahwilliams@google.com Include firefox in check to quote font families (flutter/engine#12288) 2019-09-16 bkonyi@google.com 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 stuartmorgan@google.com 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

git@github.com:flutter/engine.git/compare/63873d9f421f...d1692d4 git log 63873d9..d1692d4 --no-merges --oneline 2019-09-17 hterkelsen@users.noreply.github.com Update canvaskit backend (flutter/engine#12318) 2019-09-17 mouad.debbar@gmail.com README for the felt tool (flutter/engine#12323) 2019-09-17 jason-simmons@users.noreply.github.com Fix continuous event polling in the GLFW event loop (flutter/engine#12320) 2019-09-17 15365765+rafern@users.noreply.github.com Tests for flutter#11283 (flutter/engine#12322) 2019-09-17 ditman@gmail.com Improve check to render (or not) a DRRect when inner falls outside of outer on RecordingCanvas (flutter/engine#12229) 2019-09-17 bkonyi@google.com Roll src/third_party/dart dd1969a43a..7505b3a5f0 (39 commits) 2019-09-17 30870216+gaaclarke@users.noreply.github.com Channel buffers (flutter/engine#12167) 2019-09-17 xster@google.com Make iOS FlutterViewController stop sending inactive/pause on app lifecycle events when not visible (flutter/engine#12128) 2019-09-17 stuartmorgan@google.com Adds PluginRegistry to the C++ client wrapper API (flutter/engine#12287) 2019-09-17 liyuqian@google.com Add "type" to getDisplayRefreshRate protocol (flutter/engine#12319) 2019-09-17 mouad.debbar@gmail.com Add a build command to felt (flutter/engine#12303) 2019-09-17 skia-flutter-autoroll@skia.org Roll src/third_party/skia df432d5efb70..d545bfbb94ca (1 commits) (flutter/engine#12316) 2019-09-17 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from rwf0-... to RRgw-... (flutter/engine#12315) 2019-09-17 skia-flutter-autoroll@skia.org Roll src/third_party/skia f8486f2c5fb6..df432d5efb70 (1 commits) (flutter/engine#12313) 2019-09-17 skia-flutter-autoroll@skia.org Roll src/third_party/skia b47704b0bd34..f8486f2c5fb6 (2 commits) (flutter/engine#12312) 2019-09-16 jason-simmons@users.noreply.github.com Fix the declaration of setSystemGestureExclusionRects to match the PlatformMessageHandler interface (flutter/engine#12306) 2019-09-16 gw280@google.com Manage resource and onscreen contexts using separate IOSGLContext objects (flutter/engine#12277) 2019-09-16 goderbauer@google.com Cleanup in web_ui (flutter/engine#12307) 2019-09-16 30870216+gaaclarke@users.noreply.github.com Made flutter startup faster by allowing initialization to be parallelized (flutter/engine#10182) 2019-09-16 skia-flutter-autoroll@skia.org Roll src/third_party/skia c22498502cda..b47704b0bd34 (16 commits) (flutter/engine#12304) 2019-09-16 jonahwilliams@google.com Include firefox in check to quote font families (flutter/engine#12288) 2019-09-16 bkonyi@google.com 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 stuartmorgan@google.com 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

…text objects (flutter#12277)" This reverts commit 5a8da65.

…text objects (#12277)" (#12773) This reverts commit 5a8da65.

Manage resource and onscreen contexts using separate IOSGLContext obj…

282f968

…ects. FlutterView owns the onscreen context, and PlatformViewIOS owns the resource context.

googlebot added the cla: yes label Sep 13, 2019

gw280 requested a review from dnfield September 13, 2019 21:09

gw280 commented Sep 13, 2019

View reviewed changes

dnfield approved these changes Sep 13, 2019

View reviewed changes

Create ios_surface_ if we don't have one but still have a viewcontroller

2439185

dnfield approved these changes Sep 16, 2019

View reviewed changes

code formatting

d318537

gw280 merged commit 5a8da65 into flutter:master Sep 16, 2019

xster mentioned this pull request Sep 17, 2019

Calling PlatformView.NotifyDestroyed still leaks IOSurfaces flutter/flutter#38736

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 17, 2019

5a8da65 Manage resource and onscreen contexts using separate IOSGLCon…

6fb3f9a

…text objects (flutter/engine#12277)

engine-flutter-autoroll mentioned this pull request Sep 17, 2019

Roll engine 63873d9f421f..d1692d4cc703 (22 commits) flutter/flutter#40717

Merged

gw280 mentioned this pull request Sep 19, 2019

Do not create an onscreen surface when foregrounding if no FlutterViewController is visible flutter/flutter#40872

Open

gaaclarke added a commit to gaaclarke/engine that referenced this pull request Oct 3, 2019

Revert "Manage resource and onscreen contexts using separate IOSGLCon…

65342cb

…text objects (flutter#12277)" This reverts commit 5a8da65.

gaaclarke mentioned this pull request Oct 3, 2019

Revert "Manage resource and onscreen contexts using separate IOSGLCon… #12773

Merged

chinmaygarde pushed a commit that referenced this pull request Oct 3, 2019

Revert "Manage resource and onscreen contexts using separate IOSGLCon…

142cf00

…text objects (#12277)" (#12773) This reverts commit 5a8da65.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Manage resource and onscreen contexts using separate IOSGLContext objects #12277

Manage resource and onscreen contexts using separate IOSGLContext objects #12277

Uh oh!

gw280 commented Sep 13, 2019

Uh oh!

gw280 Sep 13, 2019

Uh oh!

dnfield left a comment

Uh oh!

dnfield left a comment

Uh oh!

gw280 commented Sep 16, 2019

Uh oh!

xster commented Sep 16, 2019

Uh oh!

gw280 commented Sep 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Manage resource and onscreen contexts using separate IOSGLContext objects #12277

Manage resource and onscreen contexts using separate IOSGLContext objects #12277

Uh oh!

Conversation

gw280 commented Sep 13, 2019

Uh oh!

gw280 Sep 13, 2019

Choose a reason for hiding this comment

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

gw280 commented Sep 16, 2019

Uh oh!

xster commented Sep 16, 2019

Uh oh!

gw280 commented Sep 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants