digidem / comapeo-mobile

The next version of Mapeo mobile
GNU General Public License v3.0
5 stars 0 forks source link

IPC calls lose data due to nodejs-mobile-react-native channel (de)serialization #392

Open achou11 opened 1 month ago

achou11 commented 1 month ago

Had a situation where from the client, I was calling mapeoApi.createProject(undefined) (note the explicit undefined argument). Doing that caused me to get an error on the client side:

 ERROR  [TypeError: Cannot destructure property 'name' of '(intermediate value)(intermediate value)(intermediate value)' as it is null.]
 Error: ENOENT: no such file or directory, open '/data/data/com.comapeo.dev/files/nodejs-builtin_modules/rn-bridge/index.js'
    at Object.readFileSync (node:fs:448:20)
    at getCodeFrame (/Users/andrewchou/GitHub/digidem/comapeo-mobile/node_modules/metro/src/Server.js:868:18)
    at Server._symbolicate (/Users/andrewchou/GitHub/digidem/comapeo-mobile/node_modules/metro/src/Server.js:945:22)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at Server._processRequest (/Users/andrewchou/GitHub/digidem/comapeo-mobile/node_modules/metro/src/Server.js:394:7) {
  errno: -2,
  code: 'ENOENT',
  syscall: 'open',
  path: '/data/data/com.comapeo.dev/files/nodejs-builtin_modules/rn-bridge/index.js'
}

turns out it's because of the combination of our IPC representation and how nodejs-mobile-react-native serializes data over its channels. the IPC represents the call into something like the following:

{"id": "@@manager", "message": [0, 2, ["createProject"], [undefined]]}

and this message gets passed to nodejs-mobile-react-native's channel.postMessage(): https://github.com/digidem/comapeo-mobile/blob/8b1917bc02dd57ab8c479865853e7e4492c7c3ed/src/frontend/lib/MessagePortLike.ts#L36

That channel method takes the message and does a simple JSON.stringify() on it, which is where the issue lies. Stringifying that message turns into the following:

'{"id":"@@manager","message":[0,2,["createProject"],[null]]}'

Note how that [undefined] has now become [null]! This seems to be the default behavior of JSON stringification so when it gets parsed and passed along to eventually call the associated server-side code, we're calling mapeoApi.createProject(null), which causes the error seen in the error message above.

big thanks to @EvanHahn who helped figure this out with me. not really sure what the best solution is, but there are a couple of ideas that we came up with that both don't seem great (but maybe fine in the short term).

achou11 commented 1 month ago

Another tangential item: we should really improve the error messages so that there's some semblance of a stack trace. Spent way too much time getting distracted by the ENOENT error, which was a red herring