912898 - B2G: Don't kill adb (or lock the screen) if a debugger client is connected

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[Paul Rouget [:paul]]

Soon, we will be able to connect our tools to Firefox OS via adb.

Firefox OS kills adb on lock.

Can we not kill adb if a client is connected?

Comment 1

[Dave Hylands [:dhylands]]

So what we need to achieve this is to have the ability to query if a debug session is in progress. This only needs to be queryable from chrome.

I'm not familiar with the remote debugging code that runs on the server. Does anyone know where that can be found?

Comment 2

[Dave Hylands [:dhylands]]

I said server and meant phone (although I'm guessing that they're the same)

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

The server keeps a map of current connections in DebuggerServer._connections, so you can count the active connections with Object.keys(DebuggerServer._connections).length.

Comment 4

[[:fabrice] Fabrice Desré]

Attachment: patch v1

With this patch we don't shutdown adb anymore when a debugging session is ongoing. It's probably good enough for most cases, but I would be slightly happier is there is a way to be notified by the DebuggerServer when connections are opened and closed. Panos, is that possible?

Comment 5

[Dave Hylands [:dhylands]]

Comment on attachment 800401 [details] [diff] [review]
patch v1

Review of attachment 800401 [details] [diff] [review]:
-----------------------------------------------------------------

We should probably also disable the timer if a debug session is active. Something along the lines of:

if (enableAdb && !isDebugging)
  startTimer
else
  stopTimer

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 800401 [details] [diff] [review]
patch v1

Review of attachment 800401 [details] [diff] [review]:
-----------------------------------------------------------------

We do have a connection notification mechanism in the client side, but not in the server side:

http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/client/connection-manager.js#58

However we do send a DOM event when the server is shutting down:

http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webbrowser.js#36

Either of those approaches would work. Do we need this in this bug or as a followup?

Comment 7

[[:fabrice] Fabrice Desré]

Attachment: patch v2

Panos, I tried the window event, but it's firing before the DebuggerServer._connections list is updated. Instead I added a way to get notified of connections being opened or closed. Works fine for me, but let me know if you prefer that to be implemented differently.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 801052 [details] [diff] [review]
patch v2

Review of attachment 801052 [details] [diff] [review]:
-----------------------------------------------------------------

I would have preferred using EventEmitter to cope with multiple callbaccks, but I see that we haven't moved it to toolkit/ yet, so this should suffice for now.

::: toolkit/devtools/server/main.js
@@ +185,5 @@
> +
> +  _fireConnectionChange: function() {
> +    if (this.onConnectionChange &&
> +        typeof this.onConnectionChange === "function") {
> +      this.onConnectionChange(this);

Passing the whole DebuggerServer sounds too much. Why not pass a state parameter to _fireConnectionChange (e.g."opened", "closed") and let that propagate to the callback?

Comment 9

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/0b719ae93f4e

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0b719ae93f4e

Comment 11

[Paul Rouget [:paul]]

Dave, thanks a lot for you reactivity on this issue.