1127321 - TileClient copy constructor and operator= missing the expiration tracker member

Status: RESOLVED INVALID

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

I assume this is unintentional, and bad.

Comment 1

[Nicolas Silva [:nical]]

Attachment: add missing member copy

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Should TileClient just be using the default operators here?

Comment 3

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #2)
> Should TileClient just be using the default operators here?

Nope the PrivateProtector prevents that.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8556476 [details] [diff] [review]
add missing member copy

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

How did you find this?

Comment 5

[Nicolas Silva [:nical]]

(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?

Comment 6

[Jeff Muizelaar [:jrmuizel]]

(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.

Comment 7

[Nicolas Silva [:nical]]

(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.

Comment 8

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/31c0fd2b6e8d

Comment 9

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=6161085&repo=mozilla-inbound

Comment 10

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

I believe nsExpirationTracker already properly sets this.

Comment 12

[Nicolas Silva [:nical]]

(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.