Make WebGLFramebufferAttachable use a WeakPtr instead of raw pointer, and use a nsTArray instead of a mfbt Vector, and use nsTArray methods instead of custom loops

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

Other Branch
mozilla34
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

We recently had a sec-critical around here, that is a good reminder of why we try to avoid raw pointers when we can. The code at hand here really wants a 'weak reference' kind of pointer, whereby a texture can remember what framebuffers it was attached to without keeping them alive; so the second patch here turns this raw pointer into a MFBT WeakPtr. As a prerequisite, shortcomings in MFBT WeakPtr are addressed in bug 1044658 - in particular, we really want a WeakPtr-to-const here (that's the whole point for WebGLFramebuffer::mStatus being mutable, right?), and that is not supported before bug 1044658.

While looking at this code I also noticed that it is 1) using MFBT Vector for its array container, and 2) doing custom array-traversal that doesn't seem to be needed. This is the only use of MFBT Vector in all of content/canvas. I believe that regardless of respective merits of containers, it is worth avoiding using more different container classes/libraries than is useful for a definite purpose, so as to keep the codebase easier for people to understand. So I made this a nsTArray. This had the immediate benefit of saving some memory in the most typical case where a texture is not attached to any framebuffer (which is 99% of textures) as nsTArray has empty arrays occupy only the size of one pointer. Another change (and I haven't checked to what extent it is allowed by the switch to nsTArray) is I could replace custom for loops by just calls to existing nsTArray methods.
Bug 1010090 is the sec-critical that prompts looking at avoiding raw pointer usage.
(In reply to Benoit Jacob [:bjacob] from comment #1)
> Bug 1010090 is the sec-critical that prompts looking at avoiding raw pointer
> usage.

...which was duped to bug 1028891.
Comment on attachment 8463170 [details] [diff] [review]
Patch 1: Use a nsTArray instead of a MFBT Vector, and use nsTArray methods instead of for loops

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

Is there any guidance on which containers we are supposed to use? I may have foolishly used mfbt because I thought that was what new code was supposed to use.
Attachment #8463170 - Flags: review?(dglastonbury) → review+
Comment on attachment 8463171 [details] [diff] [review]
Patch 2: Use a WeakPtr instead of a raw pointer in WebGLFramebufferAttachable::AttachmentPoint

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

I had troubles with circular references when adding WeakPtr. I guess that was because I was using mfbt::Vector?

I had to cast away the const on WebGLFramebuffer* passed in the constructor. It was the least gross way to get patch to compile. I tried making WeakPtr "const correct" but it triggered a cascade of errors that I didn't have patience to investigate much pass 1 or 2 iterations.

::: content/canvas/src/WebGLFramebufferAttachable.cpp
@@ +49,1 @@
>          mAttachmentPoints[i].mFB->NotifyAttachableChanged();

In non-debug builds wouldn't it be best not to access null here?

::: content/canvas/src/WebGLFramebufferAttachable.h
@@ +17,5 @@
>  {
>      struct AttachmentPoint
>      {
>          AttachmentPoint(const WebGLFramebuffer* fb, GLenum attachment)
>              : mFB(fb)

This line doesn't compile.

I changed locally to:

        AttachmentPoint(const WebGLFramebuffer* fb, GLenum attachment)
            : mAttachment(attachment)
        {
            MOZ_ASSERT(fb);
            mFB = const_cast<WebGLFramebuffer*>(fb)->asWeakPtr();
        }

Which is gross.

@@ +21,5 @@
>              : mFB(fb)
>              , mAttachment(attachment)
>          {}
>  
> +        WeakPtr<const WebGLFramebuffer> mFB;

Had to change this to:

    WeakPtr<WebGLFramebuffer> mFB;
Attachment #8463171 - Flags: review?(dglastonbury) → review-
(In reply to Dan Glastonbury :djg :kamidphish from comment #6)
> Comment on attachment 8463171 [details] [diff] [review]
> Patch 2: Use a WeakPtr instead of a raw pointer in
> WebGLFramebufferAttachable::AttachmentPoint
> 
> Review of attachment 8463171 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I had troubles with circular references when adding WeakPtr. I guess that
> was because I was using mfbt::Vector?

No idea - what kind of circular references?

> 
> I had to cast away the const on WebGLFramebuffer* passed in the constructor.
> It was the least gross way to get patch to compile. I tried making WeakPtr
> "const correct" but it triggered a cascade of errors that I didn't have
> patience to investigate much pass 1 or 2 iterations.

Looks like you hit the same as I did - did you see the above comment 0 and bug 1044658 blocking this bug?
(In reply to Dan Glastonbury :djg :kamidphish from comment #5)
> Comment on attachment 8463170 [details] [diff] [review]
> Patch 1: Use a nsTArray instead of a MFBT Vector, and use nsTArray methods
> instead of for loops
> 
> Review of attachment 8463170 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is there any guidance on which containers we are supposed to use? I may have
> foolishly used mfbt because I thought that was what new code was supposed to
> use.

There isn't policy on this; for each use case we have to decide which container is the best compromise. That means it's OK to disagree and we should talk through this until we're sure to agree :-)

In comment 0 explained why I think that nsTArray is a better choice than Vector here: 1) consistency with surrounding code, 2) the size optimization of zero-length arrays, which is the case most of the time here (non-attached textures), and 3) we can make good use of nsTArray methods such as Contains() and Find(). Do you agree with this reasoning?
Needinfo for comment 7 and comment 8.
Flags: needinfo?(dglastonbury)
(In reply to Benoit Jacob [:bjacob] from comment #7)
> > I had troubles with circular references when adding WeakPtr. I guess that
> > was because I was using mfbt::Vector?
> 
> No idea - what kind of circular references?

Well FramebufferAttachment wanted Framebuffer for WeakPtr and Framebuffer wanted FramebufferAttachment for mfbt::Vector. (I think)

> Looks like you hit the same as I did - did you see the above comment 0 and
> bug 1044658 blocking this bug?

No, I only saw those after I reviewed your patches. Mea culpa.
Flags: needinfo?(dglastonbury)
(In reply to Benoit Jacob [:bjacob] from comment #8)
> There isn't policy on this; for each use case we have to decide which
> container is the best compromise. That means it's OK to disagree and we
> should talk through this until we're sure to agree :-)

I was asking for my benefit. I looks like nsTArray is a better choice in many areas.

> In comment 0 explained why I think that nsTArray is a better choice than
> Vector here: [...] Do you agree with this reasoning?

Yes, that's all good.
Comment on attachment 8463171 [details] [diff] [review]
Patch 2: Use a WeakPtr instead of a raw pointer in WebGLFramebufferAttachable::AttachmentPoint

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

I missed the changes to WeakPtr in Bug 1044658. Will follow up null ptr check in another patch.

::: content/canvas/src/WebGLFramebufferAttachable.cpp
@@ +49,1 @@
>          mAttachmentPoints[i].mFB->NotifyAttachableChanged();

In non-debug builds wouldn't it be best not to access null here?

::: content/canvas/src/WebGLFramebufferAttachable.h
@@ +17,5 @@
>  {
>      struct AttachmentPoint
>      {
>          AttachmentPoint(const WebGLFramebuffer* fb, GLenum attachment)
>              : mFB(fb)

This line doesn't compile.

I changed locally to:

        AttachmentPoint(const WebGLFramebuffer* fb, GLenum attachment)
            : mAttachment(attachment)
        {
            MOZ_ASSERT(fb);
            mFB = const_cast<WebGLFramebuffer*>(fb)->asWeakPtr();
        }

Which is gross.

@@ +21,5 @@
>              : mFB(fb)
>              , mAttachment(attachment)
>          {}
>  
> +        WeakPtr<const WebGLFramebuffer> mFB;

Had to change this to:

    WeakPtr<WebGLFramebuffer> mFB;
Attachment #8463171 - Flags: review- → review+
Thanks!

Yeah, I prefer to leave up to you the precise way that you want the lifetime of framebuffers (whence, null-checking of this pointer) to be handled. This patch intentionally sticks to crudely asserting non-nullness.

(It seems that the review comments in comment 12 are a copy of those in comment 6 unintentionally pasted there).
Oh right, that's that old bug in our review system. Nothing to be seen here.
https://hg.mozilla.org/mozilla-central/rev/621963c11023
https://hg.mozilla.org/mozilla-central/rev/79d249340b3f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.