Webviewer Memory leaks - loadScript.js & webviewer-ui.js

[dil-mwigley]

Hello,
I believe I have found some memory leaks in version 5.1.10 of the web viewer. Tested on Windows Server 2016 running Chrome 75.

In src/helpers/loadScript.js there is a window.addEventListener('message'... call that isn't properly unsubscribed:

const loadConfig = () => {
  const _loadConfig = (e, resolve) => {
    if (e.data.type === 'responseConfig') {
      loadScript(
        e.data.value,
        'Config script could not be loaded'
      ).then(() => {
        window.removeEventListener('message', _loadConfig);
        resolve();
      });
    }
  };

  return new Promise(resolve => {
    window.addEventListener('message', e => _loadConfig(e, resolve));

    if (process.env.NODE_ENV === 'development') {
      loadScript(getHashParams('config', '')).then(() => {
        window.removeEventListener('message', _loadConfig);
        resolve();
      });
    }
    window.parent.postMessage('requestConfig', '*');
  });
};

The code is adding an event listener to an anonymous function but is then trying to remove _loadConfig which is not subscribed to window.addEventListener.

The code should be changed to something like:

const loadConfig = () => {

  return new Promise(resolve => {
    const _loadConfig = (e) => {
      if (e.data.type === 'responseConfig') {
        loadScript(
          e.data.value,
          'Config script could not be loaded'
        ).then(() => {
          window.removeEventListener('message', _loadConfig);
          resolve();
        });
      }
    };

    window.addEventListener('message', _loadConfig);

    if (process.env.NODE_ENV === 'development') {
      loadScript(getHashParams('config', '')).then(() => {
        window.removeEventListener('message', _loadConfig);
        resolve();
      });
    }
    window.parent.postMessage('requestConfig', '*');
  });
};

There are also two issues causing memory leaks in webviewer.js (one similar to the above, and one due to closing over options:

    window.WebViewer.l = function() {
      return options.l || options.licenseKey;
    };

    window.WebViewer.workerTransportPromise = function() {
      return options.workerTransportPromise;
    };

    var me = this;
    window.addEventListener('message', function(event) {
      if (event.source !== me.iframe.contentWindow) {
        return;
      }

      if (event.data === 'requestl') {
        event.source.postMessage({
          type: 'responsel',
          value: options.l || options.licenseKey
        }, '*');
      }
      if (event.data === 'requestConfig') {
        var configURL = options.config
          ? me._correctRelativePath(options.config)
          : '';

        event.source.postMessage({
          type: 'responseConfig',
          value: configURL
        }, '*');
      }
    }, false);

The window.WebViewer.l and window.WebViewer.workerTransportPromise function assignments are causing memory leaks. Changing those lines to:

    window.WebViewer.l = function() {
      return this.options.l || this.options.licenseKey;
    };

    window.WebViewer.workerTransportPromise = function() {
      return this.options.workerTransportPromise;
    };

somewhat but does not fully mitigate the memory leaks (this.options is still closed over, causing a small leak) - I believe one way to fully mitigate this would be to set objects instead of functions.

The window.addEventListener('message',... code has a similar problem to the loadScript.js code. One fix is to change the code to:

    var me = this;
    var licenseKeyListener = function(event) {
      if (event.source !== me.iframe.contentWindow) {
        return;
      }

      if (event.data === 'requestl') {
        event.source.postMessage({
          type: 'responsel',
          value: options.l || options.licenseKey
        }, '*');
        window.removeEventListener('message', licenseKeyListener);
      }
    }
    var configListener = function(event) {
      if (event.source !== me.iframe.contentWindow) {
        return;
      }

      if (event.data === 'requestConfig') {
        var configURL = options.config
          ? me._correctRelativePath(options.config)
          : '';

        event.source.postMessage({
          type: 'responseConfig',
          value: configURL
        }, '*');
        window.removeEventListener('message', configListener);
      }
    }

    window.addEventListener('message', licenseKeyListener, false);
    window.addEventListener('message', configListener , false);

(assuming that the license and options don't need to be sent to the window multiple times - if they need to be sent multiple times then a more complex mitigation will be required).

Currently I am observing a ~10mb memory leak each time I load the PDF viewer - which is a rather catastrophic memory leak. With these changes the leaks become small but not necessarily non-existent (I think my changes fully fix the window.addEventListener issues but not the other issues). I was not experiencing these memory leaks with version 5.0.0 of the viewer.

[mparizeau-pdftron]

Thanks for the detailed information!

For the first part with the UI that's definitely a mistake and we'll fix that shortly.

For the second part about webviewer.js I had a few more questions about how you're loading (and possibly reloading?) WebViewer. Are you instantiating WebViewer multiple times and throwing away the WebViewer iframe each time you're done with it? If not, could you describe in a bit more detail how you're running your test?

For assigning window.WebViewer.l and window.WebViewer.workerTransportPromise I believe if the WebViewer constructor is called a second time then these variables would be reassigned which means that the old functions would be garbage collected and nothing would be leaking there. We're holding onto the options object for the life of WebViewer, but I doubt that your options object would be taking up 10MB.

Note that this implementation is pretty similar to the 5.0 implementation which looked like this:

window.WebViewer = {
  l: function() {
    return options.l;
  },
  workerTransportPromise: function() {
    return options.workerTransportPromise;
  }
};

Also unfortunately since the functions aren't arrow functions this.options won't be defined, but I'm not sure this will make a difference even if we changed it to an arrow function.

You're right that for the message listener we're never actually removing it, so if you instantiate WebViewer multiple times and then remove it from the DOM and no longer reference the variable then there will be a leak.

From a quick look, in theory the events should only happen once each, but another option might be to add a dispose function to WebViewer that you can call when you're finished using it. The dispose function would just remove the message listener.

One note is that generally we recommend reusing a single WebViewer instance if possible. Of course that might not work for your scenario. Also interestingly 5.0 had a message event listener in the constructor for the license request so it may just be the problem with the UI that is mainly causing the issue you're seeing.

Let me know how the dispose function sounds and if you could provide a bit more information about how you're running the test and loading multiple WebViewer instances that would be great!

[dil-mwigley]

Thanks for the quick reply; sorry for the delay getting back to you!

You're right that my l: function() { and workerTransportPromise: function() changes are incorrect. Sorry! And you're right that changing it to an arrow function wouldn't help.

And yep I also see the lack of removeEventListener in the 5.0.0 webviewer.js. If I only apply the UI change I still get the memory leak and my webviewer.js changes make it go away, but obviously my proposed fix is in the wrong place; I'll do some more digging to see if I can find the actual source of the regression.

Are you instantiating WebViewer multiple times and throwing away the WebViewer iframe each time you're done with it?

Yes that's right. So we have the web viewer inside an Angular component that gets destroyed each time we close a book (and recreated when we reopen it). I believe we were having layout issues with re-inserting a copy of the pdf viewer instead of recreating it from scratch each time a new book is opened - I think that the original method of not destroying the pdf viewer involved having it inside a position: absolute and that meant we couldn't use flexboxes for our layout. That being said I think that as long as it's OK to take the pdf viewer out of the DOM then it should be easy for us to only initialise the pdf viewer once so I'll have a go at that.

A dispose function would be fine for us.

[dil-mwigley]

Alright so what I've found is that if I remove the license part of the webviewer.js addEventListener code (we don't use it) and change our webviewer.js code to:

    window.WebViewer.l = function() {
      return options.l || options.licenseKey;
    };

    window.WebViewer.workerTransportPromise = function() {
      return options.workerTransportPromise;
    };
    var configListener = function(event) {

      if (event.source !== me.iframe.contentWindow) {
        return;
      }

      if (event.data === 'requestConfig') {
        var configURL = options.config
          ? me._correctRelativePath(options.config)
          : '';

        event.source.postMessage({
          type: 'responseConfig',
          value: configURL
        }, '*');
        window.removeEventListener('message', configListener);
      }
    }

    window.addEventListener('message', configListener , false);

without changing anything else then the memory leak goes away. This is pretty confusing since the 5.0 code was doing:

    window.addEventListener('message', function(event) {
      if (event.data === 'requestl') {
        event.source.postMessage({
          type: 'responsel',
          value: options.l
        }, '*');
      }
    }, false);

but if I re-add that code then the memory leak reappears. I'm assuming there's something deeper within the viewer that's the actual source of the leak but I can't find it - the chrome dev tools keep taking me to these lines of code as the source of the leak. Maybe this information will help you to track the root cause down though :)

[mparizeau-pdftron]

Thanks for the update! We've made some progress on this but still need do some more testing and cleaning up of the potential fixes. I think we've identified most of the issues and we should have more information for you on this next week.

[mparizeau-pdftron]

@dil-mwigley We have a build now that you can try out http://www.pdftron.com/ID-zJWLuhTffd3c/WebViewer/WebViewer_5.2.0.72175-leak-fix.zip

This build adds a dispose function on the WebViewer instance that you can call just before the element is removed so probably in your ngOnDestroy function.

Let us know if this fixes the issue for you!

[dil-mwigley]

Yes calling dispose in the new build seems to fix the issue. Thank-you!

[mparizeau-pdftron]

That's great to hear! The fix will part of our stable nightly build for tomorrow that you'll be able to download here https://www.pdftron.com/nightly/?p=stable/