1049957 - Use UniquePtr for ownership in GLScreenBuffer, SharedSurface, SurfaceFactory, and SurfaceStream

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Kelsey Gilbert [:jgilbert]]

We should use UniquePtr to establish and track ownership.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Yes please.

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 0: Increase MakeUnique max params

Comment 3

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 1: GLScreenBuffer

Comment 4

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 2: SurfaceStream

Comment 5

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3: SharedSurface, SurfaceFactory

Comment 6

[u480271]

Comment on attachment 8468888 [details] [diff] [review]
patch 1: GLScreenBuffer

Review of attachment 8468888 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/GLScreenBuffer.cpp
@@ +606,5 @@
>  
>      if (surf->mAttachType == AttachmentType::Screen) {
>          // Don't need anything. Our read buffer will be the 'screen'.
>  
> +        return UniquePtr<ReadBuffer>( new ReadBuffer(gl, 0, 0, 0,

Not a fan of spaces after ( but I understand the reason.
Is it not possible to use MakeUnique? I suppose not, since contructor params.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8468888 [details] [diff] [review]
patch 1: GLScreenBuffer

Review of attachment 8468888 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/GLScreenBuffer.cpp
@@ +66,5 @@
>  
> +    UniquePtr<GLScreenBuffer> ret( new GLScreenBuffer(gl, caps,
> +                                                      Move(factory),
> +                                                      stream) );
> +    return Move(ret);

Could just

  return MakeUnique<GLScreenBuffer>(gl, caps, Move(factor), stream);

to avoid a local, a move out of it, and a bunch of repetition.

@@ +568,5 @@
>      gl->fGenFramebuffers(1, &fb);
>      gl->AttachBuffersToFB(0, colorMSRB, depthRB, stencilRB, fb);
>  
> +    UniquePtr<DrawBuffer> ret( new DrawBuffer(gl, size, fb, colorMSRB,
> +                                              depthRB, stencilRB) );

auto ret = MakeUnique<DrawBuffer>(gl, size, fb, colorMSRB, depthRB, stencilRB); might be nicer.

@@ +607,5 @@
>      if (surf->mAttachType == AttachmentType::Screen) {
>          // Don't need anything. Our read buffer will be the 'screen'.
>  
> +        return UniquePtr<ReadBuffer>( new ReadBuffer(gl, 0, 0, 0,
> +                                                     surf) );

return MakeUnique<ReadBuffer>(gl, 0, 0, 0, surf);

@@ +642,5 @@
>      gl->AttachBuffersToFB(colorTex, colorRB, depthRB, stencilRB, fb, target);
>      gl->mFBOMapping[fb] = surf;
>  
> +    UniquePtr<ReadBuffer> ret( new ReadBuffer(gl, fb, depthRB,
> +                                              stencilRB, surf) );

auto ret = MakeUnique<ReadBuffer>(gl, fb, depthRB, stencilRB, surf);

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 8468890 [details] [diff] [review]
patch 3: SharedSurface, SurfaceFactory

Review of attachment 8468890 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/GLScreenBuffer.cpp
@@ +36,5 @@
>      {
>          return nullptr;
>      }
>  
> +    UniquePtr<SurfaceFactory> factory = nullptr;

Explicit initialization isn't necessary, it'll auto-init to nullptr if you're fine with that.

::: gfx/gl/GLScreenBuffer.h
@@ +126,5 @@
>      GLContext* const mGL; // Owns us.
>  public:
>      const SurfaceCaps mCaps;
>  protected:
> +    UniquePtr<SurfaceFactory> mFactory; // Owned by us.

UniquePtr makes explicit what the comment says, seems to me.  Same for the others.  But up to you.

::: gfx/gl/SharedSurface.cpp
@@ +259,1 @@
>          mScraps.pop();

If this queue already contains UniquePtr, popping out of it will move out the UniquePtr, and then that temporary that's created by the pop() call will destroy the contained surface for you.  No need for a local at all here, if the queue contains UniquePtr.

@@ +269,1 @@
>          mScraps.pop();

UniquePtr<SharedSurface> cur = mScraps.pop();

@@ +286,4 @@
>  
>      if (surf->mType == mType) {
> +        mScraps.push(Move(surf));
> +        MOZ_ASSERT(!surf);

You're not passing in a reference any more, so whatever the state of |surf| at this point, nobody can observe it, so who cares?  And to the extent null matters for double-deletion, that's inherent in UniquePtr, not really worth asserting, to me.

::: gfx/gl/SharedSurfaceANGLE.cpp
@@ +200,5 @@
>      }
>  
> +    typedef SharedSurface_ANGLEShareHandle ptrT;
> +    return UniquePtr<ptrT>( new ptrT(gl, egl, size, hasAlpha, context,
> +                                     pbuffer, shareHandle) );

return MakeUnique<ptrT>(gl, egl, size, hasAlpha, context, pbuffer, sharedHandle);

@@ +218,5 @@
> +        return nullptr;
> +    }
> +
> +    typedef SurfaceFactory_ANGLEShareHandle ptrT;
> +    return UniquePtr<ptrT>( new ptrT(gl, egl, caps) );

return MakeUnique<ptrT>(gl, egl, caps);

::: gfx/gl/SharedSurfaceEGL.cpp
@@ +47,5 @@
>      }
>  
> +    typedef SharedSurface_EGLImage ptrT;
> +    return UniquePtr<ptrT>( new ptrT(prodGL, egl, size, hasAlpha,
> +                                     formats, prodTex, image) );

MakeUnique

@@ +202,5 @@
>          return nullptr;
>      }
>  
> +    typedef SurfaceFactory_EGLImage ptrT;
> +    return UniquePtr<ptrT>( new ptrT(prodGL, context, caps) );

MakeUnique

::: gfx/gl/SharedSurfaceGL.cpp
@@ +46,5 @@
>          MOZ_CRASH("Unhandled Tex format.");
>      }
> +
> +    typedef SharedSurface_Basic ptrT;
> +    return UniquePtr<ptrT>( new ptrT(gl, size, hasAlpha, format, tex) );

MakeUnique

@@ +128,5 @@
>      }
>  
> +    typedef SharedSurface_GLTexture ptrT;
> +    return UniquePtr<ptrT>( new ptrT(prodGL, consGL, size, hasAlpha,
> +                                     tex, ownsTex) );

MakeUnique

::: gfx/gl/SharedSurfaceGralloc.cpp
@@ +112,5 @@
>  
> +
> +    typedef SharedSurface_Gralloc ptrT;
> +    UniquePtr<ptrT> surf( new ptrT(prodGL, size, hasAlpha, egl,
> +                                   allocator, grallocTC, prodTex) );

auto surf = MakeUnique<ptrT>(...);

::: gfx/gl/SharedSurfaceIO.cpp
@@ +23,5 @@
>  
>      gfx::IntSize size(surface->GetWidth(), surface->GetHeight());
> +
> +    typedef SharedSurface_IOSurface ptrT;
> +    return UniquePtr<ptrT>( new ptrT(ioSurf, gl, size, hasAlpha) );

MakeUnique

@@ +148,5 @@
>      gfx::IntSize maxDims(MacIOSurface::GetMaxWidth(),
>                           MacIOSurface::GetMaxHeight());
> +
> +    typedef SurfaceFactory_IOSurface ptrT;
> +    return UniquePtr<ptrT>( new ptrT(gl, caps, maxDims) );

MakeUnique

::: gfx/gl/SurfaceStream.cpp
@@ +87,5 @@
>      if (surf) {
>          // Before next use, wait until SharedSurface's buffer
>          // is no longer being used.
>          surf->WaitForBufferOwnership();
> +        mSurfaces.insert(surf.get());

This...reads very dodgy.  Perhaps all the lifetimes work out all right, but this sort of shared-ownership of pointers thing is not really what UniquePtr was designed for at all.  I would hesitate to do this if it were me.

@@ +103,5 @@
> +    UniquePtr<SharedSurface>& to = *slotTo;
> +
> +    MOZ_ASSERT(!to);
> +    to = Move(from);
> +    from = nullptr;

Assigning a Move(uniqueptr) into a UniquePtr, nulls out uniqueptr.  This second line is unnecessary.

@@ +132,3 @@
>          surf = nullptr;
>      }
>      MOZ_ASSERT(!surf);

This assertion is vacuously correct now.

@@ +146,5 @@
> +
> +    UniquePtr<SharedSurface> ret = Move(surf);
> +    surf = nullptr;
> +
> +    return Move(ret);

These three lines are equivalent to

  return Move(surf);

@@ +164,5 @@
> +
> +    UniquePtr<SharedSurface> ret = Move(surf);
> +    surf = nullptr;
> +
> +    return Move(ret);

And these are

  return Move(surf);

@@ +179,3 @@
>          surf = nullptr;
>      }
>      MOZ_ASSERT(!surf);

!surf is similarly obvious here, if push() always succeeds.

@@ +190,1 @@
>          mScraps.pop();

UniquePtr<SharedSurface> cur = mScraps.pop();

@@ +217,1 @@
>          mScraps.pop();

UniquePtr<SharedSurface> cur = mScraps.pop();

::: gfx/layers/client/ClientCanvasLayer.cpp
@@ +98,5 @@
>              // Well, this *should* work...
>  #ifdef XP_MACOSX
>              factory = SurfaceFactory_IOSurface::Create(mGLContext, caps);
>  #else
> +            factory = MakeUnique<SurfaceFactory_GLTexture>(mGLContext, nullptr, caps);

As long as gcc < 4.6 is a thing, you can't pass plain nullptr here.  0 will work (...I think), or you can just cast to the explicit pointer type.

Comment 9

[Jeff Walden [:Waldo]]

Of course this was all explained to me just the other day, but the places that don't use MakeUnique, probably don't because the relevant constructor is private/protected/not accessible to MakeUnique.  It might or might not be possible to friend that particular MakeUnique template arity, not sure, but at the least it's tricky.  :-\

And also pop() may not return the element from the queue/vector/whatever with stl, in which case the current code is more the right thing.  Although it's kind of a sad right thing, seems to me, that you can't extract all in one go but rather have to mutate the front/top element, then pop it in a second go.

Comment 10

[u480271]

Comment on attachment 8468890 [details] [diff] [review]
patch 3: SharedSurface, SurfaceFactory

Review of attachment 8468890 [details] [diff] [review]:
-----------------------------------------------------------------

If my questions are unfounded then an easy r+ but I'd like to see the reasons recorded.

::: gfx/gl/GLScreenBuffer.cpp
@@ +54,5 @@
>          factory = SurfaceFactory_IOSurface::Create(gl, caps);
>      }
>  #endif
>  
> +    if (!factory) {

Are the extra { }'s necessary?

@@ +371,4 @@
>  {
>      MOZ_ASSERT(mStream);
>  
>      if (newFactory) {

ditto

::: gfx/gl/GLScreenBuffer.h
@@ +174,5 @@
>          return mStream;
>      }
>  
>      SurfaceFactory* Factory() const {
> +        return mFactory.get();

Just an observation and rumination: This leaks the pointer from the unique ptr. So it's possible that some one could keep the raw pointer and pass it around and possibly delete it.

::: gfx/gl/SharedSurfaceANGLE.cpp
@@ +199,5 @@
>          return nullptr;
>      }
>  
> +    typedef SharedSurface_ANGLEShareHandle ptrT;
> +    return UniquePtr<ptrT>( new ptrT(gl, egl, size, hasAlpha, context,

Does that need a Move() like the others?

@@ +218,5 @@
> +        return nullptr;
> +    }
> +
> +    typedef SurfaceFactory_ANGLEShareHandle ptrT;
> +    return UniquePtr<ptrT>( new ptrT(gl, egl, caps) );

Move()?

::: gfx/gl/SurfaceStream.cpp
@@ +87,5 @@
>      if (surf) {
>          // Before next use, wait until SharedSurface's buffer
>          // is no longer being used.
>          surf->WaitForBufferOwnership();
> +        mSurfaces.insert(surf.get());

Is it dangerous to be extracting and storing a raw pointer?

@@ +146,5 @@
> +
> +    UniquePtr<SharedSurface> ret = Move(surf);
> +    surf = nullptr;
> +
> +    return Move(ret);

Can't this just be return Move(surf); instead of moving into ret?

I'm basing this suggestion upon this code fragment from Waldo's blog post:

    int* i = new int(37);
    UniquePtr<int> i1(i);
    UniquePtr<int> i2(Move(i1));
    assert(i1 == nullptr);
    assert(i2.get() == i);

@@ +159,5 @@
> +    UniquePtr<SharedSurface>& surf = *surfSlot;
> +
> +    if (surf) {
> +        mSurfaces.insert(surf.get());
> +    }

remove extra {}s

@@ +164,5 @@
> +
> +    UniquePtr<SharedSurface> ret = Move(surf);
> +    surf = nullptr;
> +
> +    return Move(ret);

Same comment above.

@@ +179,1 @@
>          surf = nullptr;

Isn't surf == nullptr because of Move(surf)?

@@ +402,3 @@
>  
>          if (mProducer && mStaging->mSize == mProducer->mSize)
> +            SharedSurface::ProdCopy(mStaging.get(), mProducer.get(), factory);

Perhaps in this case add extra {}s to show that the else is for the outer if. On first glance, I thought the else was incorrect indented.

@@ +498,1 @@
>          }

Does WaitForCompositor update mStaging?

Why not?
if (mStaging) {
    WaitForCompositor();
    Scrap(&mStaging);
}

Comment 11

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #6)
> Comment on attachment 8468888 [details] [diff] [review]
> patch 1: GLScreenBuffer
> 
> Review of attachment 8468888 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLScreenBuffer.cpp
> @@ +606,5 @@
> >  
> >      if (surf->mAttachType == AttachmentType::Screen) {
> >          // Don't need anything. Our read buffer will be the 'screen'.
> >  
> > +        return UniquePtr<ReadBuffer>( new ReadBuffer(gl, 0, 0, 0,
> 
> Not a fan of spaces after ( but I understand the reason.
> Is it not possible to use MakeUnique? I suppose not, since contructor params.

All of the static Create() helpers that have protected/private ctors can't use MakeUnique, because MakeUnique isn't a friend of the class. (accessing protected/private function errors)

Comment 12

[Kelsey Gilbert [:jgilbert]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> Comment on attachment 8468888 [details] [diff] [review]
> patch 1: GLScreenBuffer
> 
> Review of attachment 8468888 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLScreenBuffer.cpp
> @@ +66,5 @@
> >  
> > +    UniquePtr<GLScreenBuffer> ret( new GLScreenBuffer(gl, caps,
> > +                                                      Move(factory),
> > +                                                      stream) );
> > +    return Move(ret);
> 
> Could just
> 
>   return MakeUnique<GLScreenBuffer>(gl, caps, Move(factor), stream);
> 
> to avoid a local, a move out of it, and a bunch of repetition.
GLScreenBuffer ctor is protected, so MakeUnique fails to access it. ([1])
> 
> @@ +568,5 @@
> >      gl->fGenFramebuffers(1, &fb);
> >      gl->AttachBuffersToFB(0, colorMSRB, depthRB, stencilRB, fb);
> >  
> > +    UniquePtr<DrawBuffer> ret( new DrawBuffer(gl, size, fb, colorMSRB,
> > +                                              depthRB, stencilRB) );
> 
> auto ret = MakeUnique<DrawBuffer>(gl, size, fb, colorMSRB, depthRB,
> stencilRB); might be nicer.
[1]
> 
> @@ +607,5 @@
> >      if (surf->mAttachType == AttachmentType::Screen) {
> >          // Don't need anything. Our read buffer will be the 'screen'.
> >  
> > +        return UniquePtr<ReadBuffer>( new ReadBuffer(gl, 0, 0, 0,
> > +                                                     surf) );
> 
> return MakeUnique<ReadBuffer>(gl, 0, 0, 0, surf);
[1]
> 
> @@ +642,5 @@
> >      gl->AttachBuffersToFB(colorTex, colorRB, depthRB, stencilRB, fb, target);
> >      gl->mFBOMapping[fb] = surf;
> >  
> > +    UniquePtr<ReadBuffer> ret( new ReadBuffer(gl, fb, depthRB,
> > +                                              stencilRB, surf) );
> 
> auto ret = MakeUnique<ReadBuffer>(gl, fb, depthRB, stencilRB, surf);
[1]

Comment 13

[u480271]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #9)
> And also pop() may not return the element from the queue/vector/whatever
> with stl, in which case the current code is more the right thing.  Although
> it's kind of a sad right thing, seems to me, that you can't extract all in
> one go but rather have to mutate the front/top element, then pop it in a
> second go.

STL pop() doesn't return the popped value, which is sucky.

Comment 14

[Kelsey Gilbert [:jgilbert]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8)
> Comment on attachment 8468890 [details] [diff] [review]
> patch 3: SharedSurface, SurfaceFactory
> 
> Review of attachment 8468890 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLScreenBuffer.cpp
> @@ +36,5 @@
> >      {
> >          return nullptr;
> >      }
> >  
> > +    UniquePtr<SurfaceFactory> factory = nullptr;
> 
> Explicit initialization isn't necessary, it'll auto-init to nullptr if
> you're fine with that.
Right, change.
> 
> ::: gfx/gl/GLScreenBuffer.h
> @@ +126,5 @@
> >      GLContext* const mGL; // Owns us.
> >  public:
> >      const SurfaceCaps mCaps;
> >  protected:
> > +    UniquePtr<SurfaceFactory> mFactory; // Owned by us.
> 
> UniquePtr makes explicit what the comment says, seems to me.  Same for the
> others.  But up to you.
Agreed, changed.
> 
> ::: gfx/gl/SharedSurface.cpp
> @@ +259,1 @@
> >          mScraps.pop();
> 
> If this queue already contains UniquePtr, popping out of it will move out
> the UniquePtr, and then that temporary that's created by the pop() call will
> destroy the contained surface for you.  No need for a local at all here, if
> the queue contains UniquePtr.
std::queue's pop() doesn't work this way. It's `void std::queue::pop()`. We need to use `front()` to access the top of the queue.
> 
> @@ +269,1 @@
> >          mScraps.pop();
> 
> UniquePtr<SharedSurface> cur = mScraps.pop();
> 
> @@ +286,4 @@
> >  
> >      if (surf->mType == mType) {
> > +        mScraps.push(Move(surf));
> > +        MOZ_ASSERT(!surf);
> 
> You're not passing in a reference any more, so whatever the state of |surf|
> at this point, nobody can observe it, so who cares?  And to the extent null
> matters for double-deletion, that's inherent in UniquePtr, not really worth
> asserting, to me.
Right, changed.
> 
> ::: gfx/gl/SharedSurfaceANGLE.cpp
> @@ +200,5 @@
> >      }
> >  
> > +    typedef SharedSurface_ANGLEShareHandle ptrT;
> > +    return UniquePtr<ptrT>( new ptrT(gl, egl, size, hasAlpha, context,
> > +                                     pbuffer, shareHandle) );
> 
> return MakeUnique<ptrT>(gl, egl, size, hasAlpha, context, pbuffer,
> sharedHandle);
Class has a protected ctor.[1]
> 
> @@ +218,5 @@
> > +        return nullptr;
> > +    }
> > +
> > +    typedef SurfaceFactory_ANGLEShareHandle ptrT;
> > +    return UniquePtr<ptrT>( new ptrT(gl, egl, caps) );
> 
> return MakeUnique<ptrT>(gl, egl, caps);
[1]
> 
> ::: gfx/gl/SharedSurfaceEGL.cpp
> @@ +47,5 @@
> >      }
> >  
> > +    typedef SharedSurface_EGLImage ptrT;
> > +    return UniquePtr<ptrT>( new ptrT(prodGL, egl, size, hasAlpha,
> > +                                     formats, prodTex, image) );
> 
> MakeUnique
[1]
> 
> @@ +202,5 @@
> >          return nullptr;
> >      }
> >  
> > +    typedef SurfaceFactory_EGLImage ptrT;
> > +    return UniquePtr<ptrT>( new ptrT(prodGL, context, caps) );
> 
> MakeUnique
[1]
> 
> ::: gfx/gl/SharedSurfaceGL.cpp
> @@ +46,5 @@
> >          MOZ_CRASH("Unhandled Tex format.");
> >      }
> > +
> > +    typedef SharedSurface_Basic ptrT;
> > +    return UniquePtr<ptrT>( new ptrT(gl, size, hasAlpha, format, tex) );
> 
> MakeUnique
[1]
> 
> @@ +128,5 @@
> >      }
> >  
> > +    typedef SharedSurface_GLTexture ptrT;
> > +    return UniquePtr<ptrT>( new ptrT(prodGL, consGL, size, hasAlpha,
> > +                                     tex, ownsTex) );
> 
> MakeUnique
[1]
> 
> ::: gfx/gl/SharedSurfaceGralloc.cpp
> @@ +112,5 @@
> >  
> > +
> > +    typedef SharedSurface_Gralloc ptrT;
> > +    UniquePtr<ptrT> surf( new ptrT(prodGL, size, hasAlpha, egl,
> > +                                   allocator, grallocTC, prodTex) );
> 
> auto surf = MakeUnique<ptrT>(...);
[1]
> 
> ::: gfx/gl/SharedSurfaceIO.cpp
> @@ +23,5 @@
> >  
> >      gfx::IntSize size(surface->GetWidth(), surface->GetHeight());
> > +
> > +    typedef SharedSurface_IOSurface ptrT;
> > +    return UniquePtr<ptrT>( new ptrT(ioSurf, gl, size, hasAlpha) );
> 
> MakeUnique
[1]
> 
> @@ +148,5 @@
> >      gfx::IntSize maxDims(MacIOSurface::GetMaxWidth(),
> >                           MacIOSurface::GetMaxHeight());
> > +
> > +    typedef SurfaceFactory_IOSurface ptrT;
> > +    return UniquePtr<ptrT>( new ptrT(gl, caps, maxDims) );
> 
> MakeUnique
[1]
> 
> ::: gfx/gl/SurfaceStream.cpp
> @@ +87,5 @@
> >      if (surf) {
> >          // Before next use, wait until SharedSurface's buffer
> >          // is no longer being used.
> >          surf->WaitForBufferOwnership();
> > +        mSurfaces.insert(surf.get());
> 
> This...reads very dodgy.  Perhaps all the lifetimes work out all right, but
> this sort of shared-ownership of pointers thing is not really what UniquePtr
> was designed for at all.  I would hesitate to do this if it were me.
`mSurfaces` is only for DEBUG, but it's not behind ifdefs for some reason. I put in the ifdefs.
> 
> @@ +103,5 @@
> > +    UniquePtr<SharedSurface>& to = *slotTo;
> > +
> > +    MOZ_ASSERT(!to);
> > +    to = Move(from);
> > +    from = nullptr;
> 
> Assigning a Move(uniqueptr) into a UniquePtr, nulls out uniqueptr.  This
> second line is unnecessary.
This is true, but what's our upgrade strategy for std::unique_ptr and std::move? If we're going to upgrade at some point, it's important that I null this out, instead of leaving this in a 'destructable but undefined' state.
> 
> @@ +132,3 @@
> >          surf = nullptr;
> >      }
> >      MOZ_ASSERT(!surf);
> 
> This assertion is vacuously correct now.
This is about establishing postconditions for the function.
> 
> @@ +146,5 @@
> > +
> > +    UniquePtr<SharedSurface> ret = Move(surf);
> > +    surf = nullptr;
> > +
> > +    return Move(ret);
> 
> These three lines are equivalent to
> 
>   return Move(surf);
I want to make sure *surfSlot is nulled.
> 
> @@ +164,5 @@
> > +
> > +    UniquePtr<SharedSurface> ret = Move(surf);
> > +    surf = nullptr;
> > +
> > +    return Move(ret);
> 
> And these are
> 
>   return Move(surf);
I want to make sure *surfSlot is nulled.
> 
> @@ +179,3 @@
> >          surf = nullptr;
> >      }
> >      MOZ_ASSERT(!surf);
> 
> !surf is similarly obvious here, if push() always succeeds.
This is about establishing postconditions for the function.
> 
> @@ +190,1 @@
> >          mScraps.pop();
> 
> UniquePtr<SharedSurface> cur = mScraps.pop();
std::queue::pop returns void.
> 
> @@ +217,1 @@
> >          mScraps.pop();
> 
> UniquePtr<SharedSurface> cur = mScraps.pop();
std::queue::pop returns void.
> 
> ::: gfx/layers/client/ClientCanvasLayer.cpp
> @@ +98,5 @@
> >              // Well, this *should* work...
> >  #ifdef XP_MACOSX
> >              factory = SurfaceFactory_IOSurface::Create(mGLContext, caps);
> >  #else
> > +            factory = MakeUnique<SurfaceFactory_GLTexture>(mGLContext, nullptr, caps);
> 
> As long as gcc < 4.6 is a thing, you can't pass plain nullptr here.  0 will
> work (...I think), or you can just cast to the explicit pointer type.
Right, my bad. I'll do one of the things we talked about.

Comment 15

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #10)
> Comment on attachment 8468890 [details] [diff] [review]
> patch 3: SharedSurface, SurfaceFactory
> 
> Review of attachment 8468890 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If my questions are unfounded then an easy r+ but I'd like to see the
> reasons recorded.
> 
> ::: gfx/gl/GLScreenBuffer.cpp
> @@ +54,5 @@
> >          factory = SurfaceFactory_IOSurface::Create(gl, caps);
> >      }
> >  #endif
> >  
> > +    if (!factory) {
> 
> Are the extra { }'s necessary?
Technically no, but I felt like I wanted them, for some reason. It's technically better style, but I don't care strongly.
> 
> @@ +371,4 @@
> >  {
> >      MOZ_ASSERT(mStream);
> >  
> >      if (newFactory) {
> 
> ditto
> 
> ::: gfx/gl/GLScreenBuffer.h
> @@ +174,5 @@
> >          return mStream;
> >      }
> >  
> >      SurfaceFactory* Factory() const {
> > +        return mFactory.get();
> 
> Just an observation and rumination: This leaks the pointer from the unique
> ptr. So it's possible that some one could keep the raw pointer and pass it
> around and possibly delete it.
Yep, we shouldn't do this.
If we make a BorrowedPointer<T> type for this, I'd return that. (ideally the BorrowedPointer<T> in DEBUG would be a WeakPtr that asserts if the WeakPtr is ever set to null while a BorrowedPointer<T> still exists)
> 
> ::: gfx/gl/SharedSurfaceANGLE.cpp
> @@ +199,5 @@
> >          return nullptr;
> >      }
> >  
> > +    typedef SharedSurface_ANGLEShareHandle ptrT;
> > +    return UniquePtr<ptrT>( new ptrT(gl, egl, size, hasAlpha, context,
> 
> Does that need a Move() like the others?
No, because we create the UniquePtr directly 'on' the 'return var'.[1]
> 
> @@ +218,5 @@
> > +        return nullptr;
> > +    }
> > +
> > +    typedef SurfaceFactory_ANGLEShareHandle ptrT;
> > +    return UniquePtr<ptrT>( new ptrT(gl, egl, caps) );
> 
> Move()?
[1]
> 
> ::: gfx/gl/SurfaceStream.cpp
> @@ +87,5 @@
> >      if (surf) {
> >          // Before next use, wait until SharedSurface's buffer
> >          // is no longer being used.
> >          surf->WaitForBufferOwnership();
> > +        mSurfaces.insert(surf.get());
> 
> Is it dangerous to be extracting and storing a raw pointer?
I mentioned this in my response to :waldo, but this code is for DEBUG-only, but wasn't #ifdef'd off. I added the ifdefs in my updated patch.
> 
> @@ +146,5 @@
> > +
> > +    UniquePtr<SharedSurface> ret = Move(surf);
> > +    surf = nullptr;
> > +
> > +    return Move(ret);
> 
> Can't this just be return Move(surf); instead of moving into ret?
> 
> I'm basing this suggestion upon this code fragment from Waldo's blog post:
> 
>     int* i = new int(37);
>     UniquePtr<int> i1(i);
>     UniquePtr<int> i2(Move(i1));
>     assert(i1 == nullptr);
>     assert(i2.get() == i);

I want to make sure that `surf` is nulled out. (specifically that *surfSlot is nulled out)
This is because we access the value of `surf` later, and `move` means that `surf` will be left in a "destructable but undefined state". It would only be safe to use assignment on `surf` after moving it. Requesting its value after `move` is undefined.

I believe if we switch to std::unique_ptr and std::move later, we won't be guaranteed that it is nulled out and deleted. (even if it is now?) Example:

> UniquePtr<Qux> foo = MakeCurrent<Qux>();
> UniquePtr<Qux> bar;
> 
> void MoveTo(UniquePtr<Qux>* from, UniquePtr<Qux>* to) {
>   MOZ_ASSERT(!*to);
>   *to = Move(*from);
> }
> 
> MoveTo(foo, bar);
> MoveTo(foo, bar); // This can assert with loose move guarantees.

Remember that the scope of the UniquePtrs involved here is external to the local function scope. (scope of the UniquePtrs here in approximately the scope of SurfaceStream's lifetime)

>
> 
> @@ +159,5 @@
> > +    UniquePtr<SharedSurface>& surf = *surfSlot;
> > +
> > +    if (surf) {
> > +        mSurfaces.insert(surf.get());
> > +    }
> 
> remove extra {}s
I think I'd like to keep these. I mostly like to omit the {}s when we're doing an early-return case, or both the condition and the expression are both simple. (really vague here, sorry)
> 
> @@ +164,5 @@
> > +
> > +    UniquePtr<SharedSurface> ret = Move(surf);
> > +    surf = nullptr;
> > +
> > +    return Move(ret);
> 
> Same comment above.
> 
> @@ +179,1 @@
> >          surf = nullptr;
> 
> Isn't surf == nullptr because of Move(surf)?
'No', see above.
> 
> @@ +402,3 @@
> >  
> >          if (mProducer && mStaging->mSize == mProducer->mSize)
> > +            SharedSurface::ProdCopy(mStaging.get(), mProducer.get(), factory);
> 
> Perhaps in this case add extra {}s to show that the else is for the outer
> if. On first glance, I thought the else was incorrect indented.
I agree, something this complicated should have {}.
> 
> @@ +498,1 @@
> >          }
> 
> Does WaitForCompositor update mStaging?
Not itself, it just waits.
> 
> Why not?
> if (mStaging) {
>     WaitForCompositor();
>     Scrap(&mStaging);
> }
The idea here is that the consuming thread might move mStaging to mConsumer, so we want to wait for that to happen. I'm not sure why we don't WaitForCompositor unconditionally, though. I do not want to change this code in this patch.

Comment 16

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3: SharedSurface, SurfaceFactory

Updated from previous review.

Comment 17

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3: SharedSurface, SurfaceFactory

Sorry for the churn. I forgot to fix the pass-nullptr-to-makeunique issue.

Comment 18

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #14)
> Class has a protected ctor.[1]

Does that win us anything? It's a rub, I know. Remove protected ctor to make MakeUnique accessible vs. the slight chance that someone might construct these objects. (We have code reviews to aid catching that kind of thing)

> This is true, but what's our upgrade strategy for std::unique_ptr and
> std::move? If we're going to upgrade at some point, it's important that I
> null this out, instead of leaving this in a 'destructable but undefined'
> state.

jwalden should answer this, but why would be upgrade? Mozilla has a tradition of having the equivalent of stl (MFBT and ns). If our UniquePtr adds functionality that is useful, such as nulling the old pointer, why would we "upgrade" to something that is inferior?

Comment 19

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #17)
> Created attachment 8468983 [details] [diff] [review]
> patch 3: SharedSurface, SurfaceFactory
> 
> Sorry for the churn. I forgot to fix the pass-nullptr-to-makeunique issue.

I dream of a better code review system where I could just review the diffs to the patch... :-)

Comment 20

[Jeff Walden [:Waldo]]

(In reply to Jeff Gilbert [:jgilbert] from comment #14)
> > @@ +103,5 @@
> > > +    UniquePtr<SharedSurface>& to = *slotTo;
> > > +
> > > +    MOZ_ASSERT(!to);
> > > +    to = Move(from);
> > > +    from = nullptr;
> > 
> > Assigning a Move(uniqueptr) into a UniquePtr, nulls out uniqueptr.  This
> > second line is unnecessary.
> This is true, but what's our upgrade strategy for std::unique_ptr and
> std::move? If we're going to upgrade at some point, it's important that I
> null this out, instead of leaving this in a 'destructable but undefined'
> state.

First: it's not "undefined", it's "unspecified".  The former is super-bad stuff like eating all your cheese or overwriting your hard drive with pi to the trillionth digit.  Unspecified isn't that at all.  I should change the comment to not use a wildly misleading term.  :-)

Second: the language for that is C++11 super-general language, but specific instances may give more detail if they want.  For C++11, the move assignment operator is specified to behave as if by reset(other.release()), explicitly nulling out the input.  And for move construction, the move constructor and the unique_ptr<U, E>&& constructor have the effect of "transferring ownership" from other to *this.  That phrase is defined to leave the incoming pointer nulled out, with its pointer instead residing in *this.  (And also some effects on the deleter, but unless the deleter holds state -- an infrequent occurrence -- those don't matter.)

So it is very much defined that assigning Move(uniqueptr), or std::move(u_p), into another will null out that input.

> > @@ +146,5 @@
> > > +
> > > +    UniquePtr<SharedSurface> ret = Move(surf);
> > > +    surf = nullptr;
> > > +
> > > +    return Move(ret);
> > 
> > These three lines are equivalent to
> > 
> >   return Move(surf);
> I want to make sure *surfSlot is nulled.

Given |surf| is defined as |*surfSlot|, and the above, you're still guaranteed it's nulled out.

(In reply to Jeff Gilbert [:jgilbert] from comment #15)
> If we make a BorrowedPointer<T> type for this, I'd return that. (ideally the
> BorrowedPointer<T> in DEBUG would be a WeakPtr that asserts if the WeakPtr
> is ever set to null while a BorrowedPointer<T> still exists)

There's at least one mild proposal for an exempt_ptr<T>, that wouldn't give the assert here, but would at least be a smart pointer to "decorate" the pointer and indicate something's funny about it.

(In reply to Dan Glastonbury :djg :kamidphish from comment #18)
> jwalden should answer this, but why would be upgrade? Mozilla has a
> tradition of having the equivalent of stl (MFBT and ns).

The tradition exists because STL has been inadequate for one reason or another at the time, or we couldn't use it because of compiler requirements.  It's not a great tradition to emulate.  There's nothing special about unique_ptr that would prevent us using it, except for compiler requirements.  If/when those update, we'll get rid of it.

> If our UniquePtr adds functionality that is useful, such as nulling
> the old pointer, why would we "upgrade" to something that is
> inferior?

No such addition has occurred.  To the best of my knowledge, we've added no functionality to UniquePtr or MakeUnique that isn't already present in unique_ptr, or that isn't permitted as "undefined behavior" (e.g. dereferencing a null UniquePtr).  A few omissions (some of laziness, some of necessity), but no additions.

Comment 21

[Jeff Walden [:Waldo]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #20)
> For C++11, the move assignment
> operator is specified to behave as if by reset(other.release()), explicitly
> nulling out the input.  And for move construction, the move constructor and
> the unique_ptr<U, E>&& constructor have the effect of "transferring
> ownership" from other to *this.

The operator/constructor specifically of unique_ptr, that is.

Comment 22

[Kelsey Gilbert [:jgilbert]]

 https://tbpl.mozilla.org/?tree=Try&rev=25a4c6f2974e

Comment 23

[Kelsey Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Try&rev=4fedbc53a342

Comment 24

[Kelsey Gilbert [:jgilbert]]

(In reply to Jeff Gilbert [:jgilbert] from comment #23)
> https://tbpl.mozilla.org/?tree=Try&rev=4fedbc53a342

Oops, wrong bug.

Comment 25

[Kelsey Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Try&rev=b3b078ddae53

Comment 26

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 5: Custom wrapper for std::queue<UniquePtr<T>>

Comment 27

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 4: Fix compilation errors

Comment 28

[Kelsey Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Try&rev=244b092d2da0

Comment 29

[Kelsey Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Try&rev=026b1ac3f414

Comment 30

[Jeff Walden [:Waldo]]

Comment on attachment 8473432 [details] [diff] [review]
patch 5: Custom wrapper for std::queue<UniquePtr<T>>

Review of attachment 8473432 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/SharedSurface.cpp
@@ +268,5 @@
>  
>  SurfaceFactory::~SurfaceFactory()
>  {
> +    while (!mScraps.Empty()) {
> +        UniquePtr<SharedSurface> cur = mScraps.Pop();

The local isn't necessary here, with the return type being a UniquePtr.

::: gfx/gl/SurfaceStream.cpp
@@ +189,1 @@
>          surf = nullptr;

Null-assignment still not necessary as explained before.  :-)

Comment 31

[u480271]

Comment on attachment 8473432 [details] [diff] [review]
patch 5: Custom wrapper for std::queue<UniquePtr<T>>

Review of attachment 8473432 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/SharedSurface.h
@@ +122,5 @@
> +public:
> +    ~UniquePtrQueue() {
> +        MOZ_ASSERT(Empty());
> +    }
> +    

WS

@@ +142,5 @@
> +        mQueue.pop();
> +
> +        ret.reset(p);
> +        return Move(ret);
> +    }

UniquePtr<T> Pop() {
    UniquePtr<T> ret;
    if (!mQueue.empty()) {
        ret.reset(mQueue.front());
        mQueue.pop();
    }

    return Move(ret);
}

::: gfx/gl/SurfaceStream.cpp
@@ +189,1 @@
>          surf = nullptr;

I'll agree with :Waldo and suggest code not written is better.

Comment 32

[u480271]

Comment on attachment 8473433 [details] [diff] [review]
patch 4: Fix compilation errors

Review of attachment 8473433 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/GLScreenBuffer.cpp
@@ -651,5 @@
>  
>      UniquePtr<ReadBuffer> ret( new ReadBuffer(gl, fb, depthRB,
>                                                stencilRB, surf) );
> -    if (!gl->IsFramebufferComplete(fb))
> -        return nullptr;

I prefer this version with out the extra {}s

Comment 33

[Kelsey Gilbert [:jgilbert]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #30)
> Comment on attachment 8473432 [details] [diff] [review]
> patch 5: Custom wrapper for std::queue<UniquePtr<T>>
> 
> Review of attachment 8473432 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/SharedSurface.cpp
> @@ +268,5 @@
> >  
> >  SurfaceFactory::~SurfaceFactory()
> >  {
> > +    while (!mScraps.Empty()) {
> > +        UniquePtr<SharedSurface> cur = mScraps.Pop();
> 
> The local isn't necessary here, with the return type being a UniquePtr.
Right, ok.

> 
> ::: gfx/gl/SurfaceStream.cpp
> @@ +189,1 @@
> >          surf = nullptr;
> 
> Null-assignment still not necessary as explained before.  :-)
Right. I'll assume it from now on. For readability, I'm going to add asserts as a form of indicating precondition.

Comment 34

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #31)
> Comment on attachment 8473432 [details] [diff] [review]
> patch 5: Custom wrapper for std::queue<UniquePtr<T>>
> 
> Review of attachment 8473432 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/SharedSurface.h
> @@ +122,5 @@
> > +public:
> > +    ~UniquePtrQueue() {
> > +        MOZ_ASSERT(Empty());
> > +    }
> > +    
> 
> WS
Fixed.
> 
> @@ +142,5 @@
> > +        mQueue.pop();
> > +
> > +        ret.reset(p);
> > +        return Move(ret);
> > +    }
> 
> UniquePtr<T> Pop() {
>     UniquePtr<T> ret;
>     if (!mQueue.empty()) {
>         ret.reset(mQueue.front());
>         mQueue.pop();
>     }
> 
>     return Move(ret);
> }
Sure.
> 
> ::: gfx/gl/SurfaceStream.cpp
> @@ +189,1 @@
> >          surf = nullptr;
> 
> I'll agree with :Waldo and suggest code not written is better.

Alright, removed, and in some places replaced with asserts.

Comment 35

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #32)
> Comment on attachment 8473433 [details] [diff] [review]
> patch 4: Fix compilation errors
> 
> Review of attachment 8473433 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLScreenBuffer.cpp
> @@ -651,5 @@
> >  
> >      UniquePtr<ReadBuffer> ret( new ReadBuffer(gl, fb, depthRB,
> >                                                stencilRB, surf) );
> > -    if (!gl->IsFramebufferComplete(fb))
> > -        return nullptr;
> 
> I prefer this version with out the extra {}s

I agree, but B2G on ICS's GCC 4.4 doesn't allow `return nullptr` to return into UniquePre<T>, so we would need to create a temporary UniquePtr<T> and move-return it.

Comment 36

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 5: Address review comments

Carrying forward r+ on this, since it's just cleanup for r+'d patches.

Comment 37

[Kelsey Gilbert [:jgilbert]]

Let's double check that this still builds:
https://tbpl.mozilla.org/?tree=Try&rev=b394a348c706

Comment 38

[Kelsey Gilbert [:jgilbert]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/965f780a7aa4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0c2662b805d2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2055bb42b4e5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/57a7fffebc80
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/476eb01b1280
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6007c13f6010
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3a946d84ad69

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/965f780a7aa4
https://hg.mozilla.org/mozilla-central/rev/0c2662b805d2
https://hg.mozilla.org/mozilla-central/rev/2055bb42b4e5
https://hg.mozilla.org/mozilla-central/rev/57a7fffebc80
https://hg.mozilla.org/mozilla-central/rev/476eb01b1280
https://hg.mozilla.org/mozilla-central/rev/6007c13f6010
https://hg.mozilla.org/mozilla-central/rev/3a946d84ad69