1392755 - lazyIdleThread::ShutdownThread failures in test automation

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

we have noticed more than a few intermittent failures, crashes, assertions, leaks which reference lazyIdleThread::Shutdown.

Possibly there is a single root cause, or there are many different failures and we end up closing this bug out.

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

:ryanvm, do you know who would know more about this set of failures and could look into the code?

Comment 2

[Ryan VanderMeulen [:RyanVM]]

froydnj would probably be who I'd start with (/me ducks)

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

Attachment: log excerpt

The LazyIdleThread is shutting itself down (for being idle?), and while it's spinning a nested event loop for that, QuotaManager tries to shut itself down.  So this is unexpected re-entrancy on the part of LazyIdleThread.

Relatedly, QuotaManager's use of LazyIdleThread causes other problems like confusing mozStorage and its memory pressure events because mozStorage latches the wrong event target.  (Unless LazyIdleThread's reported eventtarget has been cleaned up so that the LazyIdleThread event target is returned instead of the current nsThread in use?)  Plus, QuotaManager seems like one of the more latency-sensitive and important threads we have.

Not that we shouldn't fix LazyIdleThread, but :janv, how would you feel about switching to a normal persistent nsThread for QM?

Comment 4

[Jan Varga [:janv]]

Yeah, we don't need the "lazy" behavior anymore.

Comment 5

[Nathan Froyd [:froydnj]]

Andrew, are you volunteering for this?? :)

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

Yes, but due to scheduling concerns, Andrew from 1-1.5 days in the future is going to handle this.

Comment 7

[Intermittent Failures Robot]

1 failures in 908 pushes (0.001 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* try: 1

Platform breakdown:
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1392755&startday=2017-08-21&endday=2017-08-27&tree=all

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Any chance you'll get to this soon, Andrew? It's possible that eliminating the use of LazyIdleThread in the quota manager would fix bug 1391895.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

Yes; I should have this pushed to try and up for review later today.

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff23e1c4418a778dba42ff1c3167183df5553294

Comment 11

[Jan Varga [:janv]]

So simple :)

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Use a normal thread instead of LazyIdleThread for QuotaManager IO thread

QuotaManager is now a hip, popular subsystem used by many consumers, many of
which are now latency sensitive.  There is no longer any meaningful benefit to
using LazyIdleThread, but there are latency downsides.  Also, LazyIdleThread
has some bugs at shutdown and complications for callers that use
NS_GetCurrentThread().  So, begone LazyIdleThread!

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8902856 [details] [diff] [review]
Use a normal thread instead of LazyIdleThread for QuotaManager IO thread

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

(In reply to Jan Varga [:janv] from comment #11)
> So simple :)

Yeah, I thought it would be harder.  Also, apparently you're up, so I'll let you do the r+ honors.  Also, I apparently have no idea when you're awake.  I'm pretty sure it's all the time.

Comment 14

[Jan Varga [:janv]]

Comment on attachment 8902856 [details] [diff] [review]
Use a normal thread instead of LazyIdleThread for QuotaManager IO thread

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

Thanks for driving this!

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e0eb137038981bb7d8781d2842cdf982361c82c
Bug 1392755 - Use a normal thread instead of LazyIdleThread for QuotaManager IO thread. r=jvarga

Comment 16

[Andrew Sutherland [:asuth] (he/him)]

Now that the tree is open again and I've triaged the many non-android oranges to make sure there's no subtle regressions, I've pushed and we'll declare victory.  I saw no intermittents related to QM consumers, closest thing was LocalStorage bug 1347690 and that's only got storage in the name, it doesn't touch QM.  (And I'll be fixing that one as part of labeling in bug 1378716.)

Comment 17

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8902856 [details] [diff] [review]
Use a normal thread instead of LazyIdleThread for QuotaManager IO thread

OVERVIEW: This bug wants to be uplifted to make the tree less orange.  There is no risk to users.  RyanVM will throw a small celebration if this gets uplifted.

Approval Request Comment
[Feature/Bug causing the regression]:
Longstanding issue.

[User impact if declined]:
Users won't notice one way or the other unless they're running a debug build.  Then they'll notice fewer assertion-related crashes.

[Is this code covered by automated tests?]:
Yes.  They go orange more without this fix.

[Has the fix been verified in Nightly?]:
I verified locally on nightly, plus pushed to try on top of trunk and examined the failures to make sure there were no crazy invariants violated.

[Needs manual test from QE? If yes, steps to reproduce]:
No.  Not meaningfully possible to manually test.

[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
Without the patch, we use LazyIdleThread for the QuotaManager IO thread.  This is a normal thread wrapped so that the underlying "real" thread can be terminated when it hasn't done anything for a while.  We then spin up a new real thread when needed.  This wrapping is slightly buggy at shutdown and introduces edge-cases that are known to be problematic for subsystems like mozStorage's memory-pressure-triggered cleanup.  By eliminating the lazy thread and going to an always-there thread, we're eliminating these problems.

Additionally, we got a clean try run and all QuotaManager logic and consumer logic is inherently async and the use of LazyIdleThread or not using LazyIdleThread is not observable to them.  (Except for the edge cases.)

[String changes made/needed]:
No localized string changes made.

Disclaimer: Note that the thread name changed.  This is an internal-only debugging name exposed only to debuggers and system monitoring tools like "pstree" on linux.  The change from "Storage I/O" to "QuotaManager IO" is to aid people filing bugs or debugging Gecko.  "Storage" is an ambiguous term and a red herring, whereas QuotaManager IO exactly describes what's going on.

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6e0eb1370389

Comment 19

[Intermittent Failures Robot]

7 failures in 939 pushes (0.007 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* mozilla-beta: 6
* try: 1

Platform breakdown:
* linux64: 4
* windows8-64: 3

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1392755&startday=2017-08-28&endday=2017-09-03&tree=all

Comment 20

[Gerry Chang [:gchang]]

Comment on attachment 8902856 [details] [diff] [review]
Use a normal thread instead of LazyIdleThread for QuotaManager IO thread

Fix test automation failures. Beta56+.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cdd1559ed4d3