Release Surface when calling makeCurrent with null.

Refactorings to egl::Surface to enable ref-counting were causing
a situation where we could have two Window surfaces alive at the
same time. This would confuse the window procedure logic in
SurfaceD3D. Releasing the surface fixes this issue and conforms
closely to the wording on the spec on when Surfaces should be
deleted. Also add a test for message loops and surfaces.

BUG=475085
BUG=angleproject:963

Change-Id: Icdee3a7db97c9b54d779dabf1e1f82a89fefc546
Reviewed-on: https://chromium-review.googlesource.com/265064
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Tested-by: Jamie Madill <jmadill@chromium.org>

src/libANGLE/Context.cpp

@@ -174,6 +174,8 @@ Context::~Context()
 
 void Context::makeCurrent(egl::Surface *surface)
 {
+    ASSERT(surface != nullptr);
+
     if (!mHasBeenCurrent)
     {
         initRendererString();
@@ -187,12 +189,16 @@ void Context::makeCurrent(egl::Surface *surface)
 
     // TODO(jmadill): do not allocate new pointers here
     Framebuffer *framebufferZero = new DefaultFramebuffer(mCaps, mRenderer, surface);
-
     setFramebufferZero(framebufferZero);
-
     mRenderBuffer = surface->getRenderBuffer();
 }
 
+void Context::releaseSurface()
+{
+    setFramebufferZero(nullptr);
+    mRenderBuffer = EGL_NONE;
+}
+
 // NOTE: this function should not assume that this context is current!
 void Context::markContextLost()
 {
@@ -666,17 +672,19 @@ void Context::setFramebufferZero(Framebuffer *buffer)
     // First, check to see if the old default framebuffer
     // was set for draw or read framebuffer, and change
     // the bindings to point to the new one before deleting it.
-    if (mState.getDrawFramebuffer()->id() == 0)
+    if (mState.getDrawFramebuffer() == nullptr ||
+        mState.getDrawFramebuffer()->id() == 0)
     {
         mState.setDrawFramebufferBinding(buffer);
     }
 
-    if (mState.getReadFramebuffer()->id() == 0)
+    if (mState.getReadFramebuffer() == nullptr ||
+        mState.getReadFramebuffer()->id() == 0)
     {
         mState.setReadFramebufferBinding(buffer);
     }
 
-    delete mFramebufferMap[0];
+    SafeDelete(mFramebufferMap[0]);
     mFramebufferMap[0] = buffer;
 }
 
@@ -884,7 +892,7 @@ bool Context::getIndexedIntegerv(GLenum target, GLuint index, GLint *data)
 {
     // Queries about context capabilities and maximums are answered by Context.
     // Queries about current GL state values are answered by State.
-    // Indexed integer queries all refer to current state, so this function is a 
+    // Indexed integer queries all refer to current state, so this function is a
     // mere passthrough.
     return mState.getIndexedIntegerv(target, index, data);
 }
@@ -893,7 +901,7 @@ bool Context::getIndexedInteger64v(GLenum target, GLuint index, GLint64 *data)
 {
     // Queries about context capabilities and maximums are answered by Context.
     // Queries about current GL state values are answered by State.
-    // Indexed integer queries all refer to current state, so this function is a 
+    // Indexed integer queries all refer to current state, so this function is a
     // mere passthrough.
     return mState.getIndexedInteger64v(target, index, data);
 }
@@ -1321,7 +1329,7 @@ void Context::detachTexture(GLuint texture)
 
 void Context::detachBuffer(GLuint buffer)
 {
-    // Buffer detachment is handled by Context, because the buffer must also be 
+    // Buffer detachment is handled by Context, because the buffer must also be
     // attached from any VAOs in existence, and Context holds the VAO map.
 
     // [OpenGL ES 2.0.24] section 2.9 page 22:
@@ -1365,8 +1373,8 @@ void Context::detachRenderbuffer(GLuint renderbuffer)
 
 void Context::detachVertexArray(GLuint vertexArray)
 {
-    // Vertex array detachment is handled by Context, because 0 is a valid 
-    // VAO, and a pointer to it must be passed from Context to State at 
+    // Vertex array detachment is handled by Context, because 0 is a valid
+    // VAO, and a pointer to it must be passed from Context to State at
     // binding time.
 
     // [OpenGL ES 3.0.2] section 2.10 page 43:

src/libANGLE/Context.h

@@ -63,11 +63,12 @@ class Context final : angle::NonCopyable
     virtual ~Context();
 
     void makeCurrent(egl::Surface *surface);
+    void releaseSurface();
 
     virtual void markContextLost();
     bool isContextLost();
 
-    // These create  and destroy methods are merely pass-throughs to 
+    // These create  and destroy methods are merely pass-throughs to
     // ResourceManager, which owns these object types
     GLuint createBuffer();
     GLuint createShader(GLenum type);
@@ -94,7 +95,7 @@ class Context final : angle::NonCopyable
     // NV Fences are owned by the Context.
     GLuint createFenceNV();
     void deleteFenceNV(GLuint fence);
-    
+
     // Queries are owned by the Context;
     GLuint createQuery();
     void deleteQuery(GLuint query);

src/libANGLE/Display.cpp

@@ -496,8 +496,9 @@ Error Display::makeCurrent(egl::Surface *drawSurface, egl::Surface *readSurface,
         return error;
     }
 
-    if (context && drawSurface)
+    if (context != nullptr && drawSurface != nullptr)
     {
+        ASSERT(readSurface == drawSurface);
         context->makeCurrent(drawSurface);
     }
 

src/libANGLE/State.cpp

@@ -765,7 +765,8 @@ const Framebuffer *State::getDrawFramebuffer() const
 
 bool State::removeReadFramebufferBinding(GLuint framebuffer)
 {
-    if (mReadFramebuffer->id() == framebuffer)
+    if (mReadFramebuffer != nullptr &&
+        mReadFramebuffer->id() == framebuffer)
     {
         mReadFramebuffer = NULL;
         return true;
@@ -776,7 +777,8 @@ bool State::removeReadFramebufferBinding(GLuint framebuffer)
 
 bool State::removeDrawFramebufferBinding(GLuint framebuffer)
 {
-    if (mDrawFramebuffer->id() == framebuffer)
+    if (mReadFramebuffer != nullptr &&
+        mDrawFramebuffer->id() == framebuffer)
     {
         mDrawFramebuffer = NULL;
         return true;

src/libGLESv2/entry_points_egl.cpp

@@ -512,6 +512,20 @@ EGLBoolean EGLAPIENTRY MakeCurrent(EGLDisplay dpy, EGLSurface draw, EGLSurface r
         return EGL_FALSE;
     }
 
+    if (dpy == EGL_NO_DISPLAY)
+    {
+        SetGlobalError(Error(EGL_BAD_DISPLAY, "'dpy' not a valid EGLDisplay handle"));
+        return EGL_FALSE;
+    }
+
+    // EGL 1.5 spec: dpy can be uninitialized if all other parameters are null
+    if (dpy != EGL_NO_DISPLAY && !display->isInitialized() &&
+        (ctx != EGL_NO_CONTEXT || draw != EGL_NO_SURFACE || read != EGL_NO_SURFACE))
+    {
+        SetGlobalError(Error(EGL_NOT_INITIALIZED, "'dpy' not initialized"));
+        return EGL_FALSE;
+    }
+
     if (ctx != EGL_NO_CONTEXT)
     {
         Error error = ValidateContext(display, context);
@@ -564,14 +578,20 @@ EGLBoolean EGLAPIENTRY MakeCurrent(EGLDisplay dpy, EGLSurface draw, EGLSurface r
         UNIMPLEMENTED();   // FIXME
     }
 
+    gl::Context *previousContext = GetGlobalContext();
+
     SetGlobalDisplay(display);
     SetGlobalDrawSurface(drawSurface);
     SetGlobalReadSurface(readSurface);
     SetGlobalContext(context);
 
-    if (context != nullptr && display != nullptr && drawSurface != nullptr)
+    display->makeCurrent(drawSurface, readSurface, context);
+
+    // Release the surface from the previously-current context, to allow
+    // destroyed surfaces to delete themselves.
+    if (previousContext != nullptr && context != previousContext)
     {
-        display->makeCurrent(drawSurface, readSurface, context);
+        previousContext->releaseSurface();
     }
 
     SetGlobalError(Error(EGL_SUCCESS));

src/tests/angle_end2end_tests.gypi

@@ -54,8 +54,9 @@
             '<(angle_path)/src/tests/end2end_tests/UnpackRowLength.cpp',
             '<(angle_path)/src/tests/end2end_tests/VertexAttributeTest.cpp',
             '<(angle_path)/src/tests/end2end_tests/ViewportTest.cpp',
-            '<(angle_path)/src/tests/standalone_tests/EGLThreadTest.cpp',
             '<(angle_path)/src/tests/standalone_tests/EGLQueryContextTest.cpp',
+            '<(angle_path)/src/tests/standalone_tests/EGLSurfaceTest.cpp',
+            '<(angle_path)/src/tests/standalone_tests/EGLThreadTest.cpp',
         ],
     },
     'dependencies':

src/tests/end2end_tests/ANGLETest.cpp

@@ -3,15 +3,14 @@
 #include "OSWindow.h"
 
 ANGLETest::ANGLETest(EGLint glesMajorVersion, const EGLPlatformParameters &platform)
-    : mEGLWindow(NULL)
+    : mEGLWindow(nullptr)
 {
     mEGLWindow = new EGLWindow(1280, 720, glesMajorVersion, platform);
 }
 
 ANGLETest::~ANGLETest()
 {
-    delete mEGLWindow;
-    mEGLWindow = NULL;
+    SafeDelete(mEGLWindow);
 }
 
 void ANGLETest::SetUp()
@@ -50,7 +49,10 @@ void ANGLETest::TearDown()
 
 void ANGLETest::swapBuffers()
 {
-    mEGLWindow->swap();
+    if (mEGLWindow->isGLInitialized())
+    {
+        mEGLWindow->swap();
+    }
 }
 
 void ANGLETest::drawQuad(GLuint program, const std::string& positionAttribName, GLfloat quadDepth, GLfloat quadScale)

src/tests/standalone_tests/EGLSurfaceTest.cpp

@@ -0,0 +1,226 @@
+//
+// Copyright 2015 The ANGLE Project Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// EGLSurfaceTest:
+//   Tests pertaining to egl::Surface.
+//
+
+#include <gtest/gtest.h>
+#include <EGL/egl.h>
+#include <EGL/eglext.h>
+#include <GLES2/gl2.h>
+
+#include "common/angleutils.h"
+#include "OSWindow.h"
+
+namespace
+{
+
+class EGLSurfaceTest : public testing::Test
+{
+  protected:
+    EGLSurfaceTest()
+        : mDisplay(EGL_NO_DISPLAY),
+          mWindowSurface(EGL_NO_SURFACE),
+          mPbufferSurface(EGL_NO_SURFACE),
+          mContext(EGL_NO_CONTEXT),
+          mSecondContext(EGL_NO_CONTEXT),
+          mOSWindow(nullptr)
+    {
+    }
+
+    void SetUp() override
+    {
+        mOSWindow = CreateOSWindow();
+        mOSWindow->initialize("EGLSurfaceTest", 64, 64);
+    }
+
+    // Release any resources created in the test body
+    void TearDown() override
+    {
+        mOSWindow->destroy();
+        SafeDelete(mOSWindow);
+
+        if (mDisplay != EGL_NO_DISPLAY)
+        {
+            eglMakeCurrent(mDisplay, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT);
+
+            if (mWindowSurface != EGL_NO_SURFACE)
+            {
+                eglDestroySurface(mDisplay, mWindowSurface);
+                mWindowSurface = EGL_NO_SURFACE;
+            }
+
+            if (mPbufferSurface != EGL_NO_SURFACE)
+            {
+                eglDestroySurface(mDisplay, mPbufferSurface);
+                mPbufferSurface = EGL_NO_SURFACE;
+            }
+
+            if (mContext != EGL_NO_CONTEXT)
+            {
+                eglDestroyContext(mDisplay, mContext);
+                mContext = EGL_NO_CONTEXT;
+            }
+
+            if (mSecondContext != EGL_NO_CONTEXT)
+            {
+                eglDestroyContext(mDisplay, mSecondContext);
+                mSecondContext = EGL_NO_CONTEXT;
+            }
+
+            eglTerminate(mDisplay);
+            mDisplay = EGL_NO_DISPLAY;
+        }
+
+        ASSERT_TRUE(mWindowSurface == EGL_NO_SURFACE && mContext == EGL_NO_CONTEXT);
+    }
+
+    void initializeSurface(EGLenum platformType)
+    {
+        PFNEGLGETPLATFORMDISPLAYEXTPROC eglGetPlatformDisplayEXT = reinterpret_cast<PFNEGLGETPLATFORMDISPLAYEXTPROC>(eglGetProcAddress("eglGetPlatformDisplayEXT"));
+        ASSERT_TRUE(eglGetPlatformDisplayEXT != nullptr);
+
+        const EGLint displayAttributes[] =
+        {
+            EGL_PLATFORM_ANGLE_TYPE_ANGLE, platformType,
+            EGL_PLATFORM_ANGLE_MAX_VERSION_MAJOR_ANGLE, EGL_DONT_CARE,
+            EGL_PLATFORM_ANGLE_MAX_VERSION_MINOR_ANGLE, EGL_DONT_CARE,
+            EGL_PLATFORM_ANGLE_DEVICE_TYPE_ANGLE, EGL_PLATFORM_ANGLE_DEVICE_TYPE_HARDWARE_ANGLE,
+            EGL_NONE,
+        };
+
+        mDisplay = eglGetPlatformDisplayEXT(EGL_PLATFORM_ANGLE_ANGLE, mOSWindow->getNativeDisplay(), displayAttributes);
+        ASSERT_TRUE(mDisplay != EGL_NO_DISPLAY);
+
+        EGLint majorVersion, minorVersion;
+        ASSERT_TRUE(eglInitialize(mDisplay, &majorVersion, &minorVersion) == EGL_TRUE);
+
+        eglBindAPI(EGL_OPENGL_ES_API);
+        ASSERT_TRUE(eglGetError() == EGL_SUCCESS);
+
+        const EGLint configAttributes[] =
+        {
+            EGL_RED_SIZE, EGL_DONT_CARE,
+            EGL_GREEN_SIZE, EGL_DONT_CARE,
+            EGL_BLUE_SIZE, EGL_DONT_CARE,
+            EGL_ALPHA_SIZE, EGL_DONT_CARE,
+            EGL_DEPTH_SIZE, EGL_DONT_CARE,
+            EGL_STENCIL_SIZE, EGL_DONT_CARE,
+            EGL_SAMPLE_BUFFERS, 0,
+            EGL_NONE
+        };
+
+        EGLint configCount;
+        ASSERT_TRUE(eglChooseConfig(mDisplay, configAttributes, &mConfig, 1, &configCount) || (configCount != 1) == EGL_TRUE);
+
+        std::vector<EGLint> surfaceAttributes;
+        surfaceAttributes.push_back(EGL_NONE);
+        surfaceAttributes.push_back(EGL_NONE);
+
+        // Create first window surface
+        mWindowSurface = eglCreateWindowSurface(mDisplay, mConfig, mOSWindow->getNativeWindow(), &surfaceAttributes[0]);
+        ASSERT_TRUE(eglGetError() == EGL_SUCCESS);
+
+        mPbufferSurface = eglCreatePbufferSurface(mDisplay, mConfig, &surfaceAttributes[0]);
+
+        EGLint contextAttibutes[] =
+        {
+            EGL_CONTEXT_CLIENT_VERSION, 2,
+            EGL_NONE
+        };
+
+        mContext = eglCreateContext(mDisplay, mConfig, nullptr, contextAttibutes);
+        ASSERT_TRUE(eglGetError() == EGL_SUCCESS);
+
+        mSecondContext = eglCreateContext(mDisplay, mConfig, nullptr, contextAttibutes);
+        ASSERT_TRUE(eglGetError() == EGL_SUCCESS);
+    }
+
+    void runMessageLoopTest(EGLSurface secondSurface, EGLContext secondContext)
+    {
+        eglMakeCurrent(mDisplay, mWindowSurface, mWindowSurface, mContext);
+        ASSERT_TRUE(eglGetError() == EGL_SUCCESS);
+
+        // Make a second context current
+        eglMakeCurrent(mDisplay, secondSurface, secondSurface, secondContext);
+        eglDestroySurface(mDisplay, mWindowSurface);
+
+        // Create second window surface
+        std::vector<EGLint> surfaceAttributes;
+        surfaceAttributes.push_back(EGL_NONE);
+        surfaceAttributes.push_back(EGL_NONE);
+
+        mWindowSurface = eglCreateWindowSurface(mDisplay, mConfig, mOSWindow->getNativeWindow(), &surfaceAttributes[0]);
+        ASSERT_TRUE(eglGetError() == EGL_SUCCESS);
+
+        eglMakeCurrent(mDisplay, mWindowSurface, mWindowSurface, mContext);
+        ASSERT_TRUE(eglGetError() == EGL_SUCCESS);
+
+        mOSWindow->signalTestEvent();
+        mOSWindow->messageLoop();
+        ASSERT_TRUE(mOSWindow->didTestEventFire());
+
+        // Simple operation to test the FBO is set appropriately
+        glClear(GL_COLOR_BUFFER_BIT);
+    }
+
+    EGLDisplay mDisplay;
+    EGLSurface mWindowSurface;
+    EGLSurface mPbufferSurface;
+    EGLContext mContext;
+    EGLContext mSecondContext;
+    EGLConfig mConfig;
+    OSWindow *mOSWindow;
+};
+
+// Test a surface bug where we could have two Window surfaces active
+// at one time, blocking message loops. See http://crbug.com/475085
+TEST_F(EGLSurfaceTest, MessageLoopBug)
+{
+    const char *extensionsString = eglQueryString(EGL_NO_DISPLAY, EGL_EXTENSIONS);
+    if (strstr(extensionsString, "EGL_ANGLE_platform_angle_d3d") == nullptr)
+    {
+        std::cout << "D3D Platform not supported in ANGLE";
+        return;
+    }
+
+    initializeSurface(EGL_PLATFORM_ANGLE_TYPE_D3D11_ANGLE);
+
+    runMessageLoopTest(EGL_NO_SURFACE, EGL_NO_CONTEXT);
+}
+
+// Tests the message loop bug, but with setting a second context
+// instead of null.
+TEST_F(EGLSurfaceTest, MessageLoopBugContext)
+{
+    const char *extensionsString = eglQueryString(EGL_NO_DISPLAY, EGL_EXTENSIONS);
+    if (strstr(extensionsString, "EGL_ANGLE_platform_angle_d3d") == nullptr)
+    {
+        std::cout << "D3D Platform not supported in ANGLE";
+        return;
+    }
+
+    initializeSurface(EGL_PLATFORM_ANGLE_TYPE_D3D11_ANGLE);
+
+    runMessageLoopTest(mPbufferSurface, mSecondContext);
+}
+
+// Test a bug where calling makeCurrent twice would release the surface
+TEST_F(EGLSurfaceTest, MakeCurrentTwice)
+{
+    initializeSurface(EGL_PLATFORM_ANGLE_TYPE_DEFAULT_ANGLE);
+
+    eglMakeCurrent(mDisplay, mWindowSurface, mWindowSurface, mContext);
+    ASSERT_TRUE(eglGetError() == EGL_SUCCESS);
+
+    eglMakeCurrent(mDisplay, mWindowSurface, mWindowSurface, mContext);
+    ASSERT_TRUE(eglGetError() == EGL_SUCCESS);
+
+    // Simple operation to test the FBO is set appropriately
+    glClear(GL_COLOR_BUFFER_BIT);
+}
+
+}

util/EGLWindow.cpp

@@ -229,3 +229,10 @@ void EGLWindow::destroyGL()
         mDisplay = EGL_NO_DISPLAY;
     }
 }
+
+bool EGLWindow::isGLInitialized() const
+{
+    return mSurface != EGL_NO_SURFACE &&
+           mContext != EGL_NO_CONTEXT &&
+           mDisplay != EGL_NO_DISPLAY;
+}

util/EGLWindow.h

@@ -83,6 +83,7 @@ class EGLWindow : angle::NonCopyable
 
     bool initializeGL(OSWindow *osWindow);
     void destroyGL();
+    bool isGLInitialized() const;
 
   private:
     EGLConfig mConfig;

util/Event.h

@@ -68,6 +68,7 @@ class Event
         EVENT_MOUSE_MOVED,           // The mouse cursor moved
         EVENT_MOUSE_ENTERED,         // The mouse cursor entered the area of the window
         EVENT_MOUSE_LEFT,            // The mouse cursor left the area of the window
+        EVENT_TEST,                  // Event for testing purposes
     };
 
     EventType Type;

util/OSWindow.cpp

@@ -51,3 +51,17 @@ void OSWindow::pushEvent(Event event)
 
     mEvents.push_back(event);
 }
+
+bool OSWindow::didTestEventFire()
+{
+    Event topEvent;
+    while (popEvent(&topEvent))
+    {
+        if (topEvent.Type == Event::EVENT_TEST)
+        {
+            return true;
+        }
+    }
+
+    return false;
+}

util/OSWindow.h

@@ -37,6 +37,11 @@ class OSWindow
     virtual bool resize(int width, int height) = 0;
     virtual void setVisible(bool isVisible) = 0;
 
+    virtual void signalTestEvent() = 0;
+
+    // Pops events look for the test event
+    bool didTestEventFire();
+
   protected:
     int mWidth;
     int mHeight;

util/win32/Win32Window.cpp

@@ -133,13 +133,13 @@ LRESULT CALLBACK WndProc(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam)
     {
       case WM_NCCREATE:
         {
-            LPCREATESTRUCT pCreateStruct = (LPCREATESTRUCT)lParam;
-            SetWindowLongPtr(hWnd, GWLP_USERDATA, (LONG_PTR)pCreateStruct->lpCreateParams);
+            LPCREATESTRUCT pCreateStruct = reinterpret_cast<LPCREATESTRUCT>(lParam);
+            SetWindowLongPtr(hWnd, GWLP_USERDATA, reinterpret_cast<LONG_PTR>(pCreateStruct->lpCreateParams));
             return DefWindowProcA(hWnd, message, wParam, lParam);
         }
     }
 
-    OSWindow *window = (OSWindow*)(LONG_PTR)GetWindowLongPtr(hWnd, GWLP_USERDATA);
+    Win32Window *window = reinterpret_cast<Win32Window*>(GetWindowLongPtr(hWnd, GWLP_USERDATA));
     if (window)
     {
         switch (message)
@@ -357,6 +357,14 @@ LRESULT CALLBACK WndProc(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam)
                 window->pushEvent(event);
                 break;
             }
+
+          case WM_USER:
+            {
+                Event testEvent;
+                testEvent.Type = Event::EVENT_TEST;
+                window->pushEvent(testEvent);
+                break;
+            }
         }
 
     }
@@ -563,3 +571,8 @@ void Win32Window::pushEvent(Event event)
         break;
     }
 }
+
+void Win32Window::signalTestEvent()
+{
+    PostMessage(mNativeWindow, WM_USER, 0, 0);
+}

util/win32/Win32Window.h

@@ -17,19 +17,21 @@ class Win32Window : public OSWindow
     Win32Window();
     ~Win32Window();
 
-    bool initialize(const std::string &name, size_t width, size_t height);
-    void destroy();
+    bool initialize(const std::string &name, size_t width, size_t height) override;
+    void destroy() override;
 
-    EGLNativeWindowType getNativeWindow() const;
-    EGLNativeDisplayType getNativeDisplay() const;
+    EGLNativeWindowType getNativeWindow() const override;
+    EGLNativeDisplayType getNativeDisplay() const override;
 
-    void messageLoop();
+    void messageLoop() override;
 
-    virtual void pushEvent(Event event);
+    void pushEvent(Event event) override;
 
-    void setMousePosition(int x, int y);
-    bool resize(int width, int height);
-    void setVisible(bool isVisible);
+    void setMousePosition(int x, int y) override;
+    bool resize(int width, int height) override;
+    void setVisible(bool isVisible) override;
+
+    void signalTestEvent() override;
 
   private:
     std::string mParentClassName;