TileClient copy constructor and operator= missing the expiration tracker member

RESOLVED INVALID

Status

()

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

People

(Reporter: nical, Assigned: nical)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
I assume this is unintentional, and bad.
(Assignee)

Comment 1

3 years ago
Created attachment 8556476 [details] [diff] [review]
add missing member copy
Attachment #8556476 - Flags: review?(jmuizelaar)
Should TileClient just be using the default operators here?
(In reply to Jeff Muizelaar [:jrmuizel] from comment #2)
> Should TileClient just be using the default operators here?

Nope the PrivateProtector prevents that.
Comment on attachment 8556476 [details] [diff] [review]
add missing member copy

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

How did you find this?
Attachment #8556476 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 5

3 years ago
(In reply to Jeff Muizelaar [:jrmuizel] from comment #4)
> Comment on attachment 8556476 [details] [diff] [review]
> add missing member copy
> 
> Review of attachment 8556476 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> How did you find this?

I made the same mistake implementing edge padding for DrawTargetTiled (I had to add a member and forgot about the copy constructor). Then I decided that from now on I dislike explicit copy constructors.
Is there a warning that we could turn on that would check whether people assign all members in copy constructors and assignment operators?
(In reply to Nicolas Silva [:nical] from comment #5)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #4)
> > Comment on attachment 8556476 [details] [diff] [review]
> > add missing member copy
> > 
> > Review of attachment 8556476 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > How did you find this?
> 
> I made the same mistake implementing edge padding for DrawTargetTiled (I had
> to add a member and forgot about the copy constructor). Then I decided that
> from now on I dislike explicit copy constructors.
> Is there a warning that we could turn on that would check whether people
> assign all members in copy constructors and assignment operators?

I've started work on a clang analysis pass that requires you to initialize all members in constructors
unless that member has a default constructor or is annotated as not requiring initialization. It should be possible to extend this so that it checks the copy constructors, move constructors, assignment operators etc.
(Assignee)

Comment 7

3 years ago
(In reply to Jeff Muizelaar [:jrmuizel] from comment #6)
> I've started work on a clang analysis pass that requires you to initialize
> all members in constructors
> unless that member has a default constructor or is annotated as not
> requiring initialization. It should be possible to extend this so that it
> checks the copy constructors, move constructors, assignment operators etc.

Tremendous.
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=6161085&repo=mozilla-inbound
Comment on attachment 8556476 [details] [diff] [review]
add missing member copy

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

I think there might be no bug here.
Attachment #8556476 - Flags: review+ → review-
I believe nsExpirationTracker already properly sets this.
(Assignee)

Comment 12

3 years ago
(In reply to Jeff Muizelaar [:jrmuizel] from comment #10)
> I think there might be no bug here.

Hm, so if I understand correctly, this works because we know that by the end of this spin of the event-loop (assuming the expiration notifications come in a separate event-loop turn), there will be only one of the two copies of the tile that just got copied/assigned, so there is no risk of the same tile expiring twice (through 2 copies of the POD struct) which would lead to TextureClients being added twice to the pool.
(Assignee)

Updated

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.