ClearOnShutdown() should assert if called after XPCOM shutdown

RESOLVED FIXED in Firefox 12

Status

()

Core
General
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

unspecified
mozilla12
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox12 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

6 years ago
Created attachment 586973 [details] [diff] [review]
Patch v1
(Assignee)

Comment 2

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

Comment 3

6 years ago
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"?
(Assignee)

Comment 4

6 years ago
Created attachment 587084 [details] [diff] [review]
Patch v2
(Assignee)

Updated

6 years ago
Attachment #586973 - Attachment is obsolete: true
(Assignee)

Comment 5

6 years ago
Created attachment 587085 [details] [diff] [review]
Patch v2
(Assignee)

Updated

6 years ago
Attachment #587084 - Attachment is obsolete: true
(Assignee)

Comment 6

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

Updated

6 years ago
Depends on: 715405
Attachment #587085 - Flags: review?(benjamin) → review+
(Assignee)

Updated

6 years ago
Assignee: nobody → justin.lebar+bug
(Assignee)

Comment 7

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/289a4f7f390a
status-firefox12: --- → fixed
(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?
(Assignee)

Comment 9

6 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/289a4f7f390a
Status: NEW → RESOLVED
Last Resolved: 6 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?]
(Assignee)

Comment 12

5 years ago
(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.