1281994 - Refactor WebGLBuffer::Delete() , using removeFrom to remove WebGLBuffer from WebGLContext's list

Status: RESOLVED WONTFIX

(Core :: Graphics: CanvasWebGL, defect)

Description

[Ethan Lin[:ethlin]]

We should remove WebGLBuffer from list[1] to prevent the delete order problem.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLBuffer.cpp#66

Comment 1

[Ethan Lin[:ethlin]]

Attachment: remove WebGLBuffer from list

Remove the WebGLBuffer from the list in mContext.

Comment 2

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8764810 [details] [diff] [review]
remove WebGLBuffer from list

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

::: dom/canvas/WebGLBuffer.cpp
@@ +62,5 @@
>      mContext->MakeContextCurrent();
>      mContext->gl->fDeleteBuffers(1, &mGLName);
>      mByteLength = 0;
>      mCache = nullptr;
> +    LinkedListElement<WebGLBuffer>::removeFrom(mContext->mBuffers);

removeFrom() just has a slightly different assert from remove(), then just calls remove(), so this should have no functional change.

Comment 3

[Ethan Lin[:ethlin]]

(In reply to Jeff Gilbert [:jgilbert] from comment #2)
> Comment on attachment 8764810 [details] [diff] [review]
> remove WebGLBuffer from list
> 
> Review of attachment 8764810 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGLBuffer.cpp
> @@ +62,5 @@
> >      mContext->MakeContextCurrent();
> >      mContext->gl->fDeleteBuffers(1, &mGLName);
> >      mByteLength = 0;
> >      mCache = nullptr;
> > +    LinkedListElement<WebGLBuffer>::removeFrom(mContext->mBuffers);
> 
> removeFrom() just has a slightly different assert from remove(), then just
> calls remove(), so this should have no functional change.

Right, I think we should take it as refactoring. It would be more clear to use removeFrom and all other places in WebGL use removeFrom now[1]. I'll change the title of this bug.

[1] https://dxr.mozilla.org/mozilla-central/search?q=removeFrom+path%3Acanvas&redirect=false

Comment 4

[Ethan Lin[:ethlin]]

(In reply to Ethan Lin[:ethlin] from comment #3)
> (In reply to Jeff Gilbert [:jgilbert] from comment #2)
> > Comment on attachment 8764810 [details] [diff] [review]
> > remove WebGLBuffer from list
> > 
> > Review of attachment 8764810 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/canvas/WebGLBuffer.cpp
> > @@ +62,5 @@
> > >      mContext->MakeContextCurrent();
> > >      mContext->gl->fDeleteBuffers(1, &mGLName);
> > >      mByteLength = 0;
> > >      mCache = nullptr;
> > > +    LinkedListElement<WebGLBuffer>::removeFrom(mContext->mBuffers);
> > 
> > removeFrom() just has a slightly different assert from remove(), then just
> > calls remove(), so this should have no functional change.
> 
> Right, I think we should take it as refactoring. It would be more clear to
> use removeFrom and all other places in WebGL use removeFrom now[1]. I'll
> change the title of this bug.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/
> search?q=removeFrom+path%3Acanvas&redirect=false
:jgilbert, do you think it's okay to do the refactoring? I think at least it's using the same way as other WebGL objects to do the remove, though the change is small.

Comment 5

[Kelsey Gilbert [:jgilbert]]

(In reply to Ethan Lin[:ethlin] from comment #4)
> (In reply to Ethan Lin[:ethlin] from comment #3)
> > (In reply to Jeff Gilbert [:jgilbert] from comment #2)
> > > Comment on attachment 8764810 [details] [diff] [review]
> > > remove WebGLBuffer from list
> > > 
> > > Review of attachment 8764810 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: dom/canvas/WebGLBuffer.cpp
> > > @@ +62,5 @@
> > > >      mContext->MakeContextCurrent();
> > > >      mContext->gl->fDeleteBuffers(1, &mGLName);
> > > >      mByteLength = 0;
> > > >      mCache = nullptr;
> > > > +    LinkedListElement<WebGLBuffer>::removeFrom(mContext->mBuffers);
> > > 
> > > removeFrom() just has a slightly different assert from remove(), then just
> > > calls remove(), so this should have no functional change.
> > 
> > Right, I think we should take it as refactoring. It would be more clear to
> > use removeFrom and all other places in WebGL use removeFrom now[1]. I'll
> > change the title of this bug.
> > 
> > [1]
> > https://dxr.mozilla.org/mozilla-central/
> > search?q=removeFrom+path%3Acanvas&redirect=false
> :jgilbert, do you think it's okay to do the refactoring? I think at least
> it's using the same way as other WebGL objects to do the remove, though the
> change is small.

I don't think we should bother, since there is only one LinkedList these could be added to, so the assert should not be useful to us. I think it's a little more paranoid than we need.