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

RESOLVED FIXED in Firefox 40

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dholbert, Assigned: seth)

Tracking

Trunk
mozilla40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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 hidden (typo)
(Reporter)

Comment 2

4 years ago
(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.)
(Assignee)

Comment 3

4 years ago
(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.
(Reporter)

Comment 4

4 years ago
Nice! I was hoping there was something like that available.
(Assignee)

Comment 5

4 years ago
Created attachment 8601707 [details] [diff] [review]
If we're shutting down, don't warn about untracked unlocked surfaces

This should do it!
Attachment #8601707 - Flags: review?(dholbert)
(Assignee)

Updated

4 years ago
Assignee: nobody → seth
Status: NEW → ASSIGNED
(Reporter)

Comment 7

4 years ago
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+
(Assignee)

Comment 8

4 years ago
Ah, NS_WARN_IF_FALSE - forgot about that. That does seem better. Updated patch coming up. Thanks for the quick review!
(Reporter)

Comment 9

4 years ago
(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.)
(Assignee)

Comment 10

4 years ago
Created attachment 8601714 [details] [diff] [review]
If we're shutting down, don't warn about untracked unlocked surfaces

Here's the updated patch.
(Assignee)

Updated

4 years ago
Attachment #8601707 - Attachment is obsolete: true
(Assignee)

Comment 12

4 years ago
(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.
(Assignee)

Updated

4 years ago
Blocks: 1161743
https://hg.mozilla.org/mozilla-central/rev/fdec95a4cc19
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.