fix(inspector): Fix app crashes on second debugger attach by tdermendjiev · Pull Request #835 · NativeScript/ios-jsc

tdermendjiev · 2017-12-11T14:11:27Z

Introduction

We identified a couple of issues with the way connections are handled between the Inspector and the backend. On one side, the sockets were not handled correctly using the Dispatch API resulting in faulty connections upon subsequent connection attempts.
On the other side, there was an issue with the way Backend Dispatchers were handled via RefPtrs. For example, the DomainBackendDispatcher adopts a pointer to the dispatcher created by the DomainInspectorAgent, which also has a pointer to it. Adopting this pointer does not increase the refcount which in turn breaks the underlying object's (RefCountedBase) state.
The third amendment is in the way backend dispatchers are handled by the Agents in the didCreateFrontendAndBackend and willDestroyFrontendAndBackend calls. We now create a single instance of each dispatcher and leave it alive until the owning Agent is destroyed. This is the behaviour as seen in the vanilla WebKit source.

Fixes #832

After the debugger frontend is detached certain RefPtr properties are cleaned up and recreated on a subsequent connection which is causing a crash due to improper reference management. Making the m_backendDispatcher property persist between different connections fixes the bug.
Every time the debugger is detached the IO channel should be closed in order the next connection to be successful. Otherwise the connection attempt will fail with a bad descriptor error.
153d45f addresses an issue with debugger delay killing the already existing listen connection at an inappropriate time. We do not need this delay to cancel the listen connection as we immediately close it after a successful frontend connection.

ns-bot · 2017-12-11T14:11:29Z

Can one of the admins verify this patch?

mbektchiev

Great job!

ginev · 2017-12-18T08:50:53Z

@KristinaKoeva - could you please take a look at this PR and comment on the following questions:

why do we recreate the backend dispatchers when didCreateFrontendAndBackend is called and not use the pattern as seen in the WebKit's original source as pointed in Introduction section of the first comment?
regarding the handling of connection sockets - can you update us on why there is a 30s delay which closes the listening socket if no inspector is present?

After the debugger frontend is detached certain RefPtr properties are cleaned up and recreated on a subsequent connection which is causing a crash due to improper reference management. Making the *m_backendDispatcher* property persist between different connections fixes the bug.

… session Every time the debugger is detached the IO channel should be closed in order the next connection to be successful. Otherwise the connection attempt will fail with a bad descriptor error.

KristinaKoeva · 2017-12-19T20:51:01Z

The original webkit source was not the same at the time we wrote this logic :)
Take a look here - WebKit/WebKit@d81c28d

The delay is there because otherwise you will have an open socket for as long as the application lives.

Did the webkit update broke the second attach attempt ?

ginev · 2017-12-20T09:02:27Z

@KristinaKoeva - thanks. This is helpful. The issue with the RefPtr objects comes because of the fact that we recreate the DomainBackendDispatcher each time. It registers with the main Backend Dispatcher's HashMap of dispatchers and chains its supplemental dispatchers. The first iteration of this is OK but the second one causes us to find an invalid RefPtr from the HashMap and chain it to our DomainBackendDispatcher here: https://github.com/NativeScript/ios-runtime/blob/master/src/NativeScript/inspector/DomainBackendDispatcher.cpp#L31-L33

Refactor connection and disconnection logic: * Separate error and connection handlers * Separate listen socket and data socket handling and disconnection * Add method for checking whether there's an active frontend connection * Reject any new connections when there's an active one

The code was deleted in one of the previous refactorings

* VM locks in TNSRuntime+Inspector * Move catch scope declarations in locked sections * Synchronize dispatch IO channel and global inspector variable access across worker threads - all callbacks may be called in an arbitrary thread and we need to make sure that they don't run simultaneously. * Guard against deadlocks by always releasing the inspector lock before dealing with JSC VM

mbektchiev · 2017-12-22T15:36:56Z

-      if (inspector) {
-          return nil;
+      // Keep a working copy for calling into the VM after releasing inspectorLock
+      TNSRuntimeInspector* tempInspector = nil;


Move declaration to assignment line.

mbektchiev · 2017-12-22T15:37:53Z

+          //
+          //                                      [inspector pause];
+          //                                  });
+          //            CFRunLoopWakeUp(runloop);


Delete commented block.

mbektchiev · 2017-12-22T15:40:18Z

+                      tempInspector = inspector;
+                  }
+
+                  [tempInspector dispatchMessage:message];


Check whether inspector is null

mbektchiev · 2017-12-22T15:45:18Z

run ci

…ous ones This is consistent with the behavior of `AttachRequest` notification and it is more natural for the last connection attempt to succeed instead of fail due to an already existing one.

mbektchiev · 2017-12-28T09:42:01Z

run ci (after git clean -fdx in workspace)

…side of the VM lock Native call is made outside of the VM lock by design. For more information see #215 and it's corresponding PR. This creates a racing condition which might corrupt the internal state of the VM but a fix for it is outside of this PR's scope, so I'm leaving it as it has always been till now. We could address the issue by reimplementing the feature with a new VM or a JSWorker.

tdermendjiev · 2017-12-29T12:04:42Z

    self->_inspectorController->pause();
 }

+- (bool)hasFrontends {


We might delete this method as it's never called.

* Remove unused `hasFrontends` method * Delete forgotten clearing of `m_frontendDispatcher` * Remove commented code for delegating `pause` to the main loop

ginev · 2018-01-02T10:11:40Z

It might be good to rename this to be receiver as it rights to the io channel and is analogous to the sender block here.

ns-bot · 2018-01-02T10:11:48Z

Can one of the admins verify this patch?

mbektchiev · 2018-01-03T07:06:45Z

run ci

mbektchiev

From @tdermendjiev

tdermendjiev requested review from PanayotCankov, ginev and mbektchiev December 11, 2017 14:19

tdermendjiev self-assigned this Dec 11, 2017

tdermendjiev added the bug label Dec 11, 2017

ginev force-pushed the tdermendzhiev/fix-crash-on-debugging branch 2 times, most recently from b626581 to 153d45f Compare December 12, 2017 08:43

mbektchiev approved these changes Dec 13, 2017

View reviewed changes

mbektchiev added this to the 3.4.1 milestone Dec 13, 2017

tdermendjiev and others added 4 commits December 19, 2017 18:15

fix(inspector): Fix connection failure following successful debugging…

278a113

… session Every time the debugger is detached the IO channel should be closed in order the next connection to be successful. Otherwise the connection attempt will fail with a bad descriptor error.

fix(inspector): Fix issue with debug connection delay

c53b024

fixup! fix(inspector): Fix app crashes on second debugger attach

429ef7f

mbektchiev force-pushed the tdermendzhiev/fix-crash-on-debugging branch from 153d45f to 429ef7f Compare December 19, 2017 16:26

ginev changed the base branch from master to release December 20, 2017 08:54

mbektchiev added 4 commits December 20, 2017 17:23

chore: Apply clang-format to TNSDebugging.h

5632d9b

fix(Inspector): Stop waiting for debugger attach after 30 seconds

9cefe1e

The code was deleted in one of the previous refactorings

mbektchiev force-pushed the tdermendzhiev/fix-crash-on-debugging branch from 69066e4 to 64a8a9b Compare December 22, 2017 14:50

mbektchiev reviewed Dec 22, 2017

View reviewed changes

fix(Inspector): Make new frontend connections win over existing previ…

5a839fe

…ous ones This is consistent with the behavior of `AttachRequest` notification and it is more natural for the last connection attempt to succeed instead of fail due to an already existing one.

tdermendjiev commented Dec 29, 2017

View reviewed changes

Address PR Comments:

c0fa133

* Remove unused `hasFrontends` method * Delete forgotten clearing of `m_frontendDispatcher` * Remove commented code for delegating `pause` to the main loop

ginev closed this Jan 2, 2018

ginev reopened this Jan 2, 2018

mbektchiev force-pushed the tdermendzhiev/fix-crash-on-debugging branch from d186dc7 to bb70e82 Compare January 3, 2018 08:27

Rename ioHandler variable to receiver

c59d709

mbektchiev force-pushed the tdermendzhiev/fix-crash-on-debugging branch from bb70e82 to c59d709 Compare January 3, 2018 09:02

mbektchiev approved these changes Jan 3, 2018

View reviewed changes

ginev approved these changes Jan 3, 2018

View reviewed changes

mbektchiev merged commit 906abd3 into release Jan 3, 2018

mbektchiev deleted the tdermendzhiev/fix-crash-on-debugging branch January 3, 2018 09:16

Uh oh!

Conversation

tdermendjiev commented Dec 11, 2017 • edited by ginev Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Introduction

Uh oh!

ns-bot commented Dec 11, 2017

Uh oh!

mbektchiev left a comment

Choose a reason for hiding this comment

Uh oh!

ginev commented Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KristinaKoeva commented Dec 19, 2017

Uh oh!

ginev commented Dec 20, 2017

Uh oh!

mbektchiev Dec 22, 2017

Choose a reason for hiding this comment

Uh oh!

mbektchiev Dec 22, 2017

Choose a reason for hiding this comment

Uh oh!

mbektchiev Dec 22, 2017

Choose a reason for hiding this comment

Uh oh!

mbektchiev commented Dec 22, 2017

Uh oh!

mbektchiev commented Dec 28, 2017

Uh oh!

tdermendjiev Dec 29, 2017

Choose a reason for hiding this comment

Uh oh!

ginev commented Jan 2, 2018

Uh oh!

ns-bot commented Jan 2, 2018

Uh oh!

mbektchiev commented Jan 3, 2018

Uh oh!

mbektchiev left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tdermendjiev commented Dec 11, 2017 •

edited by ginev

Loading

ginev commented Dec 18, 2017 •

edited

Loading