Skip to content
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

Remove definition of Blob.close() and all related terms. #68

Merged
merged 1 commit into from
Feb 10, 2017

Conversation

mkruisselbrink
Copy link
Collaborator

As discussed in #10, the utility of the close() method is unclear, and
properly specifying it would add a large amount of complexity.

By removing closing entirely, this fixes #10, fixes #17 and fixes #18.

References to readability state and blobs being closed in other specs (in particular HTML) will be removed in follow-up changes.

As discussed in #10, the utility of the close() method is unclear, and
properly specifying it would add a large amount of complexity.

By removing closing entirely, this fixes #10, fixes #17 and fixes #18.
@domenic
Copy link
Contributor

domenic commented Feb 9, 2017

Wow, I always thought this was implemented everywhere, but I guess not in Chrome and Firefox at least. Nice.

mkruisselbrink added a commit to mkruisselbrink/web-platform-tests that referenced this pull request Feb 9, 2017
@mkruisselbrink
Copy link
Collaborator Author

Wow, I always thought this was implemented everywhere, but I guess not in Chrome and Firefox at least. Nice.

Yeah, no browsers that I'm aware off ever shipped a non-prefixed version of Blob.close(). IE/Edge have a prefixed msClose, but I don't believe it really matches the behavior the spec would probably end up with if we'd keep it around/fix the definition. And Chrome has an even buggier implementation behind a flag that was never shipped at all.

@annevk
Copy link
Member

annevk commented Feb 10, 2017

This looks good. Thanks for doing this!

@annevk
Copy link
Member

annevk commented Feb 10, 2017

(And extra thanks for doing it properly with tests and such.)

@mkruisselbrink mkruisselbrink merged commit c9e0a6c into gh-pages Feb 10, 2017
mkruisselbrink added a commit to web-platform-tests/wpt that referenced this pull request Feb 10, 2017
* Update tests for removal of Blob.close() method.

Following the spec change in w3c/FileAPI#68

* isClosed was also removed
@domenic domenic deleted the remove-close-method branch February 10, 2017 18:25
scheib pushed a commit to scheib/chromium that referenced this pull request Aug 31, 2017
Chrome had an experimental implementation of a close() method (and
corresponding isClosed attribute) to allow web apps to explicitly
drop the resources associated with a Blob when no longer needed.
This was in the spec for a while but due to flaws [1] and lack of
implementation by other browsers, it was removed from the spec [2].

Remove the method/attribute and associated tests. It required the
"experimental" flag to use, so there is no risk of web breakage.

[1] w3c/FileAPI#10
[2] w3c/FileAPI#68

Bug: 344820
Change-Id: I6c8ff1f73590f40b68f2b862e15a9db3936bc0f1
Reviewed-on: https://chromium-review.googlesource.com/638915
Reviewed-by: Philip Jägenstedt <foolip@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Commit-Queue: Joshua Bell <jsbell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499019}
TimothyGu added a commit to node-fetch/node-fetch that referenced this pull request Feb 3, 2018
TimothyGu added a commit to node-fetch/node-fetch that referenced this pull request Feb 3, 2018
@guest271314
Copy link

Yeah, no browsers that I'm aware off ever shipped a non-prefixed version of Blob.close().

close was implemented at Chromium at some point recently.

Given that (when last checked) Chromium limits the number of Blob object which can be created at a document to 50, what is the current suggested or canonical procedure to free memory of an existing Blob?

@annevk
Copy link
Member

annevk commented Feb 19, 2018

close is not in Chrome. You can just let it GC. Also, I can easily allocate a 1000 in Chrome... Anyway, those type of usage questions are best asked on Stack Overflow going forward.

@guest271314
Copy link

@annevk

At some point Chromium did implement close https://github.com/guest271314/MediaFragmentRecorder/blob/master/demos/recordMediaFragments.html#L221.

You can just let it GC.

That is an extremely broad statement without clear parameters, including the specific processing steps of "just let it" before even reaching "GC".

What is the available RAM and HD space at the device where you are able to create 1000 Blob objects without the tab crashing?

Running this https://github.com/guest271314/MediaFragmentRecorder/blob/master/demos/recordMediaFragments.html, where multiple Blob and Blob URLs are created, multiple times at a device having minimal RAM or available HD space has arbitrarily and inconsistently crashed the browser tab at Chromium.

When the Blob or Blob URL was closed or revoked the same was reflected at chrome://blob-internals.

What is the canonical approach to remove/purge the Blob snapshot state from the file which stores that data at Chromium and Firefox?

@guest271314
Copy link

@annevk FYI

This account is temporarily suspended to cool down. The suspension period ends on Jan 30 '19 at 2:22.

Should probably stop asking questions and just write the code to the extent able and possible.

@mkruisselbrink
Copy link
Collaborator Author

Yes, chrome implemented some attempt of a close() method (never shipped it though), but it had many bugs (largely because the functionality was never really specified), and actually specifying what close would do would be really complicated. It also was never the case that closing a blob in chrome would immediately cause anything to be freed, as there could be other references to the same data, etc, so even back when chrome had a hard limit on how large the total number of blobs could be just closing one wouldn't be enough to know that you could create more, as closing/freeing memory/disk space is inherently an async operation.

@guest271314
Copy link

close had to have been shipped at some point. As did use the method in code within the past two years. Why cannot we simply free the reference regardless if any other code references the Blob?

@mkruisselbrink
Copy link
Collaborator Author

Nope, chrome never shipped close, you must be misremembering (or were running with experimental web platform features turned on). But anyway, actually defining what close would do is much more complicated than "simply free the reference", especially as implementations internally do all kinds of things to share data, asynchronously move data around, etc.

@guest271314
Copy link

@mkruisselbrink Yes, the --enable-experimental-web-platform-features flag was used with Chromium. The feature appeared to be helpful when using a HD with only 500MB of available disk space; unless what blob-internals listed were actually false-positives.

What is the goal moving forward to relevant to the ability to remove the Blob snapshot state to free memory? Or is the goal of implementing close or similar functionality now abandoned?

@Ajaay
Copy link

Ajaay commented May 9, 2019

I do think this warrants an answer or at least some direction. In our experience garbage collection does not remove blobs, and therefore the creation of a large number of blobs over time causes the memory used by Chrome to increase until it crashes.

The only way we found of getting around that was to use the close method in an electron app where we could enable experimental features without the user needing to do it on their standalone Chrome build.
We're now in a position where we cannot upgrade our Electron app because we lose the close method, which renders our app useless.

It's a reasonable question to ask what the proper method is of controlling memory use when creating large numbers of blobs on even reasonably specced machines.

@annevk
Copy link
Member

annevk commented May 9, 2019

@Ajaay did you file a bug against Chrome? Not sure how that's relevant to the standard, unless it would prevent GC somehow, but I don't think it does?

fatboy0112 added a commit to fatboy0112/node-fetch that referenced this pull request Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants