1161722 - At shutdown, multiple lines of "WARNING: Not expiration-tracking an unlocked surface!: file image/src/SurfaceCache.cpp, line 529"

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Daniel Holbert [:dholbert]]

Shutdown warning:
{
WARNING: Not expiration-tracking an unlocked surface!: file $SRC/image/src/SurfaceCache.cpp, line 529
}

I get multiple copies of this warning when my debug build shuts down -- I got 16 copies for one long-running session, and 2 copies for a short running session. In both sessions, I believe I'd only visited about:blank (and perhaps about:newtab).

The comment right before it indicates that nothing's actually wrong, if it happens during shutdown.  Ideally it'd be nice if we could silence this warning, to reduce warning-fatigue & avoid hiding other warnings that highlight actual issues.

Comment 1

[Daniel Holbert [:dholbert]]

> Ideally it'd be nice if we could silence this warning

...during shutdown.

Comment 2

[Daniel Holbert [:dholbert]]

(I'm not sure if there's an easy way to detect if we're in XPCOM shutdown, other than by registering an "xpcom-shutdown" observer, which might be overkill.)

Comment 3

[Seth Fowler [:seth] [:s2h]]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> (I'm not sure if there's an easy way to detect if we're in XPCOM shutdown,
> other than by registering an "xpcom-shutdown" observer, which might be
> overkill.)

In ImageLib, at least, there is indeed an easy way to check:

https://dxr.mozilla.org/mozilla-central/source/image/src/ShutdownTracker.h?from=ShutdownTracker#37

I don't think that existed when the code for the warning was added, though.

Checking if we're shutting down before printing the warning seems reasonable to me.

Comment 4

[Daniel Holbert [:dholbert]]

Nice! I was hoping there was something like that available.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Attachment: If we're shutting down, don't warn about untracked unlocked surfaces

This should do it!

Comment 6

[Seth Fowler [:seth] [:s2h]]

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

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8601707 [details] [diff] [review]
If we're shutting down, don't warn about untracked unlocked surfaces

Thanks!

>diff --git a/image/src/SurfaceCache.cpp b/image/src/SurfaceCache.cpp
>@@ -518,20 +519,21 @@ public:
>       if (MOZ_LIKELY(aSurface->GetExpirationState()->IsTracked())) {
>         mExpirationTracker.RemoveObject(aSurface);
>+      } else if (ShutdownTracker::ShutdownHasStarted()) {
>+        // AddObject failed in StartTracking because of shutdown. That's OK.
>       } else {
>-        // Our call to AddObject must have failed in StartTracking; most likely
>-        // we're in XPCOM shutdown right now.
>-        NS_WARNING("Not expiration-tracking an unlocked surface!");
>+        NS_WARNING("Not expiration-tracking an unlocked surface "
>+                   "outside of shutdown!");

So, this would make us call ShutdownTracker::ShutdownHasStarted() for no reason in opt builds, which is slightly wasteful.

Better to just leave the logic as-is & replace the existing NS_WARNING with:
  NS_WARN_IF_FALSE(ShutdownHasStarted(), "...");

r=me with that

Comment 8

[Seth Fowler [:seth] [:s2h]]

Ah, NS_WARN_IF_FALSE - forgot about that. That does seem better. Updated patch coming up. Thanks for the quick review!

Comment 9

[Daniel Holbert [:dholbert]]

(r=me if you want to upgrade to from NS_WARN_IF_FALSE to NS_ASSERTION, too; I'll trust your judgement on how worth it that is.  It looks we used to have a variant of this assertion, but we downgraded it in bug 1109283 because it was spammed during shutdown -- but now that won't happen anymore.)

Comment 10

[Seth Fowler [:seth] [:s2h]]

Attachment: If we're shutting down, don't warn about untracked unlocked surfaces

Here's the updated patch.

Comment 11

[Seth Fowler [:seth] [:s2h]]

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

Comment 12

[Seth Fowler [:seth] [:s2h]]

(In reply to Daniel Holbert [:dholbert] from comment #9)
> (r=me if you want to upgrade to from NS_WARN_IF_FALSE to NS_ASSERTION, too;
> I'll trust your judgement on how worth it that is.  It looks we used to have
> a variant of this assertion, but we downgraded it in bug 1109283 because it
> was spammed during shutdown -- but now that won't happen anymore.)

I think that's a good idea, but let's land it separately just on the off chance that that assertion *would* fire and it'd be tricky to fix.

Comment 13

[Seth Fowler [:seth] [:s2h]]

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

Comment 14

[Carsten Book [:Tomcat]]

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