Make SkGLContext lifetime more well-defined

Remove refcounting from SkGLContext.

SkGLContext is expected to behave like GrContextFactory would own
it, as implied by the GrContextFactory function.

If it is refcounted, this does not hold.

Also other use sites, such as in SkOSWindow_win (command buffer gl
object), confirm the behavior. The object is explicitly owned and
destroyed, not shared.

Also fixes potential crashes from using GL context of an abandoned
context.

Also fixes potential crashes in DM/nanobench, if the GrContext lives
longer than GLContext through internal refing of GrContext.

Moves the non-trivial implementations from GrContextFactory.h to
.cpp, just for consistency sake.

Changes pathops_unittest.gyp. The pathops_unittest uses
GrContextFactory, but did not link to its implementation. The reason
they worked was that the implementation used (constructors, destructors)
happened to be in the .h file.

This works towards being able to use command buffer and NVPR from
the SampleApp.

BUG=skia:2992
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1511773005

Review URL: https://codereview.chromium.org/1511773005

8 files changed, 125 insertions, 70 deletions

diff --git a/bench/nanobench.cpp b/bench/nanobench.cpp
index b7858983f8..827ae037b8 100644
--- a/bench/nanobench.cpp
+++ b/bench/nanobench.cpp
@@ -175,7 +175,7 @@ struct GPUTarget : public Target {
                                                          SkSurface::kNo_Budgeted, info,
                                                          this->config.samples, &props));
         this->gl = gGrFactory->getContextInfo(this->config.ctxType,
-                                              this->config.ctxOptions)->fGLContext;
+                                              this->config.ctxOptions).fGLContext;
         if (!this->surface.get()) {
             return false;
         }
diff --git a/dm/DM.cpp b/dm/DM.cpp
index ace49076ac..ba8ad926ad 100644
--- a/dm/DM.cpp
+++ b/dm/DM.cpp
@@ -1147,16 +1147,16 @@ typedef void(*TestWithGrContext)(skiatest::Reporter*, GrContext*);
 typedef void(*TestWithGrContextAndGLContext)(skiatest::Reporter*, GrContext*, SkGLContext*);
 #if SK_SUPPORT_GPU
 template<typename T>
-void call_test(T test, skiatest::Reporter* reporter, GrContextFactory::ContextInfo* context);
+void call_test(T test, skiatest::Reporter* reporter, const GrContextFactory::ContextInfo& context);
 template<>
 void call_test(TestWithGrContext test, skiatest::Reporter* reporter,
-               GrContextFactory::ContextInfo* context) {
-    test(reporter, context->fGrContext);
+               const GrContextFactory::ContextInfo& context) {
+    test(reporter, context.fGrContext);
 }
 template<>
 void call_test(TestWithGrContextAndGLContext test, skiatest::Reporter* reporter,
-               GrContextFactory::ContextInfo* context) {
-    test(reporter, context->fGrContext, context->fGLContext);
+               const GrContextFactory::ContextInfo& context) {
+    test(reporter, context.fGrContext, context.fGLContext);
 }
 #endif
 } // namespace
@@ -1202,11 +1202,13 @@ void RunWithGPUTestContexts(T test, GPUTestContexts testContexts, Reporter* repo
         if ((testContexts & contextSelector) == 0) {
             continue;
         }
-        if (GrContextFactory::ContextInfo* context = factory->getContextInfo(contextType)) {
+        GrContextFactory::ContextInfo context = factory->getContextInfo(contextType);
+        if (context.fGrContext) {
             call_test(test, reporter, context);
         }
-        if (GrContextFactory::ContextInfo* context =
-            factory->getContextInfo(contextType, GrContextFactory::kEnableNVPR_GLContextOptions)) {
+        context = factory->getContextInfo(contextType,
+                                          GrContextFactory::kEnableNVPR_GLContextOptions);
+        if (context.fGrContext) {
             call_test(test, reporter, context);
         }
     }
diff --git a/gyp/pathops_unittest.gyp b/gyp/pathops_unittest.gyp
index 9e3dbba0de..14b231c34c 100644
--- a/gyp/pathops_unittest.gyp
+++ b/gyp/pathops_unittest.gyp
@@ -41,6 +41,10 @@
           'include_dirs': [
             '../src/gpu',
           ],
+	  'sources': [
+            '../src/gpu/GrContextFactory.cpp',
+            '../src/gpu/GrContextFactory.h',
+          ]
         }],
       ],
     },
diff --git a/include/gpu/gl/SkGLContext.h b/include/gpu/gl/SkGLContext.h
index 3420a47973..7edfa730aa 100644
--- a/include/gpu/gl/SkGLContext.h
+++ b/include/gpu/gl/SkGLContext.h
@@ -17,9 +17,9 @@
  * This class is intended for Skia's testing needs and not for general
  * use.
  */
-class SK_API SkGLContext : public SkRefCnt {
+class SK_API SkGLContext : public SkNoncopyable {
 public:
-    ~SkGLContext() override;
+    virtual ~SkGLContext();
 
     bool isValid() const { return NULL != gl(); }
 
@@ -106,8 +106,6 @@ private:
     SkAutoTUnref<const GrGLInterface> fGL;
 
     friend class GLFenceSync;  // For onPlatformGetProcAddress.
-
-    typedef SkRefCnt INHERITED;
 };
 
 /** Creates platform-dependent GL context object
diff --git a/src/gpu/GrContextFactory.cpp b/src/gpu/GrContextFactory.cpp
index 4814e7870e..b7e48254c8 100755
--- a/src/gpu/GrContextFactory.cpp
+++ b/src/gpu/GrContextFactory.cpp
@@ -23,16 +23,56 @@
 #include "gl/GrGLGpu.h"
 #include "GrCaps.h"
 
-GrContextFactory::ContextInfo* GrContextFactory::getContextInfo(GLContextType type,
-                                                                GLContextOptions options) {
+GrContextFactory::GrContextFactory() { }
+
+GrContextFactory::GrContextFactory(const GrContextOptions& opts)
+    : fGlobalOptions(opts) {
+}
+
+GrContextFactory::~GrContextFactory() {
+    this->destroyContexts();
+}
+
+void GrContextFactory::destroyContexts() {
+    for (Context& context : fContexts) {
+        if (context.fGLContext) {
+            context.fGLContext->makeCurrent();
+        }
+        if (!context.fGrContext->unique()) {
+            context.fGrContext->abandonContext();
+        }
+        context.fGrContext->unref();
+        delete(context.fGLContext);
+    }
+    fContexts.reset();
+}
+
+void GrContextFactory::abandonContexts() {
+    for (Context& context : fContexts) {
+        if (context.fGLContext) {
+            context.fGLContext->makeCurrent();
+            context.fGLContext->testAbandon();
+            delete(context.fGLContext);
+            context.fGLContext = nullptr;
+        }
+        context.fGrContext->abandonContext();
+    }
+}
+
+GrContextFactory::ContextInfo GrContextFactory::getContextInfo(GLContextType type,
+                                                               GLContextOptions options) {
     for (int i = 0; i < fContexts.count(); ++i) {
-        if (fContexts[i]->fType == type &&
-            fContexts[i]->fOptions == options) {
-            fContexts[i]->fGLContext->makeCurrent();
-            return fContexts[i];
+        Context& context = fContexts[i];
+        if (!context.fGLContext) {
+            continue;
+        }
+        if (context.fType == type &&
+            context.fOptions == options) {
+            context.fGLContext->makeCurrent();
+            return ContextInfo(context.fGrContext, context.fGLContext);
         }
     }
-    SkAutoTUnref<SkGLContext> glCtx;
+    SkAutoTDelete<SkGLContext> glCtx;
     SkAutoTUnref<GrContext> grCtx;
     switch (type) {
         case kNative_GLContextType:
@@ -72,7 +112,7 @@ GrContextFactory::ContextInfo* GrContextFactory::getContextInfo(GLContextType ty
             break;
     }
     if (nullptr == glCtx.get()) {
-        return nullptr;
+        return ContextInfo();
     }
 
     SkASSERT(glCtx->isValid());
@@ -82,7 +122,7 @@ GrContextFactory::ContextInfo* GrContextFactory::getContextInfo(GLContextType ty
     if (!(kEnableNVPR_GLContextOptions & options)) {
         glInterface.reset(GrGLInterfaceRemoveNVPR(glInterface));
         if (!glInterface) {
-            return nullptr;
+            return ContextInfo();
         }
     }
 
@@ -94,18 +134,18 @@ GrContextFactory::ContextInfo* GrContextFactory::getContextInfo(GLContextType ty
     grCtx.reset(GrContext::Create(kOpenGL_GrBackend, p3dctx, fGlobalOptions));
 #endif
     if (!grCtx.get()) {
-        return nullptr;
+        return ContextInfo();
     }
     if (kEnableNVPR_GLContextOptions & options) {
         if (!grCtx->caps()->shaderCaps()->pathRenderingSupport()) {
-            return nullptr;
+            return ContextInfo();
         }
     }
 
-    ContextInfo* ctx = fContexts.emplace_back(new ContextInfo);
-    ctx->fGLContext = SkRef(glCtx.get());
-    ctx->fGrContext = SkRef(grCtx.get());
-    ctx->fType = type;
-    ctx->fOptions = options;
-    return ctx;
+    Context& context = fContexts.push_back();
+    context.fGLContext = glCtx.detach();
+    context.fGrContext = SkRef(grCtx.get());
+    context.fType = type;
+    context.fOptions = options;
+    return ContextInfo(context.fGrContext, context.fGLContext);
 }
diff --git a/src/gpu/GrContextFactory.h b/src/gpu/GrContextFactory.h
index 1df99d6ff1..7afa3108c6 100644
--- a/src/gpu/GrContextFactory.h
+++ b/src/gpu/GrContextFactory.h
@@ -98,59 +98,47 @@ public:
         }
     }
 
-    explicit GrContextFactory(const GrContextOptions& opts) : fGlobalOptions(opts) { }
-    GrContextFactory() { }
+    explicit GrContextFactory(const GrContextOptions& opts);
+    GrContextFactory();
 
-    ~GrContextFactory() { this->destroyContexts(); }
+    ~GrContextFactory();
 
-    void destroyContexts() {
-        for (int i = 0; i < fContexts.count(); ++i) {
-            if (fContexts[i]->fGLContext) {  //  could be abandoned.
-                fContexts[i]->fGLContext->makeCurrent();
-            }
-            fContexts[i]->fGrContext->unref();
-            SkSafeUnref(fContexts[i]->fGLContext);
-        }
-        fContexts.reset();
-    }
-
-    void abandonContexts() {
-        for (int i = 0; i < fContexts.count(); ++i) {
-            if (fContexts[i]->fGLContext) {
-                fContexts[i]->fGLContext->testAbandon();
-                SkSafeSetNull(fContexts[i]->fGLContext);
-            }
-            fContexts[i]->fGrContext->abandonContext();
-        }
-    }
+    void destroyContexts();
+    void abandonContexts();
 
     struct ContextInfo {
-        GLContextType             fType;
-        GLContextOptions          fOptions;
-        SkGLContext*              fGLContext;
-        GrContext*                fGrContext;
+        ContextInfo()
+            : fGrContext(nullptr), fGLContext(nullptr) { }
+        ContextInfo(GrContext* grContext, SkGLContext* glContext)
+            : fGrContext(grContext), fGLContext(glContext) { }
+        GrContext* fGrContext;
+        SkGLContext* fGLContext; //! Valid until the factory destroys it via abandonContexts() or
+                                 //! destroyContexts().
     };
+
     /**
      * Get a context initialized with a type of GL context. It also makes the GL context current.
-     * Pointer is valid until destroyContexts() is called.
      */
-    ContextInfo* getContextInfo(GLContextType type,
-                                GLContextOptions options = kNone_GLContextOptions);
-
+    ContextInfo getContextInfo(GLContextType type,
+                               GLContextOptions options = kNone_GLContextOptions);
     /**
      * Get a GrContext initialized with a type of GL context. It also makes the GL context current.
      */
-    GrContext* get(GLContextType type, GLContextOptions options = kNone_GLContextOptions) {
-        if (ContextInfo* info = this->getContextInfo(type, options)) {
-            return info->fGrContext;
-        }
-        return nullptr;
+    GrContext* get(GLContextType type,
+                   GLContextOptions options = kNone_GLContextOptions) {
+        return this->getContextInfo(type, options).fGrContext;
     }
     const GrContextOptions& getGlobalOptions() const { return fGlobalOptions; }
 
 private:
-    SkTArray<SkAutoTDelete<ContextInfo>, true> fContexts;
-    const GrContextOptions        fGlobalOptions;
+    struct Context {
+        GLContextType fType;
+        GLContextOptions fOptions;
+        SkGLContext*  fGLContext;
+        GrContext*    fGrContext;
+    };
+    SkTArray<Context, true> fContexts;
+    const GrContextOptions  fGlobalOptions;
 };
 
 #endif
diff --git a/tests/EGLImageTest.cpp b/tests/EGLImageTest.cpp
index b5754d8eb5..fdae3b32d1 100644
--- a/tests/EGLImageTest.cpp
+++ b/tests/EGLImageTest.cpp
@@ -28,7 +28,6 @@ static void cleanup(SkGLContext* glctx0, GrGLuint texID0, SkGLContext* glctx1, G
         if (GR_EGL_NO_IMAGE != image1) {
             glctx1->destroyEGLImage(image1);
         }
-        glctx1->unref();
     }
 
     glctx0->makeCurrent();
@@ -91,7 +90,7 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(EGLImageTest, reporter, context0, glCtx0) {
         return;
     }
 
-    SkGLContext* glCtx1 = glCtx0->createNew();
+    SkAutoTDelete<SkGLContext> glCtx1 = glCtx0->createNew();
     if (!glCtx1) {
         return;
     }
diff --git a/tests/GrContextFactoryTest.cpp b/tests/GrContextFactoryTest.cpp
index 1b19ac68e3..7fe3b50ff8 100644
--- a/tests/GrContextFactoryTest.cpp
+++ b/tests/GrContextFactoryTest.cpp
@@ -46,4 +46,28 @@ DEF_GPUTEST(GrContextFactory_NoPathRenderingUnlessNVPRRequested, reporter, /*fac
     }
 }
 
+DEF_GPUTEST(GrContextFactory_abandon, reporter, /*factory*/) {
+    GrContextFactory testFactory;
+    for (int i = 0; i < GrContextFactory::kGLContextTypeCnt; ++i) {
+        GrContextFactory::GLContextType glCtxType = (GrContextFactory::GLContextType) i;
+        GrContextFactory::ContextInfo info1 =
+                testFactory.getContextInfo(glCtxType);
+        REPORTER_ASSERT(reporter, info1.fGrContext);
+        REPORTER_ASSERT(reporter, info1.fGLContext);
+         // Ref for comparison. The API does not explicitly say that this stays alive.
+        info1.fGrContext->ref();
+        testFactory.abandonContexts();
+
+        // Test that we get different context after abandon.
+        GrContextFactory::ContextInfo info2 =
+                testFactory.getContextInfo(glCtxType);
+        REPORTER_ASSERT(reporter, info2.fGrContext);
+        REPORTER_ASSERT(reporter, info2.fGLContext);
+        REPORTER_ASSERT(reporter, info1.fGrContext != info2.fGrContext);
+        // fGLContext should also change, but it also could get the same address.
+
+        info1.fGrContext->unref();
+    }
+}
+
 #endif