facebook / hermes

A JavaScript engine optimized for running React Native.
https://hermesengine.dev/
MIT License
9.83k stars 630 forks source link

Lack of support of some CDP API methods for debugging #209

Open RedMickey opened 4 years ago

RedMickey commented 4 years ago

Hello, we are React Native Tools team. Now are migrating our extension on use of js-debug's debugger. We faced several issues when we were trying to debug a Hermes app by means of js-debug.

1. Hermes engine doesn't support flat session API

js-debug is flat session API oriented extension. Flat sessions API allows to create a single connection for several debug sessions. So it isn't required to create a separate session for each debugged process. Here is the list of methods from Target domain that support flat sessions:

Under the hood js-debug uses Target.attachToBrowserTarget method to get debugged page. We tried to debug (using js-debug extension) a Hermes app as an application launched in Chrome, but that was unsuccessful attempt. We found out the reason is that a Hermes app doesn't return any target on Target.attachToBrowserTarget request, it returns just an empty response and this is the main problem. js-debug sends such message:

{"id":1,"method":"Target.attachToBrowserTarget","params":{}}

And a Hermes app replies the following:

{"result":{},"id":1}

So it seems that Hermes engine hasn't supported flat sessions API yet. According to Google documentation flat sessions API is going to be the default API and Target.attachToTarget (which is currently used) method will be deprecated.

2. Hermes engine doesn't return objects correctly in Runtime.evaluate method

After unsuccessful attempts to debug a Hermes app using Chrome scenarios, we tried to attach to it as a Node process(without flat session API support). In this case there are also some problems. js-debug uses Runtime.evaluate method and it expects a Hermes app to return some specific objects, but Hermes engine returns something like this:

{
  "result": {
    "result": {
      "objectId": "1",
      "description": "Object",
      "className": "Object",
      "type": "object"
    }
  },
  "id": 6
}

We found out that Runtime.evaluate method can only return values of simple types, e.g. number, but if we want to return some object (e.g. {test: 4}), Hermes will send us a reply similar to the one above.

3. Hermes engine doesn't supported several CDP methods used by js-debug

Hermes engine sends just empty responses on the following requests:

All method, besides Debugger.setPauseOnExceptions are absent in Hermes inspector message types list.

We attach logs of CDP communication.

Is it possible to implement missing API methods? Runtime.evaluate method's correct work is crucial for the debugger, so could you please take a look on it?

willholen commented 4 years ago

For # 2, the value returned is a Runtime.RemoteObject. It contains an object ID (here 1) that you can pass to Runtime.getProperties to iterate on properties.

If you are expecting to get the object and all its properties in JSON as part of the message, you can set the returnByValue flag (the object must be known to be JSON serializable).

(Hermes has no internal Object IDs, but generates ephemeral ones that are valid until the next Resume)

RedMickey commented 4 years ago

Hi @willholen and thanks for your response. With respect to usage of returnByValue flag, for example here js-debug uses returnByValue: true parameter when it calls Runtime.evaluate method. So, js-debug extension sends the following request:

{
  "id": 6,
  "method": "Runtime.evaluate",
  "params": {
    "contextId": 1,
    "returnByValue": true,
    "expression": "({ processId: process.pid, nodeVersion: process.version, architecture: process.arch })"
  }
}

And a Hermes app returns such message:

{
  "result": {
    "result": {
      "objectId": "1",
      "description": "Object",
      "className": "Object",
      "type": "object"
    }
  },
  "id": 6
}

It can be that returnByValue: true flag was ignored.

willholen commented 4 years ago

@RedMickey Oh, that's odd. Can you please verify that your version of the Hermes Inspector includes this commit?

https://github.com/facebook/react-native/commit/f1a9ca04f63e3649afb798934811667de3e01fb4

RedMickey commented 4 years ago

@willholen, we tried to test this fix on 0.63.0-rc.0 version, but we faced the problem that when 'js-debug' extension sends Debugger.enable request, a Hermes app doesn't replies on it and crashes (without any errors). That's all that we have in our logs:

Reply: debugger -> target -> {
  "id": 2,
  "method": "Debugger.enable",
  "params": {}
}

So, for now we cannot test how Runtime.evaluate method works.

RedMickey commented 4 years ago

@willholen, we had a discussion with js-debug team about Hermes debugging issues, and we found out that it seems Hermes handles Debugger.setBreakpoint request incorrectly: the debugger sends specific script Id when we are trying to set a breakpoint, but a Hermes app returns empty scriptId field in response. Here is what we found out during some investigations of Debugger.setBreakpoint method. Could you please look at that? Is this an expected behavior?

willholen commented 4 years ago

The expected behavior is for the scriptId to be set if the breakpoint is resolved. There's a unit test that verifies that this is the case.

How do I reproduce the problem? This is what I've tried:

git clone https://github.com/RedMickey/rn_demo_button
cd rn_demo_button
yarn install
chmod +x ./android/gradlew
react-native run-android

However, it fails with Task 'installDebug' not found in project ':app'.

RedMickey commented 4 years ago

@willholen, I tried to build and launch the demo app using this sequence and didn't face any problems.

The demo project is just a plain RN v0.62.2 project where we added a button to be able to set and track breakpoints on button click. If for some reasons this demo project cannot be launched, you can create new RN project and add a button to track stops on breakpoints more conveniently.

We faced the problem with Debugger.setBreakpoint method, when we were trying to set a breakpoint using js-debug extension for VS Code. VS Code Insiders has already built in js-debug extension. Here are the steps to try to debug a Hermes app using js-debug:

  1. Open RN project in VS Code Insiders

  2. Add the following debug configuration to launch.json file (Here is the guide of how to debug in VS Code)

    {
        // Use IntelliSense to learn about possible attributes.
        // Hover to view descriptions of existing attributes.
        // For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387
        "version": "0.2.0",
        "configurations": [
            {
                "type": "pwa-node",
                "request": "attach",
                "name": "Attach to program",
                "port": 8081,
                "trace": true
            }
        ]
    }
  3. Launch an Hermes app using npx react-native run-android

  4. Start debugging in VS Code using Attach to program debug scenario

  5. Set a breakpoint (we set a breakpoint in the click handler of AppTestButton)

According to logs, we got the following for Debugger.setBreakpoint:

{
  "id": 16,
  "method": "Debugger.setBreakpoint",
  "params": {
    "location": {
      "scriptId": "2",
      "lineNumber": 105706,
      "columnNumber": 0
    }
  }
}
{
  "result": {
    "actualLocation": {
      "lineNumber": 0,
      "scriptId": ""
    },
    "breakpointId": "4"
  },
  "id": 16
}

I attach full Chrome debug session logs here. You can also check js-debug extension logs. We've already added "trace": true parameter to debug configuration in launch.json file, so after the debugging was stopped, js-debug will display the path to the log file in DEBUG CONSOLE.

willholen commented 4 years ago

@willholen, we tried to test this fix on 0.63.0-rc.0 version, but we faced the problem that when 'js-debug' extension sends Debugger.enable request, a Hermes app doesn't replies on it and crashes (without any errors).

This turned out to be due to a bad build setting. The fix is currently on the master branch (https://github.com/facebook/react-native/commit/d06ee3d18973e74983590d2888552dbd57ceb951) and will probably make it into the next RC

RedMickey commented 4 years ago

@willholen, we found out the reason of incorrect handling of Debugger.setBreakpoint request.

{
  "id": 16,
  "method": "Debugger.setBreakpoint",
  "params": {
    "location": {
      "scriptId": "2",
      "lineNumber": 105706,
      "columnNumber": 0
    }
  }
}

A Hermes app returns empty scriptId field, since there is "columnNumber": 0 property in the request, but it seems that Hermes engine doesn't support inline breakpoints, so it handles that request incorrectly. So, we removed columnNumber field from the request and were able to set and reach a breakpoint. Do you have plans to implement support of inline breakpoints in Hermes engine?

willholen commented 4 years ago

It turns out that at some point we removed fuzzy matching of breakpoint line&column because they resolved incorrectly. This means that if column 0 is just indenting, there's no code generated for it, and the breakpoint fails to resolve. You then get the result you're seeing.

I agree that this is surprising behavior. I've been working on adding fuzzy matches back at least for columns, but I'm still working on verifying that it works correctly e.g. with minified source with very long lines, where only parts of the line has been compiled.

willholen commented 4 years ago

I've verified that change and it should be pushed out by the end of the day. Specifying a column of 0 will resolve to e.g. column 4, if there's four spaces of indenting. As for the others:

Debugger.setPauseOnExceptions

This call should be fully supported. It does not return data, so the expected result is {}.

Runtime.runIfWaitingForDebugger

This call is not yet supported, but it should be simple enough to add. Can you help me understand the nuances of what it means to be waiting for a debugger?

For example, if you are currently paused after hitting the pause button, is the VM waiting for a debugger, and should therefore resume execution when receiving this call? Is it the same if it's stopped on a breakpoint?

Or should it only continue if the original reason for being stopped is that the app was started in "wait for debugger" mode, or hit a debugger; statement when no debugger was attached?

Debugger.setInstrumentationBreakpoint

We actually already have a custom Hermes.setPauseOnLoad call for much the same thing, so this should also be fairly straight forward to add.

Can you please confirm that the expected behavior is a paused notification with reason: "other" and hitBreakpoints: ["1234"] (where 1234 was returned by setInstrumentationBreakpoint? Are you using both beforeScriptExecution and beforeScriptWithSourceMapExecution?

Debugger.setAsyncCallStackDepth

Hermes does not currently support the collection of async call stacks, so this will require a larger effort.

RedMickey commented 4 years ago

@willholen, thank you for the fix of column 0 handling. I also tried to debug a Hermes app version 0.63.0-rc.1. It was built without problems and now an application can return some data in value field on Runtime.evaluate request. Thanks.

From my understanding this method is used to resume execution, if a debugger hasn't been attached or a process was started in "wait for debugger" mode(for example --inspect-brk).

Js-debug uses only beforeScriptWithSourceMapExecution option. I checked how Debugger.setInstrumentationBreakpoint is used in js-debug and found out that debugged apps (I tried common React Native app, Express Node.js app and simple js script) return the following on this request:

[Reply From Target To Debugger] {
  "id": 9,
  "result": {
    "breakpointId": "8:beforeScriptWithSourceMapExecution"
  }
}

Further, after a few requests, the extension removes this breakpoint:

[Reply From Debugger To Target] {
  "id": 16,
  "method": "Debugger.removeBreakpoint",
  "params": {
    "breakpointId": "8:beforeScriptWithSourceMapExecution"
  }
}

And as seen from Debugger.paused message, hitBreakpoints field contains the breakpoint location, which was returned on Debugger.setBreakpoint request:

[Reply From Debugger To Target] {
  "id": 22,
  "method": "Debugger.setBreakpoint",
  "params": {
    "location": {
      "scriptId": "80",
      "lineNumber": 105706,
      "columnNumber": 0
    }
  }
}

[Reply From Target To Debugger] {
  "id": 20,
  "result": {
    "breakpointId": "4:105678:0:80",
    "actualLocation": {
      "scriptId": "80",
      "lineNumber": 105678,
      "columnNumber": 15
    }
  }
}

Debugger.paused response:

"hitBreakpoints": [
    "4:105706:0:80"
]

I attach logs of debugging common RN app.

willholen commented 4 years ago

Please try out Debugger.setInstrumentationBreakpoint in https://github.com/facebook/react-native/commit/bf837d6373f536839c9cb3c93c2c106f44a4abc8 and let me know if it needs any further tweaks.

Hermes does not currently fill in hitBreakpoints when it breaks on regular breakpoints, but I made sure to fill it in here so that you can differentiate.

RedMickey commented 4 years ago

@willholen, I tried to build and launch apps from these packages 0.0.0-e57a2d80a, 0.0.0-aece57be2, 0.0.0-f535c8b4b. I created RN projects using npx react-native init command, e. g.:

npx react-native init AwesomeProject --version 0.0.0-e57a2d80a

But application build process failed with the error app:compileDebugJavaWithJavac. It could be that I did something wrong, but I don't know what the problem is. I attach error logs.

willholen commented 4 years ago

Debugger.runIfWaitingForDebugger is implemented in https://github.com/facebook/react-native/commit/452cb9a78a02a7c61386f1ff0d6842c686d69929

@RedMickey React Native master is an especially fast moving target, and breaks like this is not uncommon. I would suggest following e.g. the 0.63-stable branch and cherry picking commits on top.

This requires some changes to the template project to use RN from source, but should be significantly more stable.