Closed Bug 1161722 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Graphics: ImageLib, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: dholbert, Assigned: seth)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
(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 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.
Nice! I was hoping there was something like that available.
This should do it!
Attachment #8601707 - Flags: review?(dholbert)
Assignee: nobody → seth
Status: NEW → ASSIGNED
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
Attachment #8601707 - Flags: review?(dholbert) → review+
Ah, NS_WARN_IF_FALSE - forgot about that. That does seem better. Updated patch coming up. Thanks for the quick review!
(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.)
Attachment #8601707 - Attachment is obsolete: true
(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.
Blocks: 1161743
https://hg.mozilla.org/mozilla-central/rev/fdec95a4cc19
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.