New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ie11 memory leak #429

Merged
merged 3 commits into from Sep 15, 2017

Conversation

Projects
None yet
3 participants
@adm90
Contributor

adm90 commented Jun 3, 2017

When IE 11 converts it to ZIP, it seems that it has not been released since it consumes a lot of memory.
This pull request implementing memory release.

  • Test file total size is 12.6 MB

  • Before: first, 92MB to 1.7GB, and not release memory.
    before

  • After: fixed, There is no problem even if repeating.
    after

If necessary, I will show sample code as well.

@Shammah

This comment has been minimized.

Show comment
Hide comment
@Shammah

Shammah Jul 6, 2017

I too had a memory leak problem with only IE11; when zipping a 12.6MB file my browser would allocate more than 1.3GB of memory, and the browser would hang. This was not the case in Edge and Chrome. After applying this patch, the memory leak was gone in IE11 and the browser could successfully zip.

Shammah commented Jul 6, 2017

I too had a memory leak problem with only IE11; when zipping a 12.6MB file my browser would allocate more than 1.3GB of memory, and the browser would hang. This was not the case in Edge and Chrome. After applying this patch, the memory leak was gone in IE11 and the browser could successfully zip.

@dduponchel

This comment has been minimized.

Show comment
Hide comment
@dduponchel

dduponchel Aug 19, 2017

Collaborator

I think it comes from #357 (released in v3.1.3). I will check if the version 3.1.2 has the same issue.

Collaborator

dduponchel commented Aug 19, 2017

I think it comes from #357 (released in v3.1.3). I will check if the version 3.1.2 has the same issue.

@adm90

This comment has been minimized.

Show comment
Hide comment
@adm90

adm90 Aug 20, 2017

Contributor

I confirmed it. As you said, there was no problem with version 3.1.2.

Contributor

adm90 commented Aug 20, 2017

I confirmed it. As you said, there was no problem with version 3.1.2.

@dduponchel

This comment has been minimized.

Show comment
Hide comment
@dduponchel

dduponchel Sep 10, 2017

Collaborator

I reproduced the issue on IE 11 and Edge. I found the issue on Microsoft bug tracker: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/7115840/

I find your fix is a bit too broad: MSBlobBuilder exists in both browsers, so I hesitate between your solution and a revert of #357:

  • in one case IE/Edge uses 2 times the memory needed by other browsers (which won't be a pleasant surprise), even if/when Edge fix the issue
  • in the other case everyone takes more memory but the behavior is more uniform
Collaborator

dduponchel commented Sep 10, 2017

I reproduced the issue on IE 11 and Edge. I found the issue on Microsoft bug tracker: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/7115840/

I find your fix is a bit too broad: MSBlobBuilder exists in both browsers, so I hesitate between your solution and a revert of #357:

  • in one case IE/Edge uses 2 times the memory needed by other browsers (which won't be a pleasant surprise), even if/when Edge fix the issue
  • in the other case everyone takes more memory but the behavior is more uniform
Blob creation: use the same code everywhere
This reverts e156d9c (which triggered the memory leak) and partially
reverts 72467ef.
Using the presence of `MSBlobBuilder` means we will get a different
behavior (double the amount of memory needed) only of IE11/Edge, even
after they fix the issue. It also means using feature detection to work
around bugs.
@dduponchel

This comment has been minimized.

Show comment
Hide comment
@dduponchel

dduponchel Sep 15, 2017

Collaborator

I'll take the revert of #357, I'll update this pull request to change the behavior.

Collaborator

dduponchel commented Sep 15, 2017

I'll take the revert of #357, I'll update this pull request to change the behavior.

@dduponchel dduponchel merged commit edc3323 into Stuk:master Sep 15, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@adm90 adm90 deleted the adm90:Fix_IE11MemoryLeak branch Sep 17, 2017

This was referenced Nov 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment