Last Comment Bug 716523 - ClearOnShutdown() should assert if called after XPCOM shutdown
: ClearOnShutdown() should assert if called after XPCOM shutdown
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla12
Assigned To: Justin Lebar (not reading bugmail)
:
:
Mentors:
Depends on: 715405
Blocks: 706958 715308
  Show dependency treegraph
 
Reported: 2012-01-09 05:57 PST by Justin Lebar (not reading bugmail)
Modified: 2012-03-31 11:42 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Patch v1 (1.77 KB, patch)
2012-01-09 05:58 PST, Justin Lebar (not reading bugmail)
justin.lebar+bug: review-
Details | Diff | Splinter Review
Patch v2 (7.17 KB, patch)
2012-01-09 12:38 PST, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch v2 (7.13 KB, patch)
2012-01-09 12:40 PST, Justin Lebar (not reading bugmail)
benjamin: review+
Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2012-01-09 05:57:22 PST
ClearOnShutdown isn't going to work if you call it after XPCOM shutdown.  I guess if it's called early enough after shutdown, the event loops will still be running, and we could post an event to the event loop to clear the pointer soon.  But probably better just to assert.
Comment 1 Justin Lebar (not reading bugmail) 2012-01-09 05:58:52 PST
Created attachment 586973 [details] [diff] [review]
Patch v1
Comment 2 Justin Lebar (not reading bugmail) 2012-01-09 06:04:03 PST
Comment on attachment 586973 [details] [diff] [review]
Patch v1

Actually, this is not going to work.  It could be that ClearOnShutdown() observes XPCOM shutdown before its caller does.  In that case, we'll assert, but the caller never had an opportunity to notice that XPCOM has shut down!

We could expose a "shutting down" bool on xpcom, and set it to true before we fire any shutdown notifications.  But then I bet callers are just going to have to guard their ClearOnShutdown() calls with that bool.

Maybe it's better to figure out how to get trace-malloc not to care about these "leaked" objects.  That's the whole reason ClearOnShutdown() exists, anyway.
Comment 3 Justin Lebar (not reading bugmail) 2012-01-09 06:17:48 PST
dbaron, you seem to own trace-malloc.  Would you be OK with adding an API to trace-malloc which says "treat this object as free'd"?
Comment 4 Justin Lebar (not reading bugmail) 2012-01-09 12:38:32 PST
Created attachment 587084 [details] [diff] [review]
Patch v2
Comment 5 Justin Lebar (not reading bugmail) 2012-01-09 12:40:42 PST
Created attachment 587085 [details] [diff] [review]
Patch v2
Comment 6 Justin Lebar (not reading bugmail) 2012-01-09 12:45:45 PST
Comment on attachment 587085 [details] [diff] [review]
Patch v2

The problem that this fixes is: While XPCOM shutdown notifications are being sent to observers, things can still happen.  In particular, it's not outrageous for someone to call ClearOnShutdown() at this point.  So we really need to wait until all the event queues are quiet before null'ing out the ClearOnShutdown()'ed pointers.

Accepting suggestions for a better name than "KillClearOnShutdown".  I just couldn't bring myself to do "ShutDownClearOnShutdown".
Comment 7 Justin Lebar (not reading bugmail) 2012-01-26 16:33:08 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/289a4f7f390a
Comment 8 David Baron :dbaron: ⌚️UTC-10 2012-01-26 16:58:10 PST
(In reply to Justin Lebar [:jlebar] from comment #3)
> dbaron, you seem to own trace-malloc.  Would you be OK with adding an API to
> trace-malloc which says "treat this object as free'd"?

What for?  And how does it relate to this bug?
Comment 9 Justin Lebar (not reading bugmail) 2012-01-26 17:06:40 PST
Well, that would be a different way of handling this bug.  At least, I thought it would be.

ClearOnShutdown (at least at the moment) only clears pointers for the sake of convincing trace-malloc that the pointer was freed.  Nobody relies on the clearing of pointers for correctness.

So my thought was that instead of clearing a pointer, we could tell trace-malloc that the pointer has been freed, and that would be sufficient.

But I realize now that there are at least few problems with this:

 * If you have a clear-on-shutdown'ed nsTArray, you need to delete that object so it deletes its buffer.  If we told trace-malloc that the TArray was cleared, it would still think we were leaking the buffer.

 * It's a sign of a problem if an object doesn't get deleted after its clear-on-shutdown'ed pointer is deleted.  That means someone else is holding a reference to the object, probably incorrectly.  Again, the current implementation would detect this, while the one suggested in this comment wouldn't.
Comment 10 Matt Brubeck (:mbrubeck) 2012-01-27 09:08:42 PST
https://hg.mozilla.org/mozilla-central/rev/289a4f7f390a
Comment 11 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-29 15:29:15 PDT
Is there a testcase for this bug which QA can use to verify the fix?
Comment 12 Justin Lebar (not reading bugmail) 2012-03-31 11:42:24 PDT
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #11)
> Is there a testcase for this bug which QA can use to verify the fix?

No; this was precautionary.

Note You need to log in before you can comment on or make changes to this bug.