remove parent window message handler if child iframe no longer in DOM

[srhyne]

This would what I experienced in #27 which was old messageHandler calls trying to connect to an iframe that no longer exists.

[Aaronius]

Hey @srhyne, thanks for opening the issue and this pull request. I'm glad you brought it up. It's indicative of a memory leak when the iframe is removed from the DOM but the consumer never calls connection.destroy(). In this case, the event listener that watches for messages on the parent is never removed and the handler (handleMessage) holds a reference to the iframe within its closure (as you've noted), so the iframe sticks around in memory too. I've verified that this memory leak does exist through some local testing and viewing memory snapshots.

Your PR removes the event listener on the next message that comes through, but I believe it has a couple weaknesses still that I think I'd like to cover. With the code as it exists in this PR:

• After the iframe is removed from the DOM, Penpal waits until the parent receives the next message before the event listener is removed. This means the iframe will remain in memory until the next message, if any, comes through. This is probably not a big deal as the memory footprint of an unloaded iframe is probably always small, but something I'd prefer to avoid if possible. I'd like to remove the event listener as soon as possible after the iframe is removed from the DOM in order to let it be removed from memory.
• If the consumer attempts to call a method on the child (e.g., child.multiply(2, 6)) any time after the iframe is removed from the DOM but before before any call to connection.destroy() is made, the consumer should probably get an error immediately instead of just never receiving a response. In order for the consumer to receive an error in this case, the code you wrote would need to call destroy() instead of just removing the event listener (destroy() also removes the event listener, but does more).

To resolve both of these issues, I'm considering adding a mutation observer to watch for the removal of the iframe and then where we add the iframe to the document:

(appendTo || document.body).appendChild(iframe);
detectElementRemoval(iframe, destroy);

I'm bummed about it because it will increase the file size by a few hundred bytes and having to run a mutation observer kind of sucks. I'm still thinking about this one, but wanted to let you know where I am with it.

As an interim solution to your specific problem, I could just change:

const child = iframe.contentWindow || iframe.contentDocument.parentWindow;

to

const child = iframe.contentWindow;

as I believe the last part is actually unnecessary.

[srhyne]

@Aaronius thank you for your very thoughtful review! Your points make total sense.

I actually started with a mutation observer but quickly realized that the mutation observer only listens for children of the target being destroyed. The observer callback won't fire if the target itself is removed. So the observer has to bind to body (each time) as the target not the appendTo element otherwise the observer might not fire when the appendTo (or higher parent) is removed.

You could register iframe references in memory and then have a single static observer watching for iframe's with X ID being removed and destroy that way, maybe?

[Aaronius]

@srhyne Thanks again for the PR and talking this through. I decided to not to go with mutation observers but took a different route instead. In short, I added some logic that will call destroy when a method is called from the parent and the iframe was previously removed. I also added a check that runs on an interval to see if the iframe is in the document. If it isn't, Penpal automatically calls destroy so that memory for that connection can get cleaned up. I've released these changes in v3.1.2.