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

[Gozala]

Forking #3252 (comment) 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]

js-ipfs/packages/ipfs-message-port-server/src/server.js

Lines 214 to 218 in 530767d

	const value = await query.result
	port.postMessage(
	{ type: 'result', id, result: { ok: true, value } },
	value.transfer || []
	)

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:

js-ipfs/packages/ipfs-message-port-protocol/src/rpc.ts

Lines 9 to 11 in 530767d

	type Return<T> = T extends Promise<infer U>
	? Promise<U & TransferOptions>
	: Promise<T & TransferOptions>

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:
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:
#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 🤔 )

[icidasset]

Made a separate issue for the ArrayBuffer at index 0 error, not sure how to proceed with that one: #3480

[icidasset]

Made a PR: #3481
I addressed the Set iterable stuff there.