StructuredClone allocation failure #936
Comments
|
In general reliable detection of errors is an important feature... I don't think SharedArrayBuffer changes much here, actually, even if agents sharing memory can be more closely coupled and hence more vulnerable. In any event, IMO it might be most reasonable for no message to be delivered on the data channel -- "placeholder" data are not great -- but for a message to be delivered on the error channel instead. I suppose that message could contain whatever we serialized, but that seems less important than distinguishing between what was properly transmitted and what was not. All that said I don't actually know what will be most reasonable for web devs, of which I am not one. |
|
I'm still a little unclear what's best here. Per @domenic implementations end up creating an empty |
|
/cc @binji A bit of context at #2361 (comment) |
|
So it appears SharedArrayBuffer is not yet being shipped so we might have more wiggle room here than I thought. I see one outstanding question:
It's not clear to me that alignment here is necessary, as they are rather different failures. (In particular, the SAB case is a programming error---you shouldn't have wrote code to send it there---whereas the non-S AB case is an unpredictable runtime error.) Leaving non-shared ArrayBuffers alone for now, I think we have a few options for SharedArrayBuffers:
I guess I end up being a fan of (1), (2), or (5). In the end I think this is likely a programming error, so maybe fail-hard like (5) is the way to go? I'd love to get implementer (and ideally user!) feedback on what they think is best. |
|
(Per @lukewagner this will also affect |
|
I am not sure if I understand the concept of an "error sentinel", but I am in favor of (2) or (5). The way I use Also if my receiving code silently "works" the user probably would have expected a result different from what might turn out as a no-op. I'd like to get an error or something I can work with, not a "silent" failure. Now, optimally it would give me an error, but losing all data in the process isn't the greatest thing to work around, so intuitively I'd err on (2) but (5) is possible to live with and likely considerably cleaner. |
|
IIRC this was a problem I actually had when working on FirefoxOS (in the context of transferring photos between apps on a low-end mobile device): there was no way to tell when the transfer failed because of memory allocation failure. I don't have a strong opinion on how it is signaled, but I would have liked to have some kind of error signal two or three years ago. |
|
For "deliver a new messageerror event", is this messageerror event delivered to the sender or receiver? The sender is what I would expect and matches viewing this as a programmer error, but I wasn't sure this is what you meant. |
|
Oh, I meant to the receiver (so that, in (5) at least, the receiver receives exactly one of message or messageerror), although indeed maybe the sender should get one too... hmm. |
|
I think maybe the issue I'm remembering from my FirefoxOS days was the desire to get an exception when a memory allocation (typically for a large image transfer) failed. It is an issue that basically never effects desktop browsers, but was happening for me on mobile. I wanted an exception (on the sending side) but was getting a silent failure instead. Note, however, that if we only signal the error to the sender and not the receiver, then to write robust code, every postMessage() will need to be wrapped in try/catch That kind of boilerplate on the sending side will get old really fast. So the idea of having both message and messageerror events sounds great to me. In particular, Domenic's option #5 sounds like it preserves compatibility with existing code that does not listen for messageerror events, and it sounds like it could be made to work for any postMessage(), not just SharedArrayBuffers. |
|
FWIW, I have the impression that with message channels, it's possible to post on a channel before a receiver is attached, and I presume that it is possible for the sender to disappear from the sending end before a receiver is attached (as if the channel itself has been posted as a message on some channel but not yet been delivered - not sure what's possible). If so, it may not be possible to deliver an error to the sender. Even if the sender is still attached the postmessage may have returned long before the message is delivered, so there would need to be some kind of callback to deliver the message. |
|
I was mostly concerned with deserialization failure here, which is on the receiver side. Propagating that back would indeed require sending a message of sorts, which itself could "fail" if the sender got GC'd. Serialization (which happens on the sender side) can fail too due to OOM. That is already required to simply throw, just like |
|
It appears our understanding of this issue is not quite correct. We, or at least I, were assuming that UAs allocated the memory for the new ArrayBuffer only when the message was delivered. However, that does not appear to be what happens. Instead, UAs create a copy of the original memory at deserialization time. I assume they then branch on whether the destination is same-process or cross-process:
So allocation failure at deserialization time is only an issue cross-process, which is worth realizing. Importantly, this means that the serialization steps for ArrayBuffers should include making a copy of their bytes right away. You can't just store a pointer. |
|
You don't need to copy ArrayBuffer when transfering within a process (agent cluster). And if you transfer with MessageChannel and eventually end up in a different process a copy can be done late. (I was going to make an argument that we should consider not exposing the process boundaries, but SharedArrayBuffer already does that, so we might as well expose it for ArrayBuffer too.) |
|
Actually, if we want to allow for copy-on-write-or-allocation-of-new-ArrayBuffer, always exposing the failure late might be beneficial. |
|
I don't think we should force implementations to catch and store allocation errors, only to shuttle them over to the destination, just to support a hypothetical copy-on-write future. It's simpler to allow allocation errors to bubble up where they occur, IMO. If someone does a copy-on-write version then they'll need to throw the error at write time, which could be arbitrarily later (not just at receive time). My point above was that I at least didn't realize when allocation happened for the simple case like cloning (not tranferring) an array buffer. |
This rewrites most of the cloneable and transferable object infrastructure to better reflect the reality that structured cloning requires separate serialization and deserialization steps, instead of a single operation that creates a new object in the target Realm. This is most evident in the case of MessagePorts, as noted in #2277. It also allows us to avoid awkward double-cloning with an intermediate "user-agent defined Realm", as seen in e.g. history.state or IndexedB; instead we can simply store the serialized form and later deserialize. Concretely, this: * Replaces the concept of cloneable objects with serializable objects. For platform objects, instead of defining a [[Clone]]() internal method, serializable platform objects are annotated with the new [Serializable] IDL attribute, and include serialization and deserialization steps in their definition. * Updates the concept of transferable objects. For platform objects, instead of defining a [[Transfer]]() internal method, transferable platform objects are annotated with the new [Transferable] IDL attribute, and include transfer and transfer-receiving steps. Additionally, the [[Detached]] internal slot for such objects is now managed more automatically. * Removes the StructuredClone() abstract operation in favor of separate StructuredSerialize() and StructuredDeserialize() abstract operations. In practice we found that performing a structured clone alone is never necessary in specs. It is always either coupled with a transfer list, for which StructuredCloneWithTransfer() can be used, or it is best expressed as separate serialization and deserialization steps. * Removes IsTransferable() and Transfer() abstract operations. When defined more properly, these became less useful by themselves, so they were inlined into the rest of the machinery. * Introduces StructuredSerialzieWithTransfer() and StructuredDeserializeWithTransfer(), which can be used by other specifications which need to define their own postMessage()-style algorithm but for which StructuredCloneWithTransfer() is not sufficient. Closes #785. Closes #935. Closes #2277. Closes #1162. Sets the stage for #936 and #2260/#2361.
|
While working on this in #2518 I realized that if we want to do this properly we should do away with StructuredCloneWithTransfer so we can distinctly notify the sender (for serialization) and receiver (for deserialization) errors. The tight coupling would present a different API otherwise. This impacts |
|
That makes sense, although it'll have ripple effects on at least service worker; we should check the list of PRs I made. |
|
#2421 (comment) is the list. Only service workers and HTML itself will be affected. (Worklets don't seem to plan to allow transfers, but if they do we'll catch that on time.) |
|
This was meant to be closed by 25a94f6 but unfortunately the commit message mentioned an issue that was off-by-one. |
This rewrites most of the cloneable and transferable object infrastructure to better reflect the reality that structured cloning requires separate serialization and deserialization steps, instead of a single operation that creates a new object in the target Realm. This is most evident in the case of MessagePorts, as noted in whatwg#2277. It also allows us to avoid awkward double-cloning with an intermediate "user-agent defined Realm", as seen in e.g. history.state or IndexedB; instead we can simply store the serialized form and later deserialize. Concretely, this: * Replaces the concept of cloneable objects with serializable objects. For platform objects, instead of defining a [[Clone]]() internal method, serializable platform objects are annotated with the new [Serializable] IDL attribute, and include serialization and deserialization steps in their definition. * Updates the concept of transferable objects. For platform objects, instead of defining a [[Transfer]]() internal method, transferable platform objects are annotated with the new [Transferable] IDL attribute, and include transfer and transfer-receiving steps. Additionally, the [[Detached]] internal slot for such objects is now managed more automatically. * Removes the StructuredClone() abstract operation in favor of separate StructuredSerialize() and StructuredDeserialize() abstract operations. In practice we found that performing a structured clone alone is never necessary in specs. It is always either coupled with a transfer list, for which StructuredCloneWithTransfer() can be used, or it is best expressed as separate serialization and deserialization steps. * Removes IsTransferable() and Transfer() abstract operations. When defined more properly, these became less useful by themselves, so they were inlined into the rest of the machinery. * Introduces StructuredSerialzieWithTransfer() and StructuredDeserializeWithTransfer(), which can be used by other specifications which need to define their own postMessage()-style algorithm but for which StructuredCloneWithTransfer() is not sufficient. Closes whatwg#785. Closes whatwg#935. Closes whatwg#2277. Closes whatwg#1162. Sets the stage for whatwg#936 and whatwg#2260/whatwg#2361.
According to @bakulf Mozilla will not deliver a message when an object cannot be deserialized in a particular realm, e.g.,
ArrayBuffer's allocation exception. Instead, for workers anyway, the exception will go toonerror.Perhaps we should specify, maybe as part of #935, that some special error message gets delivered instead. E.g., we could set
datato the exception thrown or setdatato null and expose the exception separately. That way you don't get a global exception you don't know the origin of but instead a message you were expecting, just without the data to a problem.This is also important for
SharedArrayBufferas I understand it.@lars-t-hansen, @pizlonator, and @ajklein might have thoughts.
The text was updated successfully, but these errors were encountered: