Closed Bug 1281994 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: ethlin, Assigned: ethlin)

References

Details

Attachments

(1 file)

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
Remove the WebGLBuffer from the list in mContext.
Attachment #8764810 - Flags: review?(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.
Attachment #8764810 - Flags: review?(jgilbert) → review-
(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
Summary: WebGLBuffer should be removed from list in WebGLBuffer::Delete() → Refactor WebGLBuffer::Delete() , using removeFrom to remove WebGLBuffer from WebGLContext's list
(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.
Flags: needinfo?(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.
Flags: needinfo?(jgilbert)
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.