892448 - Wrong ref counting in CompositableHost::Create

Status: RESOLVED INVALID

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

Attachment: Fix the ref counting problem

ew, I am surprised we haven't seen that earlier. (or do we have some implicit magic when casting a RefPtr into a TemporaryRef? in any case I much prefer having return result.forget() explicitly)

Comment 1

[Nicolas Silva [:nical]]

Attachment: Fix the ref counting problem

oops, better fix

Comment 2

[Nicolas Silva [:nical]]

Attachment: Fix the ref counting problem

darn, uploaded the wrong file again, sorry for the mail churn

Comment 3

[Benoit Girard (:BenWa)]

Comment on attachment 773915 [details] [diff] [review]
Fix the ref counting problem

Can you verify if this is actually a problem? This is useful to know because:
- We should uplift this patch
- I'm seeing a large number of TiledContentHost in a different bug
- If it is a bug then we could add a runtime debug check for this kind of pattern.

Comment 4

[Nicolas Silva [:nical]]

(In reply to Benoit Girard (:BenWa) from comment #3)
> Comment on attachment 773915 [details] [diff] [review]
> Fix the ref counting problem
> 
> Can you verify if this is actually a problem? This is useful to know because:
> - We should uplift this patch
> - I'm seeing a large number of TiledContentHost in a different bug
> - If it is a bug then we could add a runtime debug check for this kind of
> pattern.

Actually it is not a problem with RefPtr. Unlike alread_AddRefed, TemporaryRef(T*) increments the refcount ( http://dxr.mozilla.org/mozilla-central/source/mfbt/RefPtr.h#l221 ), and RefPtr has the operator TemporaryRef ( http://dxr.mozilla.org/mozilla-central/source/mfbt/RefPtr.h#l184 ) which calls the right constructor in TemporaryRef that does the increment.
So when RefPtr goes out of scope the ref count is 2 and things are good.

So false alert, I assumed that TemporaryRef behaved like already_AdRefed which isn't as smart.

I don't know whether we still want to impose the return result.forget() style to avoid mistakes with nsRefPtr or not, but this patch turns out to just be cosmetic.

Comment 5

[Benoit Girard (:BenWa)]

Comment on attachment 773915 [details] [diff] [review]
Fix the ref counting problem

Taking off review since the bug is closed.