Js-ipfs: ipfs-message-port-* should ensure no transferables are duplicated

[Gozala]

Forking https://github.com/ipfs/js-ipfs/issues/3252#issuecomment-728471990 into separate issue.

Looks like some of our API calls return values that refer to same ArrayBuffer, which in turn causes one of the encoder functions https://github.com/ipfs/js-ipfs/tree/master/packages/ipfs-message-port-protocol/src to add it to the transfer list twice and error on postMessage.

[Gozala]

It should be fairly straight forward to fix this problem by switching from array of transferables to a set. Spec is bit vague about what transfer list should be https://html.spec.whatwg.org/multipage/web-messaging.html#dom-postmessageoptions-transfer (array or any iterable), but quick check on Firefox and Chrome suggest it works as expected.

[icidasset]

https://github.com/ipfs/js-ipfs/blob/530767d85d85b5932abdb87721e395c0a214b148/packages/ipfs-message-port-server/src/server.js#L214-L218

Could it be because we're sending the transfer list twice here?
In other words, shouldn't line 216 say value: value.value or something like that?
Since value contains the transfer array.

[Gozala]

Could it be because we're sending the transfer list twice here?

I don't believe so.

In other words, shouldn't line 216 say value: value.value or something like that?
Since value contains the transfer array.

I don't believe there is an issue with passing transfer as message data and there is no guarantee value.value will be a thing either, because this is the type of query.result:

https://github.com/ipfs/js-ipfs/blob/530767d85d85b5932abdb87721e395c0a214b148/packages/ipfs-message-port-protocol/src/rpc.ts#L9-L11

I do have to admit it does look a bit awkward, but I would consider improving it as a separate endeavor. You could probably easily verify that by changing line 217 to:

new Set(value.transfer || [])

[Gozala]

@icidasset since you're looking at it I've assigned it to you 🥺. If you find yourself unable to work on this please drop a message & I'll unassigned.

[icidasset]

I started working on this, but got stuck on a different error.

Is it me or does this Uint8Array look weird? It's length is supposedly 2, but the buffer looks entirely different.
And then it's claiming that value.Data isn't a buffer. That code worked perfectly before I started using this shared worker setup 🤔

Here are my changes so far:
https://github.com/ipfs/js-ipfs/compare/master...icidasset:unique-transferables

[icidasset]

Ok weird, I updated ipld-dag-pb to the latest version and added https://www.npmjs.com/package/buffer to our dependencies for the Buffer type, and now it that error is gone 🤔 🤷‍♂️

Anyhow, I made a PR to fix the issue at hand:
https://github.com/ipfs/js-ipfs/pull/3421

[Gozala]

Is it me or does this Uint8Array look weird? It's length is supposedly 2

That just means that Uint8Array is just view into the larger buffer e.g.:

const buffer = new ArrayBuffer(128)
const bytes = new Uint8Array(buffer, 0, 2)

[Gozala]

Here is the few things we need to do before we close this:

• [ ] Current solution eliminates duplicates in postMessage by doing [...new Set(...transfer)]. This works, but we should change transfer type to Set.
• [ ] postMessage can be passed a Set instance (or any iterable) as a transfer list, so we should not do conversion to an array.
• [ ] We should revisit tests and integrate it into existing ipfs-message-port-protocol tests to exercise moving nodes e.g with duplicates CIDs.

[icidasset]

@Gozala I'll take another look at this. I seem to have missed something which is causing a few occassional errors on Firefox. Will do the Set changes describe above as well.

Errors:

• postMessage, Arraybuffer at index 0 is already detached (error in ipfs-message-port-server, handleQuery)
• Attempting to access detached ArrayBuffer (error seems to be in js-ipfs itself 🤔 )