Establish mojo service between Canvas (blink) and SurfaceManager (browser)

third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeTest.cpp

Index: third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeTest.cpp
diff --git a/third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeTest.cpp b/third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeTest.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..61e9d599ba9a2b27da5093cd7c78170fffe5f5f3
--- /dev/null
+++ b/third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeTest.cpp
@@ -0,0 +1,126 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "platform/graphics/CanvasSurfaceLayerBridge.h"
+
+#include "cc/surfaces/surface_id.h"
+#include "cc/surfaces/surface_sequence.h"
+#include "platform/graphics/CanvasSurfaceLayerBridgeClient.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "wtf/OwnPtr.h"
+#include "wtf/PassOwnPtr.h"
+#include "wtf/Vector.h"
+
+namespace blink {
+
+class FakeOffscreenCanvasSurfaceServiceImpl {
+public:
+    FakeOffscreenCanvasSurfaceServiceImpl() {}
+    ~FakeOffscreenCanvasSurfaceServiceImpl();
+
+    bool GetSurfaceId(cc::SurfaceId*);
+    void RequestSurfaceCreation(const cc::SurfaceId&);
+
+    bool isSurfaceInSurfaceMap(const cc::SurfaceId&);
+
+private:
+    Vector<cc::SurfaceId> m_fakeSurfaceMap;
+};
+
+//-----------------------------------------------------------------------------
+
+class MockCanvasSurfaceLayerBridgeClient final : public CanvasSurfaceLayerBridgeClient {
+public:
+    explicit MockCanvasSurfaceLayerBridgeClient(FakeOffscreenCanvasSurfaceServiceImpl*);
+    ~MockCanvasSurfaceLayerBridgeClient() override;
+
+    bool syncGetSurfaceId(cc::SurfaceId*) override;
+    void asyncRequestSurfaceCreation(const cc::SurfaceId&) override;
+    void asyncRequire(cc::SurfaceId, cc::SurfaceSequence) override {}
+    void asyncSatisfy(cc::SurfaceSequence) override {};
+
+private:
+    FakeOffscreenCanvasSurfaceServiceImpl* m_service;
+    cc::SurfaceId m_surfaceId;
+};
+
+//-----------------------------------------------------------------------------
+
+class CanvasSurfaceLayerBridgeTest : public testing::Test {
+public:
+    CanvasSurfaceLayerBridge* surfaceLayerBridge() const { return m_surfaceLayerBridge.get(); }
+    FakeOffscreenCanvasSurfaceServiceImpl* surfaceService() const { return m_surfaceService.get(); }
+
+protected:
+    void SetUp() override;
+    void TearDown() override;
+
+private:
+    OwnPtr<CanvasSurfaceLayerBridge> m_surfaceLayerBridge;
+    OwnPtr<FakeOffscreenCanvasSurfaceServiceImpl> m_surfaceService;

[danakj, 2016/06/17 00:33:28]

Since the m_surfaceLayerBridge has a pointer to this m_surfaceService, you
should destroy it after the m_surfaceLayerBridge (aka put this above it)

[xlai (Olivia), 2016/06/17 16:04:07]

Actually m_surfaceLayerBridge itself doesn't contain a pointer to this
m_surfaceService.
Only its client keeps a pointer to it (rmb we had the discussion of separating
bridge
and bridgeclient to isolate all IPC-related stuffs out into bridgeclient?)
Also, I did set the pointer to nullptr in client's destructor. In addition,
since this
service is owned by test, I should let test destroy it and not other entities.

[danakj, 2016/06/17 17:53:03]

Sorry, what I mean is that on line 111-112 we have:

OwnPtr<CanvasSurfaceLayerBridgeClient> bridgeClient = adoptPtr(
    new MockCanvasSurfaceLayerBridgeClient(m_surfaceService.get()));
m_surfaceLayerBridge = adoptPtr(new CanvasSurfaceLayerBridge(
    std::move(bridgeClient)));

That puts an m_surfaceService* into MockCanvasSurfaceLayerBridgeClient, and
moves the mock client into the CanvasSurfaceLayerBridge, stored as
m_surfaceLayerBridge.

So that creates a dependency from m_surfaceLayerBridge on m_surfaceService. If
m_surfaceService is destroyed first, there is a dangling pointer somewhere in
the system briefly.
The MockCanvasSurfaceLayerBridgeClient only has a raw pointer, it can't use its
own members after its destructor runs, so setting the raw pointer to null there
doesn't help with this situation.

The m_surfaceService will destroy first (member variables are destroyed bottom
to top), which will run ~FakeOffscreenCanvasSurfaceServiceImpl() and then set
m_surfaceService to null. Now any pointers left in the system are dangling, such
as the one we handed to the MockCanvasSurfaceLayerBridgeClient.

The next thing to happen is the m_surfaceLayerBridge is destroyed, which will
run ~CanvasSurfaceLayerBridge(). Eventually this gets to destroying the
MockCanvasSurfaceLayerBridgeClient, but there's a dangerous pointer sitting
around until it does.

So I just meant that these two OwnPtrs should have their order inverted to be
destroyed in the other order. 
I agree, it's just the ordering here. Right now nothing is using the pointer so
it doesn't crash, but if something did in the future, it wouldn't be null, so it
wouldn't necessarily be obvious things were going extremely wrong unless you
were running under ASAN or got unlucky.

[xlai (Olivia), 2016/06/17 20:47:02]

Done. Thanks! I've reversed the order of OwnPtr in Test class.

+};
+
+//-----------------------------------------------------------------------------
+
+MockCanvasSurfaceLayerBridgeClient::MockCanvasSurfaceLayerBridgeClient(FakeOffscreenCanvasSurfaceServiceImpl* surfaceService)
+{
+    m_service = surfaceService;
+}
+
+MockCanvasSurfaceLayerBridgeClient::~MockCanvasSurfaceLayerBridgeClient()
+{
+    m_service = nullptr;
+}
+
+bool MockCanvasSurfaceLayerBridgeClient::syncGetSurfaceId(cc::SurfaceId* surfaceIdPtr)
+{
+    return m_service->GetSurfaceId(surfaceIdPtr);
+}
+
+void MockCanvasSurfaceLayerBridgeClient::asyncRequestSurfaceCreation(const cc::SurfaceId& surfaceId)
+{
+    m_service->RequestSurfaceCreation(surfaceId);
+}
+
+FakeOffscreenCanvasSurfaceServiceImpl::~FakeOffscreenCanvasSurfaceServiceImpl()
+{
+    m_fakeSurfaceMap.clear();
+}
+
+bool FakeOffscreenCanvasSurfaceServiceImpl::GetSurfaceId(cc::SurfaceId* surfaceId)
+{
+    *surfaceId = cc::SurfaceId(10, 15, 0);
+    return true;
+}
+
+void FakeOffscreenCanvasSurfaceServiceImpl::RequestSurfaceCreation(const cc::SurfaceId& surfaceId)
+{
+    m_fakeSurfaceMap.append(surfaceId);
+}
+
+bool FakeOffscreenCanvasSurfaceServiceImpl::isSurfaceInSurfaceMap(const cc::SurfaceId& surfaceId)
+{
+    return m_fakeSurfaceMap.contains(surfaceId);
+}
+
+void CanvasSurfaceLayerBridgeTest::SetUp()
+{
+    m_surfaceService = adoptPtr(new FakeOffscreenCanvasSurfaceServiceImpl());
+    OwnPtr<CanvasSurfaceLayerBridgeClient> bridgeClient = adoptPtr(new MockCanvasSurfaceLayerBridgeClient(m_surfaceService.get()));
+    m_surfaceLayerBridge = adoptPtr(new CanvasSurfaceLayerBridge(std::move(bridgeClient)));
+}
+
+void CanvasSurfaceLayerBridgeTest::TearDown()
+{
+}
+
+TEST_F(CanvasSurfaceLayerBridgeTest, SurfaceLayerCreation)
+{
+    bool success = this->surfaceLayerBridge()->createSurfaceLayer(50, 50);
+    EXPECT_TRUE(this->surfaceService()->isSurfaceInSurfaceMap(this->surfaceLayerBridge()->getSurfaceId()));
+    EXPECT_TRUE(success);
+}
+
+} // namespace blink