Memory leak when we interrupt the PDF loading

[xandao-dev]

How to reproduce

1. Go to the page that has the PDF and before it gets loaded exit and check the detached elements.

Observations

Tools used to identify the Memory Leak: Edge Detached Elements.

Our application: A SPA and I this specific page we change the path by calling $router.push.

Our implementation:

[hrynko]

Hi @xandao-dev,

Thanks for discovering this issue. I will try to fix it, but I would also appreciate the help in resolving it.

[xandao-dev]

@hrynko hey!

I was able to reproduce the issue in your project by slightly modifying the demo app to:

<template>
  <div>
    <button @click="enabled = !enabled">
      {{ enabled ? 'Disable' : 'Enable' }}
    </button>
    <vue-pdf-embed
      v-if="enabled"
      :image-resources-path="annotationIconsPath"
      :source="pdfSource"
    />
  </div>
</template>

<script>
import VuePdfEmbed from '../src/vue-pdf-embed.vue'

export default {
  components: {
    VuePdfEmbed,
  },
  data() {
    return {
      enabled: true,
      annotationIconsPath: '/node_modules/pdfjs-dist/web/images/',
      pdfSource: '/demo/sample-50-MB-pdf-file.pdf',
    }
  },
}
</script>

Note the pdf source of 50MB, we have to add it to the demo folder. Here is the link for the one I used: https://www.learningcontainer.com/sample-pdf-files-for-testing/

To check the memory leak, just spam clicks on the Enable/Disable button, and watch the detached elements tool.

[xandao-dev]

Looking closer at the first image I sent, it seems to be a problem with the pdf.GlobalWorkerOptions.workerPort

By moving the creation of the pdf worker instance to the Vue created lifecycle and then correctly terminating the worker in the beforeDestroy lifecycle hook, we can ensure proper worker termination.

I also read this issue about properly closing the loading task, but I haven't tested: mozilla/pdf.js#11595

[xandao-dev]

@hrynko Can you please check if there are any drawbacks to loading the instance during the created lifecycle? It seems to be the right way for me.

[hrynko]

Hi @xandao-dev,

Thank you for opening the PR, it looks good to me. I don't see any drawbacks in moving the worker initialization, however could you comment on the motivation for doing so?

[xandao-dev]

Sure, the worker does not get initialized again after termination in the beforeDestroy/beforeUnmount hook. Therefore, when the component is loaded again, it needs to be started again. And most important, it avoids terminating a worker from another component.

An important detail is that before we only had 1 instance, moving into the vue lifecycle we started to create one instance per component.

[xandao-dev]

If the worker is heavy, maybe it's an overkill to have an instance for each component. We do have alternatives, for example:

Stick with only one "global" (not really global, it belongs to a closure) worker, and in the beforeDestroy hook, we destroy the loading tasks instead of the worker.

Like so: loadingTask.destroy();

The option I followed is more memory-safe I guess, but it's also a bit more heavy (I don't know how much heavier).

[xandao-dev]

@hrynko hold on a bit, I would like to investigate a bit more with multiple components.

[xandao-dev]

The workers are not being removed (in that first solution should exist one per pdf):

I was dumb, GlobalWorkerOption has Global in its name for a reason. If you would like to use a worker for each instance you have to update other parts of the code. I tried to experiment with it to see if it handles better parallel PDF loading, but I wasn't able to accomplish it.

---

So let's take a step back and analyze the memory leaks again:
The this.document?.destroy() that already exists in the destruction hooks is enough to clean up the loading tasks and the worker.

The memory leaks were caused by event listeners, by cleaning them it was fixed.

I used this.documentLoadingTask.onProgress instead of this.document.loadingTask.onProgress because the latter was sometimes unreachable, possibly due to the promise not being fulfilled yet.
When testing, 'this.document?.destroy()' outperformed 'this.document.loadingTask?.destroy()', preventing rare console errors.

[xandao-dev]

Here is the template I used to test the memory leak and the issues with multiple pdfs:

<template>
  <div class="container">
    <div class="minw">
      <button @click="enabled = !enabled">
        {{ enabled ? 'Disable 1' : 'Enable 1' }}
      </button>
      <vue-pdf-embed
        v-if="enabled"
        :image-resources-path="annotationIconsPath"
        :source="pdfSource"
      />
    </div>

    <div class="minw">
      <button @click="enabled2 = !enabled2">
        {{ enabled2 ? 'Disable 2' : 'Enable 2' }}
      </button>
      <vue-pdf-embed
        v-if="enabled2"
        :image-resources-path="annotationIconsPath"
        :source="pdfSource"
      />
    </div>

    <div class="minw">
      <button @click="enabled3 = !enabled3">
        {{ enabled3 ? 'Disable 3' : 'Enable 3' }}
      </button>
      <vue-pdf-embed
        v-if="enabled3"
        :image-resources-path="annotationIconsPath"
        :source="pdfSource"
      />
    </div>

    <button
      @click="
        enabled3 = !enabled3
        enabled2 = !enabled2
        enabled = !enabled
      "
    >
      Toggle All
    </button>
  </div>
</template>

<script>
import VuePdfEmbed from '../src/vue-pdf-embed.vue'

export default {
  components: {
    VuePdfEmbed,
  },
  data() {
    return {
      enabled: true,
      enabled2: true,
      enabled3: true,
      annotationIconsPath: '/node_modules/pdfjs-dist/web/images/',
      pdfSource: '/demo/sample-50-MB-pdf-file.pdf',
    }
  },
}
</script>

<style lang="scss">
body {
  padding: 16px;
  background-color: #ccc;
}

.container {
  display: flex;
  justify-content: center;
  gap: 16px;
  max-width: 480px;
  margin: auto;
}

.minw {
  min-width: 300px;
}

.vue-pdf-embed {
  margin: auto;
  max-width: 480px;

  & > div {
    margin-bottom: 4px;
    box-shadow: 0 2px 8px 4px rgba(0, 0, 0, 0.1);
  }
}
</style>

[hrynko]

Some memory leaks were caused by event listeners, by cleaning them it was fixed.

So does this fix the initial issue? If so, could you please update the PR?

[xandao-dev]

@hrynko all done, PR updated.

[hrynko]

The issue seems to be back with your latest commit (still throwing RenderingCancelledException). Could you check if this enough to just reset onPassword and onProgress to null? Shouldn't documentLoadingTask be reset as well?

[xandao-dev]

@hrynko It's fine here. Should I've seen anything in the console?

ps. This detached element is from an extension and is not a memory leak.

[xandao-dev]

Shouldn't documentLoadingTask be reset as well?

this.document?.destroy() is basically destroying the documentLoadingTask. We can see it by debugging:

[hrynko]

I used your example with the Enable/Disable button and provided a large document. Once I click the "Disable" button (while loading the document), it throws the following error in the render method (as it was before the changes were made):

[xandao-dev]

Hmmm, can you share the pdf, please? This error is not happening here.
It's even better if you could share a stackblitz or codesandbox with the issue happening.

[hrynko]

@hrynko hey!

I was able to reproduce the issue in your project by slightly modifying the demo app to:

<template>
  <div>
    <button @click="enabled = !enabled">
      {{ enabled ? 'Disable' : 'Enable' }}
    </button>
    <vue-pdf-embed
      v-if="enabled"
      :image-resources-path="annotationIconsPath"
      :source="pdfSource"
    />
  </div>
</template>

<script>
import VuePdfEmbed from '../src/vue-pdf-embed.vue'

export default {
  components: {
    VuePdfEmbed,
  },
  data() {
    return {
      enabled: true,
      annotationIconsPath: '/node_modules/pdfjs-dist/web/images/',
      pdfSource: '/demo/sample-50-MB-pdf-file.pdf',
    }
  },
}
</script>

Note the pdf source of 50MB, we have to add it to the demo folder. Here is the link for the one I used: https://www.learningcontainer.com/sample-pdf-files-for-testing/

To check the memory leak, just spam clicks on the Enable/Disable button, and watch the detached elements tool.

I applied the latest changes from the PR and the demo code as above. The PDF used can be found by the following link: https://history.nasa.gov/alsj/a17/A17_FlightPlan.pdf

The only thing I added is the logging of the rendering-failed event. It happens when I click "Disable" for the first time, while the doc is loading.

[xandao-dev]

Ahhh, now I see, is just the component event:

The issue seems to be back

Well, that's not related to the initial issue and it's also not related to the memory leak. The behavior seems correct to me, it's an exception raised when the loading is interrupted.

[xandao-dev]

To make it clear to see what I solved, try to reproduce it with the detached elements tool available in the Edge browser.
You can also use the three-snapshot technique, but it's easier with this tool.

Spam the button for a while, hit to get detached elements, hit the garbage collection, and get detached elements again. Finally, click to analyze the memory.

A detached element is an element that has been removed from the DOM tree but is still referenced by JavaScript code. As a result, it cannot be garbage-collected, leading to a memory leak (not always). They can prevent an entire tree of components from being garbage collected with a simple small leak in an inner component.

[hrynko]

Ok, it's clear now. Is there anything else you would like to change/add in this PR?

[xandao-dev]

@hrynko we are good to go, thanks.

[xandao-dev]

@hrynko Hey, don't want to bother you, but when can I expect a release?

[hrynko]

Hi @xandao-dev,

It has already been released in v1.2.0. Thanks again for your efforts, your input is always welcome!