Closed Bug 716523 Opened 8 years ago Closed 8 years ago

ClearOnShutdown() should assert if called after XPCOM shutdown


(Core :: General, defect)

Not set



Tracking Status
firefox12 --- fixed


(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)



(Whiteboard: [qa-])


(1 file, 2 obsolete files)

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.
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #586973 - Flags: review-
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"?
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #586973 - Attachment is obsolete: true
Attached patch Patch v2Splinter Review
Attachment #587084 - Attachment is obsolete: true
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".
Attachment #587085 - Attachment description: Patch v2 (better diff?) → Patch v2
Attachment #587085 - Flags: review?(benjamin)
Depends on: 715405
Attachment #587085 - Flags: review?(benjamin) → review+
Assignee: nobody → justin.lebar+bug
(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?
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.
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Is there a testcase for this bug which QA can use to verify the fix?
Keywords: testcase-wanted
Whiteboard: [qa?]
(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.
Keywords: testcase-wanted
Whiteboard: [qa?] → [qa-]
You need to log in before you can comment on or make changes to this bug.