Greg Grisco Reporter
	Description • 10 years ago

[Blocking Requested - why for this release]:

	Greg Grisco Reporter
	Comment 1 • 10 years ago

This happened during stability test.  I don't have any STR right now, will share if it becomes available.  But this crash happened 5 times on latest build, so wanted to file this bugzilla right away.

	Sotaro Ikeda [:sotaro]
	Comment 3 • 10 years ago

Bug 1041744 might be related to this bug.

	Sotaro Ikeda [:sotaro]
	Comment 4 • 10 years ago

Bug 1037173 seems same bug.

	Sotaro Ikeda [:sotaro]
	Comment 5 • 10 years ago

From the crash, it seems to be related to gfx layers.

	Sotaro Ikeda [:sotaro]
	Comment 6 • 10 years ago

TextureClientPool::ReturnDeferredClients() code has a possibility that call pop() even when mTextureClientsDeferred is empty.

	Sotaro Ikeda [:sotaro]
	Comment 7 • 10 years ago

(In reply to Sotaro Ikeda [:sotaro PTO July/25 - Aug/3] from comment #6)
> TextureClientPool::ReturnDeferredClients() code has a possibility that call
> pop() even when mTextureClientsDeferred is empty.

I confirmed that to call pop() to empty std::stack<RefPtr<TextureClient> > causes the crash.

	Sotaro Ikeda [:sotaro]
	Comment 9 • 10 years ago

 https://tbpl.mozilla.org/?tree=Try&rev=734e8a2bea4a

	cafbot (PoC: ggrisco)
	Comment 10 • 10 years ago

Observed on: 

Device: msm8226
Gonk Version: AU_LINUX_GECKO_B2G_KK_3.6.01.04.00.000.040
Moz BuildID: 20140716000201
B2G Version: 2.0
Gecko Version: 32.0a2
Gaia:  http://git.mozilla.org/?p=releases/gaia.git;a=commit;h=5f8b1b8a2da9e3b531eee817a669f57fa4d9b9c6
Gecko: http://git.mozilla.org/?p=releases/gecko.git;a=commit;h=e00f7e464333689fcf54edb4945ece94f97f930b

	Sotaro Ikeda [:sotaro]
	Comment 11 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d2176af4ed1

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 12 • 10 years ago

I had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/978b8d1fd58d for making b2g reftest-18 frequently fail with https://tbpl.mozilla.org/php/getParsedLog.php?id=44519068&tree=Mozilla-Inbound

	Sotaro Ikeda [:sotaro]
	Comment 13 • 10 years ago

(In reply to Wes Kocher (:KWierso) from comment #12)
> I had to back this out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/978b8d1fd58d for
> making b2g reftest-18 frequently fail with
> https://tbpl.mozilla.org/php/getParsedLog.php?id=44519068&tree=Mozilla-
> Inbound

Hmm, it is wired that the patch affected to the test failure.

	Sotaro Ikeda [:sotaro]
	Comment 14 • 10 years ago

nical, can you take this bug? I am in PTO for a week. Thanks!

	Nicolas Silva [:nical] Assignee
	Comment 15 • 10 years ago

I found one issues with the: it inlines some of ReturnTextureClient in ReturnDeferredClients so that ShrinkToMaximumSize doesn't happen inside the loop, but forgets to update the number of outstanding textures (mOutstandingClients), which causes the number to become big and causes ShrinkToMaximumSize to shrink too much.

My bad for not catching that in the review.

try push with the fix: https://tbpl.mozilla.org/?tree=Try&rev=2b4c12408d37

It is not obvious to me how it would explain the failing test, though, Even if shrinking the pool goes out of sync, I'd have expected the pool to just discard too much and reallocate often, rather than getting this particular reftest failure.

	Nicolas Silva [:nical] Assignee
	Comment 16 • 10 years ago

(In reply to Nicolas Silva [:nical] from comment #15)
> 
> try push with the fix: https://tbpl.mozilla.org/?tree=Try&rev=2b4c12408d37
> 

Seems to fix the issue.

	Greg Grisco Reporter
	Comment 17 • 10 years ago

We have built the patch into AU_LINUX_GECKO_B2G_KK_3.6.01.04.00.000.043 and later.

	Greg Grisco Reporter
	Comment 19 • 10 years ago

We haven't seen this reproduce on AU43, so probably ok to land this.

	bhavana bajaj [:bajaj]
	Comment 20 • 10 years ago

:nical can you please help check this in given comment #19 ?

	Nicolas Silva [:nical] Assignee
	Comment 21 • 10 years ago

Comment on attachment 8463301 [details] [diff] [review]
patch for b2g-v2.0 - Same patch with the outstanding texture count fixed


[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 2.0 blocker
Testing completed: Try servers and a separate branch (see comments from Greg Grisco)
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch:

	Nicolas Silva [:nical] Assignee
	Comment 22 • 10 years ago

Landed on inbound, sorry I confused this another blocker that had already landed and that we had failed to uplift. The uplift request still holds I believe.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c94ca18780c

	Carsten Book [:Tomcat]
	Comment 23 • 10 years ago

sorry had to be backed out for bustage - https://tbpl.mozilla.org/php/getParsedLog.php?id=44961850&tree=Mozilla-Inbound

	Nicolas Silva [:nical] Assignee
	Comment 24 • 10 years ago

Something landed between the try push and now, and there is new build error:

> TextureClientPool.cpp:150:56: error: 'sShrinkTimeout' was not declared in this scope

Backed out://hg.mozilla.org/integration/mozilla-inbound/rev/2e02ebf33a3a

I'll fix this and reland shortly.

	Nicolas Silva [:nical] Assignee
	Comment 25 • 10 years ago

sShrinkTimeout that got renamed into mShrinkTimeoutMsec and moved from global to member of the class. Built the fix locally and relanded:

https://hg.mozilla.org/integration/mozilla-inbound/rev/db513704b2c3

	bhavana bajaj [:bajaj]
	Comment 26 • 10 years ago

Comment on attachment 8463301 [details] [diff] [review]
patch for b2g-v2.0 - Same patch with the outstanding texture count fixed

Given this is a 2.0+ it will get auto uplifted once landing on central and it does not need an approval request

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/db513704b2c3

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 10 years ago

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/487d71a5923c

	Ryan VanderMeulen [:RyanVM]
	Comment 29 • 10 years ago

Backed out for bustage.
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/9de46ab29836

https://tbpl.mozilla.org/php/getParsedLog.php?id=44996846&tree=Mozilla-B2g32-v2.0

	Sotaro Ikeda [:sotaro]
	Comment 30 • 10 years ago

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #29)
> Backed out for bustage.
> https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/9de46ab29836
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=44996846&tree=Mozilla-B2g32-
> v2.0



Error was caused by 'mShrinkTimeoutMsec'. The committed patch is affected by Bug 1043603.

attachment 8463301 [details] [diff] [review] does not have 'mShrinkTimeoutMsec'. attachment 8463301 [details] [diff] [review] should work for b2g-v2.0.

	Ryan VanderMeulen [:RyanVM]
	Comment 31 • 10 years ago

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/80198ae727c0

	Robert Mead [:RobertM]
	Comment 32 • 10 years ago

Not enough information with current STR to write test case to address bug.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 33 • 10 years ago

Comment on attachment 8463301 [details] [diff] [review]
patch for b2g-v2.0 - Same patch with the outstanding texture count fixed

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

::: gfx/layers/client/TextureClientPool.cpp
@@ +141,5 @@
> +  }
> +  ShrinkToMinimumSize();
> +  // Kick off the pool shrinking timer if there are still more unused texture
> +  // clients than our desired minimum cache size.
> +  if (mTextureClients.size() > sMinCacheSize) {

ShrinkToMinimumSize has a loop that terminates when mTextureClients.size() <= sMinCacheSize; when do we expect if (mTextureClients.size() > sMinCacheSize) to fire right after that call?
I imagine we either want to shrink it right away (calling ShrinkToMinimumSize()) or leave it for later (the if that follows and the callback inside of it), but it doesn't seem to make sense to have both?

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 34 • 10 years ago

:nical, can you take a look at comment 33 question?

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 35 • 10 years ago

Come to think of it, you probably wanted ShrinkToMaximumSize() followed by the callback for shrinking to minimum size instead?

	Nicolas Silva [:nical] Assignee
	Comment 36 • 10 years ago

(In reply to Milan Sreckovic [:milan] from comment #35)
> Come to think of it, you probably wanted ShrinkToMaximumSize() followed by
> the callback for shrinking to minimum size instead?

I don't remember exactly, but I think you are right, we want ShrinkToMaximumSize here instead of ShrinkToMinimum size. Sotaro's initial patch did use ShrinkToMinimumSize, though (my addition to the patch only fixed the management of the mOutstandingClients counter). So Sotaro will probably be able to give you a more definite answer about the original intent.
But yeah, from the look of it I think that you are right, we probably confused the two shrink methods here.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 37 • 10 years ago

Thanks.  I have to patch on bug 1048984 for this and other issues with outstanding client tracking...

	Adam Alldredge [:AdamA][Inactive]
	Comment 38 • 10 years ago

Unable to verify, there are no steps to reproduce.