Skip to content

Added static method for removing objects previously added by .mapObject() #614

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

AlexTrotsenko
Copy link
Contributor

I am developing debugger for J2V8, which uses Stetho for communication over Chrome DevTools protocol. Besides implementing Debugger domain I need to add local v8 variables to Runtime by static .mapObject() in order them to be visible in the Chrome Debugger UI.

The problem, that @ChromeDevtoolsMethod .releaseObject() method is not called by Chrome DevTools and I want to remove objects stored in the session manually in order to avoid memory leaks and possible OOM errors. That's why I would like to have static method to remove them from session. Some work-around are possible already now, but it makes sense for me to have it because static counter-part for adding object is exists - .mapObject() .

@jasta jasta merged commit e7cd7b9 into facebook:master Oct 10, 2018
@jasta
Copy link
Contributor

jasta commented Oct 10, 2018

I'm fine to merge this and I think I agree with your motivation, but one thing to call out: this will be fragile and fairly hard to maintain as a real API into the future. I'm sympathetic to your use case and want to support you, but I'm not necessarily confident that directly reaching into the Runtime module is the best contract for us to maintain. Is your integration open sourced somewhere I can see so at least Stetho can track this as a major customer to support and make sure we don't accidentally break?

@AlexTrotsenko
Copy link
Contributor Author

AlexTrotsenko commented Feb 25, 2019

@jasta sorry for delay with the answer - I have missed your reply somehow.
The project is available on github: https://github.com/AlexTrotsenko/j2v8-debugger

The exact place where V8 variable is added to Runtime is following (in Kotlin).

Concerning you notes about regarding fragility of such API:

  • According to what I see in the doc of the Session you have faced with similar leaks issues in the past for UI elements.
  • If you have something better in mind for not using the remove() method - I would be happy to use it as well.
    E.g. I can imaging using concept of additional , but optional grouping of mapped objects- something like "sub-session" or "tag" etc.
    The resulting code might looks like following then:
  public static int mapObject(JsonRpcPeer peer, Object object, @Nullable SubSession subSession) {
    final Session session = getSession(peer);

    if (subSession != null) {
      subSession.registerOnSubSessionEnded(new OnEndedReceiver() {
        @Override
        public void onDisconnect() {
          session.removeSubSession(subSession);
        }
      });
    }

    return session.getObjects(subSession).putObject(object);
  }


  public static int mapObject(JsonRpcPeer peer, Object object) {
    return mapObject(peer, object, null);
  }

Then my client code might looks like following:

private class V8ToChromeDevToolsBreakHandler(private val currentPeerProvider: () -> JsonRpcPeer?) : BreakHandler {
   val subSession = SubSession()
   ...
    override fun onBreak(event: DebugHandler.DebugEvent?, state: ExecutionState?, eventData: EventData?, data: V8Object?) {
                        ... 
                       val storedVariablesId = Runtime.mapObject(currentPeerProvider(), knowVariables, subSession)
                        .... 
    }
    ....
    /**
     * Resumes V8 execution. Called from Stetho thread.
     */
    fun resume() {
        subSession.onSubSessionEnded()
        resumeWith(null)
    }
}

P.S. Thanks taking time and merging all my pull requests - I will integrate the latest version of Stetho into j2v8-debugger project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants