Implements globalThis slots #175

tars0x9752 · 2022-07-21T13:41:39Z

#164
Implements globalThis slots mentioned in #164 (comment) to fix the slots issue with dual package hazard.

12wrigja · 2022-07-22T17:29:31Z

Generally, this LGTM. I have a small preference that we check in some form of relevant test along side this change.

@ptomato and @justingrant - this makes the slot map accessible outside the module (and so I suppose users could mess with said slot map to cause all sorts of problems). Of course this diverges from the spec, but is that an acceptable change to you both / do either of you have thoughts on how two versions of this polyfill could "communicate" with each other to avoid dual package hazards?

ptomato · 2022-07-25T22:34:06Z

Symbol.for('@@Temporal__slots__private_do_not_use') seems "private" enough for friendly code, but not for adversarial code. I don't know if we want it to be a goal of this polyfill to be robust against adversarial code. Maybe we should discuss that.

I'm also curious about something — I can no longer find the comment but at one time I suggested that we migrate over time away from a private slots WeakMap and towards "Impl" objects that share their private members with each other. (This is not happening imminently; I haven't done any work whatsoever towards it.) I wonder if the dual package hazard would still be present in that case? I suspect it would, and would no longer be able to be solved by a global symbol.

12wrigja · 2022-07-26T22:50:40Z

migrate over time away from a private slots WeakMap and towards "Impl" objects

This sounds similar to #105 which was trying to replace the usage of GetIntrinsic with circular dependencies - maybe that helps narrow down where your comment was?

I think that would have similar problems - at a high-level, if we intend to verify that something "is a specific Temporal thing", then we need access to that from independent executions of a script in order to avoid dual-(multi-?)package hazards, and more generally we need to ensure that any local state that can change the output of function calls remains in sync between the N versions. Slots was the first thing that came to mind, but I haven't delved deeply into the other pieces of module-local state to see whether there's more problems here. Conversely, I think the caches we have would not be a problem though they would affect the performance of an application as the caches would be out of sync.

12wrigja · 2022-07-29T16:36:35Z

but not for adversarial code. I don't know if we want it to be a goal of this polyfill to be robust against adversarial code. Maybe we should discuss that.

Agreed, which is why I wanted your opinion. Unlike the capturing of VM intrinsic functions I don't think there's a way to make sure that the WeakMap we get back from this Symbol can only be changed by the polyfill sources - anyone could easily get this weakmap even after the polyfill sources have loaded and adjust the content.

12wrigja · 2022-09-06T19:55:22Z

I think we should move forward with this change, and we can consider alternatives to the static Symbol key (release build time hashing?) if needed.

My only concern is setting up some CI testing that covers this, which I think should probably end up as part of
#167.

ptomato · 2022-09-06T21:20:17Z

I wonder if we could avoid exposing the slot map, by instead exposing a function that gets a value from the slot map given an object and a slot name, and another function that sets all the slots at once of an object that wasn't previously in the slot map? That way, slots couldn't be overwritten once an object was created, so the objects would still be immutable even if user code accessed the private symbols.

12wrigja · 2022-09-16T22:03:37Z

I think that's a great idea - reducing the API surface down to only what we need seems reasonable.

@tars0x9752 is that something you can look into? We'd need two symbols, one for each exposed function, but that seems pretty reasonable. We probably also want to adjust the map to be "write-once" - if something already has set a key we should throw.

tars0x9752 · 2022-09-18T13:01:42Z

I'm not quit sure yet, but I'll look into it when I have time. And let me clarify one thing: When you say "expose", I read it as "expose through globalThis". Am I correct?

12wrigja · 2022-09-19T16:38:47Z

Right - in the same way we are currently making the map itself accessible via the globalThis[symbol].

tars0x9752 · 2022-10-01T16:27:23Z

I did a quick test, and I might be wrong, but just exposing CreateSlots and GetSlots seems to work even with the dual package. I'll do some more research when I have more time. BTW, Am I allowed to completely rewrite SetSlot and CreateSlots and where they are used?

ptomato · 2022-10-03T15:32:47Z

Depends on what rewriting you have in mind, I guess 😄 but that seems like it should be OK?

tars0x9752 · 2022-10-06T09:03:03Z

Now the fixes are almost done and here's how it looks like. Sorry if it's not what you guys wanted to be. If it looks OK, I'll commit these changes to this PR branch as well, and let me know if it's not what you guys are looking for.

tars0x9752@b52ad61

Here's what I do:

expose GetSlots and CreateSlots to avoid dual package hazards and to avoid exposing the slots itself.
rewrite the CreateSlots and SetSlot to adjust the map to be "write-once".
- and make sure SetSlot can only be used with CreateSlots.
a bit off topic but change shebang in test262.sh to be more portable. (#!/usr/bin/env bash instead of #!/bin/bash. Because I usually use NixOS and /bin/bash does not exist.)
I have confirmed that it passes test and test262, and the error does not occur, at least in a simple dual package hazard situation.

ptomato · 2022-10-06T22:17:15Z

I'd have probably changed CreateSlots to just take an argument that provides all the slots and their values at once, and removed SetSlot altogether. But I don't see anything wrong with doing it this way, either.

to avoid dual package hazards

tars0x9752 · 2022-10-09T10:52:53Z

Thanks for your responses. I've just updated this branch as well. And for now, I minimized the changes as in the comment here instead. Let me know if you prefer to remove SetSlot altogether.

12wrigja · 2023-03-10T00:37:47Z

lib/slots.ts

+    throw new TypeError('Missing slots for the given container');
+  }
+
+  if (id in slots) {


Wouldn't it be better to write this as

const existingSlot = slots[id];
if (existingSlot) throw new TypeError(...);
slots[id] = value;

Fixed it as suggested!

12wrigja

I'm so sorry @tars0x9752 - I didn't realize this was waiting for my review. One small comment, but otherwise this LGTM.

12wrigja · 2023-03-10T00:40:12Z

One more question: you confirmed this didn't have any dual-package hazards - is it possible (in a follow-up PR) to run an automated test for some of these things?

justingrant

Nice!

tars0x9752 · 2023-03-20T10:32:17Z

One more question: you confirmed this didn't have any dual-package hazards - is it possible (in a follow-up PR) to run an automated test for some of these things?

Sorry, honestly I forgot about the details of this PR. So I don't think I'll be able to provide such a follow-up PR anytime soon.

tars0x9752 · 2023-03-20T10:45:05Z

Oops, it looks like test262 is failing in Node v18 and above. 🤔 I'll look into it again when I have more time.

tars0x9752 · 2023-03-20T10:53:02Z

Hmm, I have a feeling that the problem is related to A NARROW NO-BREAK SPACE (U+202f), do you know what the cause is?

justingrant · 2023-03-20T16:58:26Z

Hmm, I have a feeling that the problem is related to A NARROW NO-BREAK SPACE (U+202f), do you know what the cause is?

Arrrgh, this was caused by a regression in V8 (https://bugs.chromium.org/p/chromium/issues/detail?id=1416538) which broke Test262. Specifically broke changes that I made recently to prevent Node-version-specific behavior from breaking tests. Sigh.

The fix will have to be made inside Test262.

As long as those are the only tests that are failing, @12wrigja I think it's OK to ignore those failures since they're unrelated to this PR.

12wrigja · 2023-03-22T00:41:59Z

I've sent #230 to add those to the expected failure list which will fix the CI.

Your changes otherwise look good to me, so I'm going to rebase this in. Thanks for contributing!

Use globalThis slots

6e3f4d6

tars0x9752 marked this pull request as ready for review July 21, 2022 13:56

12wrigja requested review from ptomato, 12wrigja and justingrant July 22, 2022 17:25

tars0x9752 added 3 commits October 9, 2022 18:11

Expose GetSlots and CreateSlots instead of the slots itself

24a1628

to avoid dual package hazards

Update shebang in test262.sh to be more portable

f1767a3

Add more precise types

829c58b

tars0x9752 force-pushed the global-slot branch from 9b5b46d to 829c58b Compare October 9, 2022 10:34

12wrigja reviewed Mar 10, 2023

View reviewed changes

12wrigja approved these changes Mar 10, 2023

View reviewed changes

justingrant approved these changes Mar 19, 2023

View reviewed changes

Apply suggested changes about existingSlot

5c2e9b3

12wrigja merged commit 73a0bf3 into js-temporal:main Mar 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements globalThis slots #175

Implements globalThis slots #175

tars0x9752 commented Jul 21, 2022

12wrigja commented Jul 22, 2022 •

edited

Loading

ptomato commented Jul 25, 2022

12wrigja commented Jul 26, 2022

12wrigja commented Jul 29, 2022

12wrigja commented Sep 6, 2022

ptomato commented Sep 6, 2022

12wrigja commented Sep 16, 2022

tars0x9752 commented Sep 18, 2022

12wrigja commented Sep 19, 2022

tars0x9752 commented Oct 1, 2022

ptomato commented Oct 3, 2022

tars0x9752 commented Oct 6, 2022

ptomato commented Oct 6, 2022

tars0x9752 commented Oct 9, 2022

12wrigja Mar 10, 2023

tars0x9752 Mar 20, 2023

12wrigja left a comment

12wrigja commented Mar 10, 2023

justingrant left a comment

tars0x9752 commented Mar 20, 2023

tars0x9752 commented Mar 20, 2023

tars0x9752 commented Mar 20, 2023

justingrant commented Mar 20, 2023

12wrigja commented Mar 22, 2023

Implements globalThis slots #175

Implements globalThis slots #175

Conversation

tars0x9752 commented Jul 21, 2022

12wrigja commented Jul 22, 2022 • edited Loading

ptomato commented Jul 25, 2022

12wrigja commented Jul 26, 2022

12wrigja commented Jul 29, 2022

12wrigja commented Sep 6, 2022

ptomato commented Sep 6, 2022

12wrigja commented Sep 16, 2022

tars0x9752 commented Sep 18, 2022

12wrigja commented Sep 19, 2022

tars0x9752 commented Oct 1, 2022

ptomato commented Oct 3, 2022

tars0x9752 commented Oct 6, 2022

ptomato commented Oct 6, 2022

tars0x9752 commented Oct 9, 2022

12wrigja Mar 10, 2023

Choose a reason for hiding this comment

tars0x9752 Mar 20, 2023

Choose a reason for hiding this comment

12wrigja left a comment

Choose a reason for hiding this comment

12wrigja commented Mar 10, 2023

justingrant left a comment

Choose a reason for hiding this comment

tars0x9752 commented Mar 20, 2023

tars0x9752 commented Mar 20, 2023

tars0x9752 commented Mar 20, 2023

justingrant commented Mar 20, 2023

12wrigja commented Mar 22, 2023

12wrigja commented Jul 22, 2022 •

edited

Loading