1057386 - Services should be able to release their resources shortly before we check for shutdown leaks to avoid false positives

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

We already do that right now in browser-test.js, which means it's the test suite's responsibility to call all those services. Letting services know when to destroy resources via an observer notification or maybe using AsyncShutdown would be great.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1057386-Provide-an-API-to-let-the-shutdown-leak-.patch

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Try run with --per-dir:

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

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8481956 [details] [diff] [review]
0001-Bug-1057386-Provide-an-API-to-let-the-shutdown-leak-.patch

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

::: testing/mochitest/browser-test.js
@@ -490,5 @@
>            SocialShare.uninit();
>            TabView.uninit();
> -
> -          // Destroying ContentSearch is asynchronous.
> -          promise = ContentSearch.destroy();

Forgot to remove the definition of |promise|, did it locally.

Comment 4

[Drew Willcoxon :adw]

Comment on attachment 8481956 [details] [diff] [review]
0001-Bug-1057386-Provide-an-API-to-let-the-shutdown-leak-.patch

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

Hmm, it seems like overkill to create a new jsm just to basically share a Barrier object.  But I don't have a better idea.

I think it would be better to simply set ShutdownLeaks to the Barrier instead of making it a wrapper for the Barrier and its client.  That would be a simpler design, and it would avoid having to change this module and its callers if the Barrier or client APIs change in the future.  But it's not a big deal, so I won't withhold r+ for it.

One other thing is that ShutdownLeaks is kind of a weaselly name.  What I mean is that this module exists only for shutdowns in the test harness, not all shutdowns.  I'd almost prefer for the harness to provide the module, and then clients in the code would try-catch their Cu.import()s.  There's no precedent for something like that as far as I know, but it doesn't seem inappropriate to me in this case.

Actually -- I do have one idea that might satisfy all three of those things, but maybe it's not better.  Right before it checks for leaks, the harness could broadcast a notification that includes a Barrier (via wrappedJSObject) that it creates locally.  Each interested client would observe it and add their blockers, and then the harness would wait.

::: browser/modules/ContentSearch.jsm
@@ +89,5 @@
>    // { controller, previousFormHistoryResult }.  See _onMessageGetSuggestions.
>    _suggestionMap: new WeakMap(),
>  
> +  // Tells whether we have already shut down.
> +  _destroyed: null,

Could you call this _destroyedPromise or something similar to indicate it's a promise?  It sounds like a bool as is.

@@ +98,5 @@
>        addMessageListener(INBOUND_MESSAGE, this);
>      Services.obs.addObserver(this, "browser-search-engine-modified", false);
> +
> +    ShutdownLeaks.addBlocker(
> +      "Wait until all current messages have been processed", () => this.destroy());

There's at most one current message, so this should say "Wait until the current message has been processed", although I think "Wait until the service is destroyed" is a little more accurate.  Also, the string should be prefixed with "ContentSearch:" -- even if the blocker is logged with a stack trace, but I don't know if that happens.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1057386-Provide-an-API-to-let-the-shutdown-leak-.patch, v2

Your idea about sending a notification with the barrier is great. Implemented that.

Comment 6

[Drew Willcoxon :adw]

Comment on attachment 8484217 [details] [diff] [review]
0001-Bug-1057386-Provide-an-API-to-let-the-shutdown-leak-.patch, v2

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

Nice!

::: browser/modules/ContentSearch.jsm
@@ +86,5 @@
>    // This is used to handle search suggestions.  It maps xul:browsers to objects
>    // { controller, previousFormHistoryResult }.  See _onMessageGetSuggestions.
>    _suggestionMap: new WeakMap(),
>  
> +  // Tells whether we have already shut down.

Nit: This still makes it sound like the var is a bool.  I'd say something like "Resolved when we finish shutting down."

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/55e95d529fd3

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/55e95d529fd3

Comment 9

[Marco Mucci [:MarcoM]]

Hi Tim, can you mark this as [qa+] or [qa-] in the whiteboard.