-
Notifications
You must be signed in to change notification settings - Fork 46
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
Blob.close() has several problems #10
Comments
So as far as I can tell nobody implements |
Note that if we decide to keep and implement it, we need to carefully audit all callsites that take a blob and decide how they should handle a closed blob. |
It's (unfortunately?) not true that nobody implements So I don't think just getting rid of |
That sounds reasonable. If you can put the processing model here for review before proceeding with enabling it by default that would be good. I'd like Firefox engineering to have a chance to review it first. |
So I played around with this a bit more myself, trying to figure out what sensible behavior would be for a bunch of situations. And I wrote some tests to see what Chrome and Edge currently do. Modulo throwing different types of exceptions Edge at least passes (almost all) those tests. Chrome unfortunately fairs a bit worse, mostly due to bugs in our implementation. But writing those tests also made me realize that no matter how we treat closed Blobs in the various APIs, it's probably never going to be entirely nice and consistent. Naively I would expect things to behave such that any kind of cloning of a blob/passing a blob to some kind of API behaves as if a completely new blob, with a completely new copy of the data was created. As such calling close() on one blob should not effect previously clones of that blob in any way (where clone means structured clone, A more tricky question is what various methods should do when a closed Blob is passed to them. For consistency it could make sense to act similarly to how neutered array buffers are treated (for cases where both blobs and array buffers are valid options). That would mean that trying to postMessage a closed Blob would fail with a DataCloneException, but for example trying to pass a closed Blob as body to an XHR.send, or as body for a Response or Request will result in a valid zero-byte body instead (why is that the case for neutered array buffers anyway?) Or maybe a more consistent model would be that any method that takes a Blob should throw if passed a closed blob, as it almost certainly is an error to pass a closed Blob to XHR.send, new Response, etc. Finally there is the question what to do with blobs returned by various methods. XHR.response is documented to return the same Blob every time it is called, so closing that blob would really close the blob. On the other hand what should FormData.get do? Currently that method is seemingly specified to return the same File instance every time it is called (as it just returns the internal state directly), but what should happen if you close that returned instance? So yeah, actually figuring this all out is far from trivial. And I'm sure I missed cases in my tests. Coming back to my current attempt at tests, Edge passes all my tests except that trying to XHR.send a closed blob throws an exception (yet posting a closed blob using fetch(new Request(...)) "works" and just posts zero bytes). Chrome is kind of all over the place... |
And I wonder if Microsoft has any usage stats for their Blob.msClose method... |
The behavior for a detached ArrayBuffer has historical baggage and still the specification and implementations are not aligned. If we could just throw here that would be better I think, but you'll keep issues such as adding a Blob to a FormData instance and then closing it… |
I think I've actually changed my mind about Blob.close(). The main use-case I've heard for it is to allow websites to make sure memory is freed up when the website no longer needs the blob, to then for example allow it to create more blobs. But that particular use-case wouldn't actually be supported very well by Blob.close() as specified. One issue being that at least in the chrome implementation chrome might not actually be able to free up memory until some time later. And in that case it's not much better than just relying on garbage collection. Possibly a close method that returned a promise (that would resolve when the memory has been freed) would be more useful for this, but would also get even more confusing to explain to web developers. Also the fact that various ongoing usages of the blob might or might not keep the memory alive anyway seems like it would make it very difficult to explain what closing a blob would actually do, other than "the memory might get freed at some point in the future", which garbage collecting the Blob instance itself already does... |
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}
Not defined what happens with
b
inProbably it should not be closed.
We need to define how this affects other specifications such as
FormData
,XMLHttpRequest
, etc.If
blob.close()
revokes URLs it needs to be defined how that happens. Are the URLs stored on the instance?The text was updated successfully, but these errors were encountered: