Bug 1291709 - PdfjsChromeUtils.jsm leaks browser.xul windows.

[brendandahl]

https://bugzilla.mozilla.org/show_bug.cgi?id=1291709

[timvandermeij]

Looks good to me after reading about how WeakSet works. Could someone with more understanding about the extension code than me verify and merge?

[Snuffleupagus]

This seems fine to me, but the question is why it's actually necessary!?

Unless I'm mistaken, when the viewer unloads (see PdfStreamConverter.jsm#L831-L835) we send the PDFJS:Parent:removeEventListener message (see PdfStreamConverter.jsm#L857).
That message is then handled by the parent in PdfjsChromeUtils.jsm#L161-L162, and within the _removeEventListener method the browser in question is actually removed from the Set (see PdfjsChromeUtils.jsm#L236).

Why is the existing code not working as intended?
Could it be that because the message is asynchronous it arrives after the browser has already been thorn down, and the _removeEventListener method is thus never invoked!?

[brendandahl]

This seems fine to me, but the question is why it's actually necessary!?

Good point, looking a bit closer it seems we never receive the message PDFJS:Parent:removeEventListener in pdfjschromeutils.jsm. I see it being sent from pdfstreamconverter side though.

[Snuffleupagus]

Good point, looking a bit closer it seems we never receive the message PDFJS:Parent:removeEventListener in pdfjschromeutils.jsm. I see it being sent from pdfstreamconverter side though.

I've also done some quick testing, and it seems that things work as expected when navigating away from the PDF viewer (i.e. by going to a new web page in the same tab as the PDF viewer).
In contrast, what does not work is when the tab/window containing the PDF viewer is simply closed, which makes me suspect that my earlier idea might be correct:

Could it be that because the message is asynchronous it arrives after the browser has already been thorn down, [...]

There must be some kind of close/quit/... event that we could listen for to handle the latter case, but I'm wondering if it's worth the trouble of figuring that out if just using a WeakSet does the job!?

[wanderview]

There must be some kind of close/quit/... event that we could listen for to handle the latter case, but I'm wondering if it's worth the trouble of figuring that out if just using a WeakSet does the job!?

I think you can listener to the "unload" event on the browser window. This is the browser.xul window. Its not clear to me exactly what this corresponds to in the pdfjs extension, though.

We added something similar to AutoCompleteE10S.jsm:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/AutoCompleteE10S.jsm#97

I agree its better to just listen for the unload instead of using WeakSet.

[wanderview]

In auto complete code the "browser window" is coming from message.target.ownerDocument.defaultView:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/AutoCompleteE10S.jsm#199

[wanderview]

There was a comment that appears to have been deleted, but just to clarify:

I believe you have to listen for tab close and window close separately. This sucks and I would prefer that closing the window trigger tab closes as well, but it doesn't currently do it. Not sure if we can add that without breaking code that does not expect to get two close events.

[Snuffleupagus]

There was a comment that appears to have been deleted, but just to clarify:

Sorry about that; I removed my previous comment since I didn't really think it through properly.

I believe you have to listen for tab close and window close separately. This sucks and I would prefer that closing the window trigger tab closes as well, but it doesn't currently do it. Not sure if we can add that without breaking code that does not expect to get two close events.

That's unfortunate, and it confirms what I've seen when testing this. Trying to call e.g. _removeEventListener in all the various cases (i.e. navigating away from the viewer, closing the tab, and closing the window), seems to easily trigger the error in PdfjsChromeUtils.jsm#L233.

[timvandermeij]

Should we merge the patch in its current form? It would be good to stop leaking windows and from the comments above other approaches have obvious downsides. What do you think?

[Snuffleupagus]

Should we merge the patch in its current form? It would be good to stop leaking windows and from the comments above other approaches have obvious downsides. What do you think?

I'd be in favour of taking this PR in its current form as a temporary fix, so that we at least won't keep old browser.xul instances around forever.
However, I really think that we need to solve this properly, it's just that a complete solution (i.e. one covering: navigating away from the viewer, closing the tab, and closing the window) unfortunately doesn't seem entirely straightforward to implement.

[wanderview]

FWIW, I filed a bug to create a new event type that is fired for either tab close or window close:

https://bugzilla.mozilla.org/show_bug.cgi?id=1296099

Seems like the best long term solution to me. In the meantime, though, it would be nice to land the WeakSet(). I ran into this leak through normal browsing again today.