Fixing chrome debugging on v10 #3411

kraenhansen · 2020-11-18T14:28:25Z

What, How & Why?

This closes #3358 and #3361 by

Fixing the implementation of the RPC stub for App
Introducing a RPCNetworkTransport implementation of the GenericNetworkTransport provided by object store. This struct is statically provided a function (assigned by the RPCServer when a session gets created), which calls back into the browser to execute fetch requests, ultimately on behalf of object store.
Bonus: Introduced a better serialization+deserialization of Error instances to propagate the stack when an error is thrown during an RPC call.

☑️ ToDos

📝 Changelog entry
📝 Compatibility label is updated or copied from previous entry
🚦 Tests
📝 Public documentation PR created or is not necessary
💥 Breaking label has been applied or is not necessary

geragray · 2020-11-19T19:20:08Z

@Anzormumladze I understand your frustration when it comes to debugging with JS SDK. You are right, this is a well know issue (explained in detail here #491). We are hoping to fix it by investing in #2455. Please subscribe.

I would also like to point out that leaving derogatory comments is against our code of conduct https://realm.io/conduct/. Please refrain from using foul language in the future.

kevinrodriguez-io · 2020-11-20T04:34:34Z

@kraenhansen You're doing an amazing work to get this out. Tell me if I can help with something, looking forward to fix this issue for our developer team.

Anzormumladze · 2020-11-20T18:34:11Z

@Anzormumladze I understand your frustration when it comes to debugging with JS SDK. You are right, this is a well know issue (explained in detail here #491). We are hoping to fix it by investing in #2455. Please subscribe.

I would also like to point out that leaving derogatory comments is against our code of conduct https://realm.io/conduct/. Please refrain from using foul language in the future.

thanks for answering me back, I deleted all my comment and looking forward to seeing good news

kneth

I think you need to add the new network transport to Android.mk.

lib/browser/rpc.js

kneth · 2020-11-23T08:30:11Z

src/js_app.hpp

    std::string user_agent_binding_info;
    auto user_agent_function = js::Object<T>::get_property(ctx, realm_constructor, "_createUserAgentDescription");
    if (js::Value<T>::is_function(ctx, user_agent_function)) {
        auto result = js::Function<T>::call(ctx, js::Value<T>::to_function(ctx, user_agent_function), realm_constructor, 0, nullptr);
        user_agent_binding_info = js::Value<T>::validated_to_string(ctx, result);
    }
+    else {
+        // FIXME: When debugging RN apps, _createUserAgentDescription cannot be found


We will have to create a callback to the browser to get it to work. On the other hand, while debugging, it is not so important.

That's a great point - I overlooked this.
It's probably not that interesting to know the version of the Chrome browser running this.
I wonder if there's a way to make object store not send any device information at all?

kneth · 2020-11-23T08:30:28Z

src/js_app.hpp

    std::string user_agent_binding_info;
    auto user_agent_function = js::Object<T>::get_property(ctx, realm_constructor, "_createUserAgentDescription");
    if (js::Value<T>::is_function(ctx, user_agent_function)) {
        auto result = js::Function<T>::call(ctx, js::Value<T>::to_function(ctx, user_agent_function), realm_constructor, 0, nullptr);
        user_agent_binding_info = js::Value<T>::validated_to_string(ctx, result);
    }
+    else {
+        // FIXME: When debugging RN apps, _createUserAgentDescription cannot be found
+        user_agent_binding_info = "N/A";


Suggested change

user_agent_binding_info = "N/A";

user_agent_binding_info = "RN debugging";

I understand that this would be the case from a practical level, but if we had a bug in our extension code the _createUserAgentDescription method might not be assigned and then I think it's misleading to send "RN debugging".

I think it's a bit pessimistic to optimize for the case where we have bugs in our code. We should instead assume that the code works as intended and provide the most relevant UX to users.

I think it's a bit pessimistic to optimize for the case where we have bugs in our code.

I disagree: In my opinion, making little to no assumption between effect (the function is undefined) and the potential cause (running in chrome debugger mode) which sometimes are unrelated (something didn't load properly), makes it easier to debug code. In other words, the link between cause and effect should be as direct as possible.

The suggested change is too specific to the situations that might be causing the function to be undefined. In my opinion, the "right" solution would be to provide an authoritative runtime check which determines is we're running in the chrome debugger mode, but I honestly see very little value in that, since these values likely wont be relevant to the developer through the MongoDB Realm admin dashboard anyway and it won't be trivial to provide meaningful values.

I agree that an unambiguous check if the code is running in a debugger is better, but don't agree with the rest. I don't think a user will easily make the connection that "js-unknown" means a debugger, so is likely to be left with the impression that either our code or theirs is broken and doesn't correctly report the platform.

If this function is only unrefined in a debugger, then using that as a check, even if semantically incorrect, is better than providing a broken user experience.

In pre-v10 we skipped setting it as object store didn't require it. With v10, it is now a requirement. You can say that we are doing more or less the same as before.

kneth · 2020-11-23T08:30:50Z

src/js_app.hpp

@@ -185,6 +201,12 @@ void AppClass<T>::constructor(ContextType ctx, ObjectType this_object, Arguments
        config.platform_version = js::Value<T>::validated_to_string(ctx, Object::get_property(ctx, result_object, platform_version_name));
        config.sdk_version = js::Value<T>::validated_to_string(ctx, Object::get_property(ctx, result_object, sdk_version_name));
    }
+    else {
+        // FIXME: When debugging RN apps, _createPlatformDescription() cannot be found
+        config.platform = "N/A";


Suggested change

config.platform = "N/A";

config.platform = "RN debugging";

For context, this was how the previous react native SDK reported its platform:
https://github.com/mongodb/stitch-js-sdk/blob/310f0bd5af80f818cdfbc3caf1ae29ffa8e9c7cf/packages/react-native/core/src/core/auth/internal/StitchAuthImpl.ts#L141-L143

Same as the other comment . I would suggest setting this to "js-unknown" to follow the pattern used by the legacy SDK and at the same time signal that the platform isn't known at this point. It'll most likely be react native chrome debugging, but we're checking for the existence of a function and not actually that we're running in that environment.

keep up the good work guys. Anxiously waiting for this bug fix ! @kneth @kraenhansen

src/js_app_credentials.hpp

kraenhansen · 2020-11-23T10:13:25Z

I think you need to add the new network transport to Android.mk.

If that was the case, I'm wondering why the integration tests are passing?

I think it doesn't have to be added since it's a header and those are taken care of on a per directory level here: https://github.com/realm/realm-js/blob/master/react-native/android/src/main/jni/Android.mk#L95

src/RealmJS.xcodeproj/project.pbxproj

src/js_app.hpp

src/rpc.cpp

lib/browser/rpc.js

blagoev · 2020-11-24T11:44:29Z

src/js_app.hpp

+    auto realm_constructor = js::Value<T>::validated_to_object(ctx, js::Object<T>::get_global(ctx, "Realm"));
+
+    Protected<typename T::GlobalContext> protected_ctx(Context::get_global_context(ctx));
+    config.transport_generator = [protected_ctx] {


We should be using this syntax everywhere we have to protect the context
https://github.com/realm/realm-js/blob/master/src/js_types.hpp#L708
this helps with making sure we don't introduce unintentional bugs cause of the two ctx names existing in the same scope

blagoev · 2020-11-24T11:52:50Z

src/js_app.hpp

    std::string user_agent_binding_info;
    auto user_agent_function = js::Object<T>::get_property(ctx, realm_constructor, "_createUserAgentDescription");
    if (js::Value<T>::is_function(ctx, user_agent_function)) {
        auto result = js::Function<T>::call(ctx, js::Value<T>::to_function(ctx, user_agent_function), realm_constructor, 0, nullptr);
        user_agent_binding_info = js::Value<T>::validated_to_string(ctx, result);
    }
+    else {
+        // The `_createUserAgentDescription` method is undefined when chrome debugging a React Native app


this function needs to accommodate that it might get executed in RN environment and we should call it here

I don't understand this comment ☝️ Can I get you to rephrase it?

In the past, a sync session should be opened without setting user_agent_binding_info so we silently skipped the call when the RN debugger was active: https://github.com/realm/realm-js/blob/v6/src/js_sync.hpp#L62-L65

_createUserAgentDescription should be called or some other mechanism introduced so we are able to get the result we are looking for. Seems like _createUserAgentDescription should be changed to support when running under chrome debugger which is what the PR is fixing.

blagoev · 2020-11-24T11:57:13Z

src/jsc/jsc_rpc_network_transport.hpp

+*/
+
+/**
+ * Provides an implementation of GenericNetworkTransport for use when the library is loaded in a runtime which doesn't provide the APIs required to perform network requests directly.


I don't see the need to implement this. we should be able to use the JSNetworkTransport.

Can you provide an explanation to this? What makes you think it would be better to use the JSNetworkTransport? That is basically just a wrapper around some JavaScript code which won't be loaded in the case of us running in chrome debugger mode. I see no advantage in trying to polyfill the _networkTransport property from C++.

The way I see it is we have JavaScriptNetworkTransport which is a generic class to do network requests. It should be possible to use it instead of defining a new one for the debug context. The way it works is it just needs a global.Realm._networkTransport.fetchWithCallbacks in order to work.

That was my initial thought as well. However it turned out to be way easier to implement an alternative specialisation of GenericNetworkTransport than providing a polyfill for fetchWithCallbacks implemented in C++. I'm sure you would have reached the same conclusion. From your comment, I don't see sufficient value in changing this approach and I'll leave it as is.

blagoev · 2020-11-24T11:58:12Z

src/js_types.hpp

@@ -126,6 +126,7 @@ struct Value {
    static bool is_boolean(ContextType, const ValueType &);
    static bool is_constructor(ContextType, const ValueType &);
    static bool is_date(ContextType, const ValueType &);
+    static bool is_error(ContextType, const ValueType &);


whenever we change the abstraction layer we need to handle it both for Nodejs and RN

blagoev · 2020-11-24T11:58:25Z

src/jsc/jsc_value.hpp

@@ -85,6 +85,12 @@ inline bool jsc::Value::is_constructor(JSContextRef ctx, const JSValueRef &value
    return JSValueIsObject(ctx, value) && JSObjectIsConstructor(ctx, (JSObjectRef)value);
 }

+template<>
+inline bool jsc::Value::is_error(JSContextRef ctx, const JSValueRef &value) {


whenever we change the abstraction layer we need to handle it both for Nodejs and RN

Thanks for catching this - I thought about it and forgot about it 🙂

blagoev · 2020-11-25T12:55:03Z

src/js_app.hpp

    std::string user_agent_binding_info;
    auto user_agent_function = js::Object<T>::get_property(ctx, realm_constructor, "_createUserAgentDescription");
    if (js::Value<T>::is_function(ctx, user_agent_function)) {
        auto result = js::Function<T>::call(ctx, js::Value<T>::to_function(ctx, user_agent_function), realm_constructor, 0, nullptr);
        user_agent_binding_info = js::Value<T>::validated_to_string(ctx, result);
    }
+    else {
+        // The `_createUserAgentDescription` method is undefined when chrome debugging a React Native app


_createUserAgentDescription should be called or some other mechanism introduced so we are able to get the result we are looking for. Seems like _createUserAgentDescription should be changed to support when running under chrome debugger which is what the PR is fixing.

blagoev · 2020-11-25T12:56:57Z

src/js_app.hpp

@@ -185,6 +200,12 @@ void AppClass<T>::constructor(ContextType ctx, ObjectType this_object, Arguments
        config.platform_version = js::Value<T>::validated_to_string(ctx, Object::get_property(ctx, result_object, platform_version_name));
        config.sdk_version = js::Value<T>::validated_to_string(ctx, Object::get_property(ctx, result_object, sdk_version_name));
    }
+    else {
+        // The `_createPlatformDescription` method is undefined when chrome debugging a React Native app


every method is undefined when chrome debugging a React Native app. we should make the changes needed to be able to call it. Or there are other options to get this information when running in Chrome debug context

src/rpc.cpp

lib/browser/rpc.js

blagoev · 2020-11-25T13:14:52Z

src/jsc/jsc_rpc_network_transport.hpp

+*/
+
+/**
+ * Provides an implementation of GenericNetworkTransport for use when the library is loaded in a runtime which doesn't provide the APIs required to perform network requests directly.


The way I see it is we have JavaScriptNetworkTransport which is a generic class to do network requests. It should be possible to use it instead of defining a new one for the debug context. The way it works is it just needs a global.Realm._networkTransport.fetchWithCallbacks in order to work.

…o work

blagoev

approving the changes since my comments are already worked on

cesarvr

LGTM

kraenhansen · 2020-12-02T17:54:16Z

I've added static members to the AppClass and exposed a _setVersions static method on it to set it from the outside as well as over the RPC bridge. I considered calling the set_versions method from rpc.cpp, but it turned out more complicated than setting the static members on the AppClass directly.

CHANGELOG.md

lib/browser/fetch.js

kraenhansen · 2020-12-03T13:33:05Z

I found (and solved a bug) where fetching would cause a "EXC_BAD_ACCESS" (the function got called in a released context).

kraenhansen · 2020-12-03T13:40:19Z

src/rpc.cpp

@@ -624,6 +624,9 @@ RPCServer::~RPCServer() {
    m_objects.clear();
    m_callbacks.clear();

+    // Clear the Object Store App cache, to prevent instances from using the context which is going to be released.
+    app::App::clear_cached_apps();


I am a bit unsure if this is the right place to call it.
I'm unaware of another place in our code that gets called when we're "unloading" our library, but we could clear the object store app cache when we initialize instead? https://github.com/realm/realm-js/blob/master/src/jsc/jsc_init.cpp#L42

@blagoev @kneth ☝️ what do you think about that?

I imagine the issue is when you reload the app, right? If so, I think it make sense to clear the cache as you shut down the RPC server.

Correct. If it's not cleared before / after a reload object store might be reusing app instances that reference a context which has been garbage collected.

Co-authored-by: Kenneth Geisshirt <kenneth.geisshirt@mongodb.com>

kneth · 2020-12-04T08:54:27Z

LGTM - merge at will!

fronck

In general: I think we should make an effort to document functions -- at least new ones -- with e.g., Doxygen-style headers to improve the general developer-friendlyness of the codebase.

Otherwise, good job; LGTM.

mendesbarreto · 2020-12-04T14:03:11Z

Thank you all for this work!!!

saivarunk · 2020-12-05T05:18:16Z

When can we expect this commit/version to be published on npm?

kraenhansen · 2020-12-07T08:10:30Z

@saivarunk it's out in v10.0.2.

kraenhansen mentioned this pull request Nov 18, 2020

[v10] RN debugger fixes #3368

Closed

8 tasks

kraenhansen self-assigned this Nov 18, 2020

This comment has been minimized.

Sign in to view

kraenhansen requested review from kneth and blagoev November 20, 2020 16:06

kraenhansen marked this pull request as ready for review November 20, 2020 16:08

kraenhansen requested a review from cesarvr November 20, 2020 16:08

kraenhansen force-pushed the kh/rn-create-session-continued branch 2 times, most recently from 0b755ce to d1fe510 Compare November 21, 2020 12:27

kneth reviewed Nov 23, 2020

View reviewed changes

This was referenced Nov 23, 2020

Realm Web: Export BSON types from the package #3242

Closed

Enabling React Native Debug mode causes Error: Must first create RPC session with a valid host from rpc.js:314 #3358

Closed

kraenhansen force-pushed the kh/rn-create-session-continued branch from b8f85c2 to 5e7a1da Compare November 24, 2020 11:15

kraenhansen requested a review from kneth November 24, 2020 11:19

blagoev suggested changes Nov 24, 2020

View reviewed changes

kneth approved these changes Nov 25, 2020

View reviewed changes

blagoev reviewed Nov 25, 2020

View reviewed changes

kneth and others added 7 commits November 30, 2020 13:17

wip

50f531c

Readd rpc.createSession()

d510aed

RN debugger can establish session. Initial attempt to get Realm.App t…

873ee20

…o work

wip

3439be1

WIP

cdc8e01

Reorganized the RPCNetworkTransport to be T independent

f63f58e

Adding jsc_rpc_network_transport.hpp to the XCode project

cda2deb

Incorporating feedback

c028109

kraenhansen force-pushed the kh/rn-create-session-continued branch from c82d5b5 to c028109 Compare November 30, 2020 12:17

blagoev approved these changes Nov 30, 2020

View reviewed changes

kneth mentioned this pull request Dec 1, 2020

Cannot read property 'debugHosts' of undefined #3433

Closed

cesarvr approved these changes Dec 1, 2020

View reviewed changes

Exposing App._setVersions and "set_versions" RPC route

1ea5409

kraenhansen requested review from blagoev, kneth and cesarvr December 2, 2020 17:50

kneth approved these changes Dec 3, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

lib/browser/fetch.js Outdated Show resolved Hide resolved

kraenhansen added 2 commits December 3, 2020 11:51

Using validated_to_function

226d3bc

Clearing the OS app cache on destruction

0f85f67

kraenhansen requested a review from fronck December 3, 2020 13:33

kraenhansen commented Dec 3, 2020

View reviewed changes

kraenhansen and others added 3 commits December 3, 2020 20:58

Add "emailPasswordAuth" property on App instances

9d2e701

Update lib/browser/fetch.js

91ba7a7

Co-authored-by: Kenneth Geisshirt <kenneth.geisshirt@mongodb.com>

Update CHANGELOG.md

20e5737

Co-authored-by: Kenneth Geisshirt <kenneth.geisshirt@mongodb.com>

cesarvr approved these changes Dec 4, 2020

View reviewed changes

kraenhansen merged commit 2177723 into master Dec 4, 2020

kraenhansen deleted the kh/rn-create-session-continued branch December 4, 2020 11:16

fronck reviewed Dec 4, 2020

View reviewed changes

This was referenced Dec 7, 2020

Enabling React Native Debug mode causes Error: _expandEmbeddedObjectSchemas must be of type 'function', got (undefined) #3442

Closed

Resolves ios app crash when reloading in debugging #3447

Closed

github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2024

	user_agent_binding_info = "N/A";
	user_agent_binding_info = "RN debugging";

Fixing chrome debugging on v10 #3411

Fixing chrome debugging on v10 #3411

Conversation

kraenhansen commented Nov 18, 2020 • edited Loading

What, How & Why?

☑️ ToDos

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

geragray commented Nov 19, 2020

kevinrodriguez-io commented Nov 20, 2020

Anzormumladze commented Nov 20, 2020

kneth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen Nov 23, 2020 • 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

kraenhansen Nov 24, 2020 • 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

kraenhansen Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen commented Nov 23, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen Nov 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen Nov 24, 2020 • 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

blagoev left a comment

Choose a reason for hiding this comment

cesarvr left a comment

Choose a reason for hiding this comment

kraenhansen commented Dec 2, 2020

kraenhansen commented Dec 3, 2020

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

kneth commented Dec 4, 2020

fronck left a comment

Choose a reason for hiding this comment

mendesbarreto commented Dec 4, 2020

saivarunk commented Dec 5, 2020

kraenhansen commented Dec 7, 2020

kraenhansen commented Nov 18, 2020 •

edited

Loading

kraenhansen Nov 23, 2020 •

edited

Loading

kraenhansen Nov 24, 2020 •

edited

Loading

kraenhansen Nov 23, 2020 •

edited

Loading

kraenhansen commented Nov 23, 2020 •

edited

Loading

kraenhansen Nov 25, 2020 •

edited

Loading

kraenhansen Nov 24, 2020 •

edited

Loading