863447 - Allow some time to complete async work if nsGlobalWindow appears leaked

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

By introducing the first indexedDB mochitest browser test in bug 861308, I found that indexedDB keeps mainthread objects alive until pending off main-thread work is complete. This may last a few milliseconds, though it totally confuses our leak detection, that runs as soon as the test completes.
This is a case I found, but I can easily see it may happen in other cases, especially now that we move towards more multi-threaded components, it may even be cause of some intermittent reported leaks.

I suggest when a leak is detected we "sleep" a small timeframe (I suggest 1s) and then we retry the leak checking.  This is not a perfect solution (we can't detect easily work completion on other threads), but should improve the situation without increasing much the time since it acts only when a globalwindow leak is detected.

I will try making the change and see how it behaves in the indexedDB case.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.0

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://tbpl.mozilla.org/?tree=Try&rev=1bbe1c1206fd

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

oh, well, looks like I must fix something in aboutHome.js, though should be part of bug 789348, I will do another try with just this patch.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 739512 [details] [diff] [review]
patch v1.0

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

Sounds reasonable to me as we're only waiting for a second when detecting a leak (which is the exception). Maybe we could also kick off a GC right before analyzing the CC graph again. smaug suggested doing this in another patch I was writing but I can't exactly remember why. Shouldn't hurt, though.

Also, <3 fat arrow functions.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Tim Taubert [:ttaubert] from comment #4)
> Maybe we could also kick off a GC right
> before analyzing the CC graph again.

afaict the analyzer already runs 3 gc before analyzing, am I wrong?

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Marco Bonardo [:mak] from comment #5)
> (In reply to Tim Taubert [:ttaubert] from comment #4)
> > Maybe we could also kick off a GC right
> > before analyzing the CC graph again.
> 
> afaict the analyzer already runs 3 gc before analyzing, am I wrong?

Right! Was just testing you, carry on :)

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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