feat(ses): redefine SharedSymbol to bypass Hermes prototype bug on obj literal short-hand methods

leotm commented 3 months ago

closes: #XXXX refs: https://github.com/endojs/endo/issues/1891

Description

Fix SES compat with Hermes when calling addIntrinsics(tameSymbolConstructor());

https://github.com/endojs/endo/issues/1891

Follow-up to

https://github.com/endojs/endo/pull/2108

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Compatibility Considerations

Upgrade Considerations

[ ] Includes *BREAKING*: in the commit message with migration instructions for any breaking change.
[ ] Updates NEWS.md for user-facing changes.

leotm commented 3 months ago

I'm confused as to what the problem is this is trying to solve?

How is the just created SharedSymbol function born with own non-configurable properties, conflicting with the original Symbol properties?

it's solving SES compatibility for React Native running on Hermes ^ when calling addIntrinsics(tameSymbolConstructor()); in the shim

otherwise the output on Android emulators is property is not configurable, no stack

Android emu > screenshot (older ses)
Android emu > screenshot 2 (ses@1.4.0)

after calling defineProperties(SharedSymbol, descs);

the original originalDescsEntries on Hermes logged out are

 LOG  length {"configurable": true, "enumerable": false, "value": 0, "writable": false}
 LOG  name {"configurable": true, "enumerable": false, "value": "Symbol", "writable": false}
 LOG  for {"configurable": true, "enumerable": false, "value": [Function for], "writable": true}
 LOG  keyFor {"configurable": true, "enumerable": false, "value": [Function keyFor], "writable": true}
 LOG  hasInstance {"configurable": false, "enumerable": false, "value": Symbol(Symbol.hasInstance), "writable": false}
 LOG  iterator {"configurable": false, "enumerable": false, "value": Symbol(Symbol.iterator), "writable": false}
 LOG  isConcatSpreadable {"configurable": false, "enumerable": false, "value": Symbol(Symbol.isConcatSpreadable), "writable": false}
 LOG  toPrimitive {"configurable": false, "enumerable": false, "value": Symbol(Symbol.toPrimitive), "writable": false}
 LOG  toStringTag {"configurable": false, "enumerable": false, "value": Symbol(Symbol.toStringTag), "writable": false}
 LOG  match {"configurable": false, "enumerable": false, "value": Symbol(Symbol.match), "writable": false}
 LOG  matchAll {"configurable": false, "enumerable": false, "value": Symbol(Symbol.matchAll), "writable": false}
 LOG  search {"configurable": false, "enumerable": false, "value": Symbol(Symbol.search), "writable": false}
 LOG  replace {"configurable": false, "enumerable": false, "value": Symbol(Symbol.replace), "writable": false}
 LOG  split {"configurable": false, "enumerable": false, "value": Symbol(Symbol.split), "writable": false}

so the conflict seems to be attempting to define 11 non-configurable properties to configurable

mhofman commented 3 months ago

otherwise the output on Android emulators is property is not configurable, no stack

Android emu > screenshot (older ses)

Android emu > screenshot 2 (ses@1.4.0)

after calling defineProperties(SharedSymbol, descs);

Flipper > Hermes Debugger > screenshot

Flipper > Hermes Debugger > screenshot 2

the original originalDescsEntries on Hermes logged out are

None of this tells me which non-configurable property exists on SharedSymbol that cannot be redefined. What is the actual trigger?

To be more clear, what are the own property descriptors of SharedSymbol right after construction?

leotm commented 3 months ago

otherwise the output on Android emulators is property is not configurable, no stack

Android emu > screenshot (older ses)

Android emu > screenshot 2 (ses@1.4.0)

after calling defineProperties(SharedSymbol, descs);

Flipper > Hermes Debugger > screenshot

Flipper > Hermes Debugger > screenshot 2

the original originalDescsEntries on Hermes logged out are

None of this tells me which non-configurable property exists on SharedSymbol that cannot be redefined. What is the actual trigger?

To be more clear, what are the own property descriptors of SharedSymbol right after construction?

getOwnPropertyDescriptors(SharedSymbol) appears to conform to the JS spec

{
   "arguments":{
      "configurable":false,
      "enumerable":false,
      "get":[
         "Function anonymous"
      ],
      "set":[
         "Function anonymous"
      ]
   },
   "caller":{
      "configurable":false,
      "enumerable":false,
      "get":[
         "Function anonymous"
      ],
      "set":[
         "Function anonymous"
      ]
   },
   "length":{
      "configurable":true,
      "enumerable":false,
      "value":1,
      "writable":false
   },
   "name":{
      "configurable":true,
      "enumerable":false,
      "value":"Symbol",
      "writable":false
   },
   "prototype":{
      "configurable":false,
      "enumerable":false,
      "value":{},
      "writable":true
   }
}

logging

// ...
  const descs=  fromEntries(
    arrayMap(originalDescsEntries, ([name, desc])=>  {
      console.log(name, desc);
      console.log(name, sharedSymbolDescs[name]);
// ...

we get

length {"configurable": true, "enumerable": false, "value": 0, "writable": false}
length {"configurable": true, "enumerable": false, "value": 1, "writable": false}

name {"configurable": true, "enumerable": false, "value": "Symbol", "writable": false}
name {"configurable": true, "enumerable": false, "value": "Symbol", "writable": false}

prototype {"configurable": false, "enumerable": false, "value": {}, "writable": true}

for {"configurable": true, "enumerable": false, "value": [Function for], "writable": true}
for undefined

keyFor {"configurable": true, "enumerable": false, "value": [Function keyFor], "writable": true}
keyFor undefined

hasInstance {"configurable": false, "enumerable": false, "value": Symbol(Symbol.hasInstance), "writable": false}
hasInstance undefined

iterator {"configurable": false, "enumerable": false, "value": Symbol(Symbol.iterator), "writable": false}
iterator undefined

isConcatSpreadable {"configurable": false, "enumerable": false, "value": Symbol(Symbol.isConcatSpreadable), "writable": false}
isConcatSpreadable undefined

toPrimitive {"configurable": false, "enumerable": false, "value": Symbol(Symbol.toPrimitive), "writable": false}
toPrimitive undefined

toStringTag {"configurable": false, "enumerable": false, "value": Symbol(Symbol.toStringTag), "writable": false}
toStringTag undefined

match {"configurable": false, "enumerable": false, "value": Symbol(Symbol.match), "writable": false}
match undefined

matchAll {"configurable": false, "enumerable": false, "value": Symbol(Symbol.matchAll), "writable": false}
matchAll undefined

search {"configurable": false, "enumerable": false, "value": Symbol(Symbol.search), "writable": false}
search undefined

replace {"configurable": false, "enumerable": false, "value": Symbol(Symbol.replace), "writable": false}
replace undefined

split {"configurable": false, "enumerable": false, "value": Symbol(Symbol.split), "writable": false}
split undefined

the issue appears to be the prototype

replacing the condition if (sharedSymbolDescs[name] && sharedSymbolDescs[name].configurable === false) { with if (name === 'prototype') { also works

erights commented 2 months ago

(rerunning failed CI jobs)

leotm commented 2 months ago

passing ^ looks good to approve then merge

leotm commented 2 months ago

sry @kriskowal looks like i added an extra mandatory approval required by you ^ i don't have merge auth so Zb's enabled auto-merge

mhofman commented 2 months ago

I am honestly unsure what the unmet merge requirement is. Will update the branch just in case

endojs / endo