Wrong ref counting in CompositableHost::Create

RESOLVED INVALID

Status

()

Core
Graphics: Layers
RESOLVED INVALID
5 years ago
5 years ago

People

(Reporter: nical, Assigned: nical)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 773908 [details] [diff] [review]
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)
(Assignee)

Updated

5 years ago
Attachment #773908 - Flags: review?(bgirard)
(Assignee)

Updated

5 years ago
Assignee: nobody → nical.bugzilla
(Assignee)

Comment 1

5 years ago
Created attachment 773913 [details] [diff] [review]
Fix the ref counting problem

oops, better fix
Attachment #773908 - Attachment is obsolete: true
Attachment #773908 - Flags: review?(bgirard)
Attachment #773913 - Flags: review?(bgirard)
(Assignee)

Comment 2

5 years ago
Created attachment 773915 [details] [diff] [review]
Fix the ref counting problem

darn, uploaded the wrong file again, sorry for the mail churn
Attachment #773913 - Attachment is obsolete: true
Attachment #773913 - Flags: review?(bgirard)
Attachment #773915 - Flags: review?(bgirard)
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.
(Assignee)

Comment 4

5 years ago
(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.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → INVALID
Comment on attachment 773915 [details] [diff] [review]
Fix the ref counting problem

Taking off review since the bug is closed.
Attachment #773915 - Flags: review?(bgirard)
You need to log in before you can comment on or make changes to this bug.