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)
Core
Graphics: ImageLib
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.
Comment hidden (typo) |
Reporter | ||
Comment 2•9 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•9 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•9 years ago
|
||
Nice! I was hoping there was something like that available.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → seth
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=de940a11e67b
Reporter | ||
Comment 7•9 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•9 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•9 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•9 years ago
|
||
Here's the updated patch.
Assignee | ||
Updated•9 years ago
|
Attachment #8601707 -
Attachment is obsolete: true
Assignee | ||
Comment 11•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1308cc123bba
Assignee | ||
Comment 12•9 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 | ||
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdec95a4cc19
Comment 14•9 years ago
|
||
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.
Description
•