ibid mechanism broken

[katelynsills]

I came across an issue when I was trying to return an array that contains the same presence twice. It errors with the message, 'ibid out of range'.

When notifyFulfillToData (line 409 of liveSlots.js on my branch) is called, it has the following as arguments:

• promiseID: 20
• data: "[{"@\qclass":"slot","index":0},{"@\qclass":"ibid","index":1}]" (slashes added to not tag qclass)
• slots: [{type: "import", id: 11}]

MarkM says: "The ibid mechanism is broken because the replacer during serialization assigns ibid indices in preorder whereas the JSON reviver only sees objects in post order, and therefore has different ibid indices."

[warner]

Can you make this into a unit test? I just tried a simple set of arrays (serialize([a1,a2,a3,a3,a2,a1])) and it seems to serialize correctly, but that case isn't introducing any new objects in the post-replacer data, and it sounds like that's the core problem.

The test should probably go into test-marshal.js in "serialize exports". If you can write a test that fails and post it here (or put it on a branch), I'll try to find the fix.

[katelynsills]

Sure, sounds good!

…

On Mon, May 6, 2019, 9:01 AM Brian Warner ***@***.***> wrote: Can you make this into a unit test? I just tried a simple set of arrays ( serialize([a1,a2,a3,a3,a2,a1])) and it seems to serialize correctly, but that case isn't introducing any new objects in the post-replacer data, and it sounds like that's the core problem. The test should probably go into test-marshal.js in "serialize exports". If you can write a test that fails and post it here (or put it on a branch), I'll try to find the fix. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <https://github.com/Agoric/SwingSet/issues/30#issuecomment-489674970>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAST63PEOH2HU7K3YGDWJ6DPUBI4NANCNFSM4HKXWUPQ> .

[katelynsills]

I added a unit test here: Agoric/SwingSet@e6c6350

[warner]

Ah, yeah, this will be tricky. We're saving a lot of code by relying upon the built-in JSON serialization features. But the replacer hook of JSON.stringify is executed on the way into each node during a depth-first traversal, while the reviver hook of JSON.parse is run on the way out of each node. As a result, numbering the nodes by order of appearance to these hooks gets a completely different ordering.

Worst case we'll have to write our own deserializer. It might also be possible to serialize the data, unserialize it (without a reviver), then re-serialize it with a replacer that changes the ibid index for each node to match the other numbering order. Or do the same thing on the receiving side. Neither is particularly appealing.

[erights]

I have some ideas here, to talk about when we're next in at the same time. In the meantime, Agoric/SwingSet@508982d safely turns ibids off in a way that should work well enough in the absence of cycles. However, it will expand pass-by-copy dags into trees.

[warner]

That quick fix sounds good, and our marshalling layer will already combine multiple refs to the same import/export slot into a single index (even without the ibid mechanism). I'll land that now, and then we can defer the full fix until later.

[dckc]

Can you use a path rather than an index, a la cycle?

[erights]

Can you use a path rather than an index, a la cycle?

Perhaps. Opened https://github.com/Agoric/SwingSet/issues/66 to track. Closing this one though, since ibids with indexes are no longer broken.

[warner]

in the old repo. this was SwingSet issue 30