Explicitly free shared memory buffers on GL client destruction.

Explicitly free shared memory buffers on destruction.

Without this change transfer buffers can leak until the ContextGroup
is destroyed.

BUG=169141
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177054

[no sievers, 2013-01-10 23:30:18]

ptal

[greggman, 2013-01-11 02:22:00]

lgtm

[benm (inactive), 2013-01-11 10:24:51]

Thanks Daniel! LGTM from the point of view that we aren't leaking any more in
Android WebView :)

[commit-bot: I haz the power, 2013-01-11 17:53:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sievers@chromium.org/11840002/1

[no sievers, 2013-01-11 18:34:17]

Looks like these 4 gpu unit tests fail with an unexpected call to
DestroyTransferBuffer():

BeginEndQueryEXT
ErrorQuery
MapUnmapBufferSubDataCHROMIUM
MapUnmapTexSubImage2DCHROMIUM

I'll probably just have to update the expectations there...

[commit-bot: I haz the power, 2013-01-11 18:42:41]

Retried try job too often on linux_rel for step(s) gpu_unittests

[no sievers, 2013-01-11 23:22:41]

Gregg, I updated the tests that allocate a transfer buffer to expect the extra
call to DestroyTransferBuffer().

For the two map sub tests, I was able to make this a more explicit part of the
test by setting up the expectation with the specific transfer buffer id and
calling FreeUnusedSharedMemory(). 

If you tell me how I can fake query completion, I'd be able to do the same for
the two query tests.

[greggman, 2013-01-12 01:44:31]

On 2013/01/11 23:22:41, Daniel Sievers wrote:
> Gregg, I updated the tests that allocate a transfer buffer to expect the extra
> call to DestroyTransferBuffer().
> 
> For the two map sub tests, I was able to make this a more explicit part of the
> test by setting up the expectation with the specific transfer buffer id and
> calling FreeUnusedSharedMemory(). 
> 
> If you tell me how I can fake query completion, I'd be able to do the same for
> the two query tests.

To mark a query as finished you need to do

   query->info.process_count_ = query->submit_count_;

I'd suggest adding a function to QueryTracker::Query called
MarkQueryAsFinishedForTesting(uint64 result)

void QueryTracker::Query::MarkQueryAsFinishedForTesting(
    uint64 result) {
  info.process_count_ = submit_count_;
  info.result = result;
};

Make it private, make GLES2ImplementationTest a friend class of Query and then
make a function in 

GLES2ImplementationTest::MarkQueryAsFinished(
   QueryTracker::Query* query, uint64 result) {
  query->MarkQueryAsFinishedForTest(result);
}

I think that will let FreeEverything destroy everything.

[no sievers, 2013-01-12 01:49:09]

On 2013/01/12 01:44:31, greggman wrote:
> On 2013/01/11 23:22:41, Daniel Sievers wrote:
> > Gregg, I updated the tests that allocate a transfer buffer to expect the
extra
> > call to DestroyTransferBuffer().
> > 
> > For the two map sub tests, I was able to make this a more explicit part of
the
> > test by setting up the expectation with the specific transfer buffer id and
> > calling FreeUnusedSharedMemory(). 
> > 
> > If you tell me how I can fake query completion, I'd be able to do the same
for
> > the two query tests.
> 
> To mark a query as finished you need to do
> 
>    query->info.process_count_ = query->submit_count_;
> 
> I'd suggest adding a function to QueryTracker::Query called
> MarkQueryAsFinishedForTesting(uint64 result)
> 
> void QueryTracker::Query::MarkQueryAsFinishedForTesting(
>     uint64 result) {
>   info.process_count_ = submit_count_;
>   info.result = result;
> };
> 
> Make it private, make GLES2ImplementationTest a friend class of Query and then
> make a function in 
> 
> GLES2ImplementationTest::MarkQueryAsFinished(
>    QueryTracker::Query* query, uint64 result) {
>   query->MarkQueryAsFinishedForTest(result);
> }
> 
> I think that will let FreeEverything destroy everything.

Actually, FreeEverything() already correctly destroys everything and the patch
#2 works as expected with the tests passing. That's because it deletes the whole
query tracker in the implementation's destructor.

What I didn't like is that the two query tests set up the expectation for
DestroyTransferBuffer() (to free the query buffer the test was using) to be
called as part of TearDown() later (where the implementation gets deleted).

I was just thinking it would be a little more obvious if the test set up the
expectation and then explicitly frees the unused buffer.

What do you think?

[no sievers, 2013-01-12 01:53:37]

On 2013/01/12 01:49:09, Daniel Sievers wrote:
> On 2013/01/12 01:44:31, greggman wrote:
> > On 2013/01/11 23:22:41, Daniel Sievers wrote:
> > > Gregg, I updated the tests that allocate a transfer buffer to expect the
> extra
> > > call to DestroyTransferBuffer().
> > > 
> > > For the two map sub tests, I was able to make this a more explicit part of
> the
> > > test by setting up the expectation with the specific transfer buffer id
and
> > > calling FreeUnusedSharedMemory(). 
> > > 
> > > If you tell me how I can fake query completion, I'd be able to do the same
> for
> > > the two query tests.
> > 
> > To mark a query as finished you need to do
> > 
> >    query->info.process_count_ = query->submit_count_;
> > 
> > I'd suggest adding a function to QueryTracker::Query called
> > MarkQueryAsFinishedForTesting(uint64 result)
> > 
> > void QueryTracker::Query::MarkQueryAsFinishedForTesting(
> >     uint64 result) {
> >   info.process_count_ = submit_count_;
> >   info.result = result;
> > };
> > 
> > Make it private, make GLES2ImplementationTest a friend class of Query and
then
> > make a function in 
> > 
> > GLES2ImplementationTest::MarkQueryAsFinished(
> >    QueryTracker::Query* query, uint64 result) {
> >   query->MarkQueryAsFinishedForTest(result);
> > }
> > 
> > I think that will let FreeEverything destroy everything.
> 
> Actually, FreeEverything() already correctly destroys everything and the patch
> #2 works as expected with the tests passing. That's because it deletes the
whole
> query tracker in the implementation's destructor.
> 
> What I didn't like is that the two query tests set up the expectation for
> DestroyTransferBuffer() (to free the query buffer the test was using) to be
> called as part of TearDown() later (where the implementation gets deleted).
> 
> I was just thinking it would be a little more obvious if the test set up the
> expectation and then explicitly frees the unused buffer.
> 
> What do you think?

I think a good argument for adding the testing code you suggested and the test
calling FreeUnusedMemory() explicitly is that then the test works regardless of
my change to gles2_implementation.cc, i.e. it doesn't add any assumptions about
the shutdown.

[greggman, 2013-01-12 06:48:38]

On 2013/01/12 01:53:37, Daniel Sievers wrote:
> On 2013/01/12 01:49:09, Daniel Sievers wrote:
> > On 2013/01/12 01:44:31, greggman wrote:
> > > On 2013/01/11 23:22:41, Daniel Sievers wrote:
> > > > Gregg, I updated the tests that allocate a transfer buffer to expect the
> > extra
> > > > call to DestroyTransferBuffer().
> > > > 
> > > > For the two map sub tests, I was able to make this a more explicit part
of
> > the
> > > > test by setting up the expectation with the specific transfer buffer id
> and
> > > > calling FreeUnusedSharedMemory(). 
> > > > 
> > > > If you tell me how I can fake query completion, I'd be able to do the
same
> > for
> > > > the two query tests.
> > > 
> > > To mark a query as finished you need to do
> > > 
> > >    query->info.process_count_ = query->submit_count_;
> > > 
> > > I'd suggest adding a function to QueryTracker::Query called
> > > MarkQueryAsFinishedForTesting(uint64 result)
> > > 
> > > void QueryTracker::Query::MarkQueryAsFinishedForTesting(
> > >     uint64 result) {
> > >   info.process_count_ = submit_count_;
> > >   info.result = result;
> > > };
> > > 
> > > Make it private, make GLES2ImplementationTest a friend class of Query and
> then
> > > make a function in 
> > > 
> > > GLES2ImplementationTest::MarkQueryAsFinished(
> > >    QueryTracker::Query* query, uint64 result) {
> > >   query->MarkQueryAsFinishedForTest(result);
> > > }
> > > 
> > > I think that will let FreeEverything destroy everything.
> > 
> > Actually, FreeEverything() already correctly destroys everything and the
patch
> > #2 works as expected with the tests passing. That's because it deletes the
> whole
> > query tracker in the implementation's destructor.
> > 
> > What I didn't like is that the two query tests set up the expectation for
> > DestroyTransferBuffer() (to free the query buffer the test was using) to be
> > called as part of TearDown() later (where the implementation gets deleted).
> > 
> > I was just thinking it would be a little more obvious if the test set up the
> > expectation and then explicitly frees the unused buffer.
> > 
> > What do you think?
> 
> I think a good argument for adding the testing code you suggested and the test
> calling FreeUnusedMemory() explicitly is that then the test works regardless
of
> my change to gles2_implementation.cc, i.e. it doesn't add any assumptions
about
> the shutdown.

I don't think I have a preference.

[no sievers, 2013-01-14 18:41:09]

On 2013/01/12 06:48:38, greggman wrote:
> On 2013/01/12 01:53:37, Daniel Sievers wrote:
> > On 2013/01/12 01:49:09, Daniel Sievers wrote:
> > > On 2013/01/12 01:44:31, greggman wrote:
> > > > On 2013/01/11 23:22:41, Daniel Sievers wrote:
> > > > > Gregg, I updated the tests that allocate a transfer buffer to expect
the
> > > extra
> > > > > call to DestroyTransferBuffer().
> > > > > 
> > > > > For the two map sub tests, I was able to make this a more explicit
part
> of
> > > the
> > > > > test by setting up the expectation with the specific transfer buffer
id
> > and
> > > > > calling FreeUnusedSharedMemory(). 
> > > > > 
> > > > > If you tell me how I can fake query completion, I'd be able to do the
> same
> > > for
> > > > > the two query tests.
> > > > 
> > > > To mark a query as finished you need to do
> > > > 
> > > >    query->info.process_count_ = query->submit_count_;
> > > > 
> > > > I'd suggest adding a function to QueryTracker::Query called
> > > > MarkQueryAsFinishedForTesting(uint64 result)
> > > > 
> > > > void QueryTracker::Query::MarkQueryAsFinishedForTesting(
> > > >     uint64 result) {
> > > >   info.process_count_ = submit_count_;
> > > >   info.result = result;
> > > > };
> > > > 
> > > > Make it private, make GLES2ImplementationTest a friend class of Query
and
> > then
> > > > make a function in 
> > > > 
> > > > GLES2ImplementationTest::MarkQueryAsFinished(
> > > >    QueryTracker::Query* query, uint64 result) {
> > > >   query->MarkQueryAsFinishedForTest(result);
> > > > }
> > > > 
> > > > I think that will let FreeEverything destroy everything.
> > > 
> > > Actually, FreeEverything() already correctly destroys everything and the
> patch
> > > #2 works as expected with the tests passing. That's because it deletes the
> > whole
> > > query tracker in the implementation's destructor.
> > > 
> > > What I didn't like is that the two query tests set up the expectation for
> > > DestroyTransferBuffer() (to free the query buffer the test was using) to
be
> > > called as part of TearDown() later (where the implementation gets
deleted).
> > > 
> > > I was just thinking it would be a little more obvious if the test set up
the
> > > expectation and then explicitly frees the unused buffer.
> > > 
> > > What do you think?
> > 
> > I think a good argument for adding the testing code you suggested and the
test
> > calling FreeUnusedMemory() explicitly is that then the test works regardless
> of
> > my change to gles2_implementation.cc, i.e. it doesn't add any assumptions
> about
> > the shutdown.
> 
> I don't think I have a preference.

Ok, I'll submit as is then.

[commit-bot: I haz the power, 2013-01-15 18:20:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sievers@chromium.org/11840002/15001

[commit-bot: I haz the power, 2013-01-15 18:20:58]

Failed to request the patch to try. Please note that binary files are still
unsupported at the moment, this is being worked on.

Thanks for your patience.

HTTP Error 500: 
<html><head>
<meta http-equiv="content-type" content="text/html;charset=utf-8">
<title>500 Server Error</title>
</head>
<body text=#000000 bgcolor=#ffffff>
<h1>Error: Server Error</h1>
<h2>The server encountered an error and could not complete your request.<p>If
the problem persists, please <A
HREF="http://code.google.com/appengine/community.html">report</A> your problem
and mention this error message and the query that caused it.</h2>
<h2></h2>
</body></html>

[commit-bot: I haz the power, 2013-01-15 23:37:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sievers@chromium.org/11840002/15001

[commit-bot: I haz the power, 2013-01-16 02:09:00]

Change committed as 177054

[commit-bot: I haz the power, 2013-03-07 18:45:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sievers@chromium.org/11840002/36001

[no sievers, 2013-03-07 22:56:08]

I *think* https://chromiumcodereview.appspot.com/12087126 might render this
patch unnecessary.

[benm (inactive), 2013-03-09 01:44:38]

On 2013/03/07 22:56:08, Daniel Sievers wrote:
> I *think* https://chromiumcodereview.appspot.com/12087126 might render this
> patch unnecessary.

I don't think so, the test team were still hitting the leak with r182680.