Ryanwang/add pages api #67

RyanCommits · 2023-06-26T22:46:53Z

Add pages API to RN plugin.
Tested on Android and iOS

Fabric iOS: https://app.staging.fullstory.com/ui/KWH/session/4958113805828096:6257381269241856

Fabric Android:
https://app.staging.fullstory.com/ui/KWH/session/5046681232408576:5138233392824320

Non-Fabric Android:
https://app.staging.fullstory.com/ui/KWH/session/5495672610947072:6325248603914240

Non-fabric iOS:
https://app.staging.fullstory.com/ui/KWH/session/4678029697810432:6536142430601216

android/src/legacy/java/com/FullStoryModule.java

src/index.js

* Add Android pages implementation * updates on feedback - update legacy RN native module method declarations - replace System.out with android.util.Log - initialize reflected methods once - getUUID is async - fix page properties merge - add TS FSPage types * cleanup and fix turbomodule * remove private functions from export * remove page methods from types * add error message * generate UUID with Math.random() * remove async method * update method type to synchronous * update android log --------- Co-authored-by: Ryan Wang <[email protected]>

* Add Android pages implementation * updates on feedback - update legacy RN native module method declarations - replace System.out with android.util.Log - initialize reflected methods once - getUUID is async - fix page properties merge - add TS FSPage types * cleanup and fix turbomodule * remove private functions from export * remove page methods from types * add error message * generate UUID with Math.random() * remove async method * update method type to synchronous * implement ios pages API * formatting * formatting * Fix rebase regression * Edit error message --------- Co-authored-by: Ryan Wang <[email protected]>

Co-authored-by: Ryan Wang <[email protected]>

JoshMiers-FS · 2023-09-08T18:47:49Z

src/FSPage.ts

+
+  private static generateUUID() {
+    return 'xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx'.replace(/[x]/g, () => {
+      const char = Math.floor(Math.random() * 16);


Weird way to generate a UUID. Is this what JS limits us to?

Math.random() doesn't have to be very random. It could be implemented by a very low quality PRNG. V8, for example, looked at the statistical quality of their Math.random implementation in 2015 and improved it. Hermes' implementation of Math.random() uses std::uniform_real_distribution. The implementation notes indicate that most implementations for std::uniform_real_distribution will occasionally return the upper end of the range, so it is possible that the Hermes implementation of Math.random() occasionally returns 1.

Unfortunately, Hermes does not export Crypto.getRandomValues, so I think we have to trust its Math.random() implementation, which, as a recent addition to the C++ standard is probably reasonably high quality, other than the fact that we should explicitly wrap it with Math.min(Math.random(), 0.99999999999999988898) (some verification may be needed here)

It does appear that LLVM may have fixed the bug in 2020. (An older bug linked from the implementation notes above is still open, but I think this is an oversight.) Not sure Hermes is using the system's libc++, meaning that older devices will have the bug.

Relatedly, LLVM uses a linear congruential generator for std::uniform_real_distribution. LCGs have low statistical quality and it's possible that we're sampling the "less random bits" by doing the Math.floor(random*16). It might be worth generating a bunch of UUIDs and making sure they're all unique.

I think the easiest way to resolve this is to expose a native function that just returns [[NSUUID alloc] init].UUIDString and use that.

Reece and I had a discussion on the tradeoffs between using Math.random() and native UUID generation, for which we updated in the design doc, but here's our conclusion:

Due to the asynchronous nature of how React Native communicates between native code and JavaScript, a challenge to overcome is how to use native code to generate UUIDs. Since a new UUID needs to be generated on page creation, relying on native code to generate the UUID and communicate that to JavaScript will cause page creation to be asynchronous. To prevent asynchronous functionality, we implement a pseudo-random UUID generator on the JavaScript side using Math.random(). The web API ‘crypto’ is not available in React Native. Setting a page’s nonce value to a non cryptographically secure number generator runs the risk of allowing a bad actor with access to an org’s session data to tie users together from a split session using the page’s nonce value. However both the feasibility and likelihood of this risk is low, and functionality of the Pages API does not depend on a cryptographically secure nonce value. Math.random() is therefore sufficient in generating a unique identifier to identify user pages.

@ReeceLaF We can add a test to verify this x4 implementation against 4 applications of the straightforward implementation. I had gotten the same output in my local testing. @RyanCommits , should I create a branch with a commit you can cherry pick?

I'm also wondering why the hex8 function has a 0-bit shift and adds 2**32. Would this not work?

function hex8(n) { return (n|0).toString(16).toUpperCase(); }

If not, a clarifying comment would be great to explain why the other code is needed.

It may also be a good idea to set the version number and variant to conform to the UUID spec.

// Set the UUID v4 identifying bits to conform to the spec defined by [IETF RFC 4122](https://datatracker.ietf.org/doc/html/rfc4122#section-4.4) var b2 = b & 0xFFFF0FFF; // clear the version bits b2 |= 0x00004000; // set the version to 4 var c2 = c & 0x3FFFFFFF; // clear the variant bits c2 |= 0x80000000; // set variant to IETF return hex8(a)+"-"+hex8(b2).substring(0,4)+"-"+hex8(b2).substring(4)+"-"+hex8(c2).substring(0,4)+"-"+hex8(c2).substring(4)+hex8(d);

I agree that hex8 could use some comments. The 0 shift is (AFAICT) the only built in JS operator to reinterpret a number as an unsigned 32 bit int. Note that the bitwise operations take advantage of all 32 bits, so this is what we want. I verified that | 0 won't accomplish this. Adding 2**32 will turn the hex representation of n >>> 0 into 1XXXXXXXX, where XXXXXXXX is 8 hex digits, 0 left padded. Then we can strip off the 1 with the .substring(1).

This may be unclear in the code, but f() is designed to expose the entire state of xorshift128 to ensure no repetition. ("A single generated UUID, which is also 128 bits, reveals the entire state"). The design of f() may deserve an expository comment.

I verified locally that f() generates the same state bits as a C program with the copy and pasted xorshift128 code from Wikipedia, when the Wikipedia xorshift128 function is applied 4 times. ({a, b, c, d} equivalent to state if that's unclear.) I'm currently preparing a Jest test that verifies that 100 applications of f() with the initial state {a, b, c, d} = {1, 0, 0, 0} correspond to 100 applications of a Javascript-translated copy of xorshift128.

Regarding the f() function's correctness, note that the only xorshift128 mixed bits are placed in a equivalent to state[0], so repeated application of xorshift128 moves the mixed bits from 0 -> 1 -> 2 -> 3. After 4 applications, the bits in d will be the mixed bits from a.

Regarding IETF conformance, this is an interesting idea. This was also an issue with the previously proposed JS UUID generation strategy. We aren't generating "real UUIDs" since they are not cryptographic - we are just trying to generate unique values.

We would have to consider whether overwriting those bits would introduce a short cycle in the UUID generation. Currently, we're leveraging the work of Marsaglia, but this will require some novel analysis.

I also tested using the original copy-pasted algo in javascript 4 times to create the UUID and saw that it produced the same result. I'm still not sure how it works out to the same value, but it definitely does so it's good with me as is. As for IETF conformance, the UUID doesn't have to be cryptographically secure. Pseudo-random values for v4 are acceptable. It should be fine if the value isn't a "real" UUID though, so I am good to approve now.

JoshMiers-FS

Android LGTM

martin-fs

I think a couple of changes are needed related to the generation of UUIDs, and we need to do a little verification. Let me know what you think!

martin-fs · 2023-09-08T19:15:45Z

src/FSPage.ts

+  private static generateUUID() {
+    return 'xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx'.replace(/[x]/g, () => {
+      const char = Math.floor(Math.random() * 16);
+      return char.toString(16);


Note that NSUUIDs are composed of upper case digits, while Number.toString(radix) produces lower case digits. There is no documentation regarding the format accepted by +[NSUUID initWithUUIDString:] other than an example of "The standard format for UUIDs" which uses uppercase characters. So we should probably call String.toUpperCase on the result.

Added .toUpperCase() to UUID generator

martin-fs · 2023-09-08T19:22:16Z

src/FSPage.ts

+
+  private static generateUUID() {
+    return 'xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx'.replace(/[x]/g, () => {
+      const char = Math.floor(Math.random() * 16);


Math.random() doesn't have to be very random. It could be implemented by a very low quality PRNG. V8, for example, looked at the statistical quality of their Math.random implementation in 2015 and improved it. Hermes' implementation of Math.random() uses std::uniform_real_distribution. The implementation notes indicate that most implementations for std::uniform_real_distribution will occasionally return the upper end of the range, so it is possible that the Hermes implementation of Math.random() occasionally returns 1.

Unfortunately, Hermes does not export Crypto.getRandomValues, so I think we have to trust its Math.random() implementation, which, as a recent addition to the C++ standard is probably reasonably high quality, other than the fact that we should explicitly wrap it with Math.min(Math.random(), 0.99999999999999988898) (some verification may be needed here)

martin-fs

Thanks for implementing the new UUID generator! (I'm curious what others think about it, as well.)

ReeceLaF

Changes LGTM!

JoshMiers-FS · 2023-09-12T22:43:08Z

Great collaboration on the UUID updates @martin-fs @RyanCommits and @ReeceLaF !

RyanCommits assigned JoshMiers-FS Jun 26, 2023

JoshMiers-FS reviewed Jun 27, 2023

View reviewed changes

android/src/legacy/java/com/FullStoryModule.java Outdated Show resolved Hide resolved

JoshMiers-FS reviewed Jun 27, 2023

View reviewed changes

src/index.js Outdated Show resolved Hide resolved

RyanCommits assigned ReeceLaF Jul 26, 2023

rwang-fs added 3 commits August 25, 2023 13:19

add pages API to JS spec

0449d48

add pages API to iOS

ee3340f

add Android pages API

2959a07

RyanCommits force-pushed the ryanwang/add-pages-api branch from 26b3b1c to 2959a07 Compare August 25, 2023 17:19

RyanCommits and others added 4 commits August 31, 2023 14:13

Merge branch 'master' into ryanwang/add-pages-api

9488920

Improve Pages types and add testing (#76)

0b76aaf

Co-authored-by: Ryan Wang <[email protected]>

RyanCommits requested a review from JoshMiers-FS September 8, 2023 18:44

JoshMiers-FS reviewed Sep 8, 2023

View reviewed changes

JoshMiers-FS approved these changes Sep 8, 2023

View reviewed changes

RyanCommits assigned martin-fs Sep 8, 2023

martin-fs reviewed Sep 8, 2023

View reviewed changes

rwang-fs added 2 commits September 8, 2023 16:19

use uppercase UUIDs, exclude lib/ from jest

b44fd57

implement new uuid generator, update tests

555a312

RyanCommits requested a review from martin-fs September 11, 2023 14:35

martin-fs approved these changes Sep 11, 2023

View reviewed changes

ReeceLaF approved these changes Sep 12, 2023

View reviewed changes

RyanCommits merged commit 5424a5f into master Sep 13, 2023

RyanCommits deleted the ryanwang/add-pages-api branch September 13, 2023 14:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ryanwang/add pages api #67

Ryanwang/add pages api #67

RyanCommits commented Jun 26, 2023 •

edited

Loading

JoshMiers-FS Sep 8, 2023

martin-fs Sep 8, 2023

martin-fs Sep 8, 2023

martin-fs Sep 8, 2023

RyanCommits Sep 8, 2023

martin-fs Sep 11, 2023

ReeceLaF Sep 11, 2023 •

edited

Loading

martin-fs Sep 12, 2023 •

edited

Loading

martin-fs Sep 12, 2023

ReeceLaF Sep 12, 2023

JoshMiers-FS left a comment

martin-fs left a comment

martin-fs Sep 8, 2023

RyanCommits Sep 8, 2023

martin-fs Sep 8, 2023

martin-fs left a comment

ReeceLaF left a comment

JoshMiers-FS commented Sep 12, 2023

Ryanwang/add pages api #67

Ryanwang/add pages api #67

Conversation

RyanCommits commented Jun 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ReeceLaF Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

martin-fs Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshMiers-FS left a comment

Choose a reason for hiding this comment

martin-fs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-fs left a comment

Choose a reason for hiding this comment

ReeceLaF left a comment

Choose a reason for hiding this comment

JoshMiers-FS commented Sep 12, 2023

RyanCommits commented Jun 26, 2023 •

edited

Loading

ReeceLaF Sep 11, 2023 •

edited

Loading

martin-fs Sep 12, 2023 •

edited

Loading