Closed Bug 511440 Opened 13 years ago Closed 13 years ago

IPC protocol classes should use leak counting macros

Categories

(Core :: IPC, defect, P3)

x86
macOS
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: benjamin, Assigned: moz)

Details

Attachments

(1 file, 1 obsolete file)

In mozilla we typically use leak-counting macros to detect and prevent leaks: MOZ_COUNT_CTOR and MOZ_COUNT_DTOR. The IPDL-generated parent/child classes should use these macros in their constructor/destructors so that the automated leak-counting machinery can help us catch leaks.

See https://developer.mozilla.org/en/Debugging_memory_leaks#How_to_instrument_your_objects_for_refcnt.2fmemory_logging for some (sparse) documentation.
Status: NEW → ASSIGNED
Priority: -- → P3
Attached patch draft1 (obsolete) — Splinter Review
My first draft.

I used XPCOM_MEM_BLOAT_LOG to obtain a log of activity from a run of "firefox -chrome chrome://global/content/test-ipc.xul", to ensure that the constructors and destructors are being logged appropriately.  However, that does not exercise all of the objects generated by the IPDL compiler.  Does anyone have an idea of how to better exercise E10s?
That will exercize the tab objects but not the plugin objects, but as long as the patch is suitably generic, that sounds ok to me. Are there particular objects you're worried about that we aren't counting?
Yes. In the bloat log, I see evidence that an IFrameEmbeddingProtocolChild object was created, but not an IFrameEmbeddingProtocolParent. I would expect the latter. Also, I see no evidence that there has been any message object (like Msg_loadURL) created. It is as if there is no actual IPC taking place.
This is one of many cases where debugging information (leak stats in this case) is going to be confused between the parent and child process: you are seeing the leakstats for the chrome process, which will have IFrameEmbeddingProtocolChild objects (but not protocolchild objects). When the content process shuts down you should see IFrameEmbeddingProtocolParent object stats.

As far as I know currently we don't shut down the content process cleanly. I expect this means that the code you have here is correct, it's just not capable of being used yet on the content process.

As for message objects, it sounds like we should leak-check those as well. I don't know how those are managed though; cjones/bent can provide more guidance.
Attachment #397203 - Flags: review?(jones.chris.g)
The parent process will always create *ProtocolParent objects, and the child *ProtocolChild objects.  Robin, I don't know how to explain your observation without more information.

I don't think it's worth leak-checking Message objects.  These are automatically managed by IPDL; clients of IPDL never see them.
(In reply to comment #5)
> The parent process will always create *ProtocolParent objects, and the child
> *ProtocolChild objects.
> 

Whoops, nope!  bsmedberg is right.
Attachment #397203 - Attachment is obsolete: true
Attachment #397324 - Flags: review?(jones.chris.g)
Attachment #397203 - Flags: review?(jones.chris.g)
(In reply to comment #6)
> (In reply to comment #5)
> > The parent process will always create *ProtocolParent objects, and the child
> > *ProtocolChild objects.
> > 
> 
> Whoops, nope!  bsmedberg is right.

Whoops, nope!  bsmedberg is wrong.  I did away with this insanity a while back for this very reason, it was confusing.

So to be clear: the parent process will always create *ProtocolParent objects, and the child *ProtocolChild objects.
Attachment #397324 - Flags: review?(jones.chris.g) → review+
Pushed http://hg.mozilla.org/projects/electrolysis/rev/c25f8028ecdf
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.