1528276 - [remote-dbg-next] Device shows "waiting for runtime" even if a valid runtime is started

Status: RESOLVED FIXED

(DevTools :: about:debugging, defect, P1)

Description

[Julian Descottes [:jdescottes]]

Seems to be a regression when targeting Fx 66 or 67 from Fx 67. Fx 67 -> 65 seems to work fine on my machine.

STRs

• setup about:debugging-new to debug your phone
• connect your phone via USB
• on your phone, start Fx67 or 66
• in about:debugging, connect to the Runtime
• select the runtime to go to the dedicated Runtime Page
• quit Firefox
• re-open Firefox
• go to about:debugging-new

ER: The runtime should still be listed with a Connect Button
AR: The runtime shows "Waiting for runtime..."

Might be a server regression, but we should look for a client side regression window first.

Comment 1

[Julian Descottes [:jdescottes]]

Regression from Bug 1521052

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1528276 - Do not destroy the DebuggerServer in non-e10s when last frame connection is closed

When reviewing https://bugzilla.mozilla.org/show_bug.cgi?id=1521052 I did not think about Firefox for Android which is not using e10s.
This means the main DebuggerServer will be killed when there are no connections left. Happy to discuss more about the preferred solution.
This is a regression in 66 and I hope to uplift a fix for this.

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: Bug 1528276 - Add test for DebuggerServer.keepAlive;r=ochameau

Depends on D20830

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1528276 - Set DebuggerServer.keepAlive for RemoteDebugger and GeckoViewRemoteDebugger;r=ochameau

Depends on D20831

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c300840573f5
Do not destroy the DebuggerServer in non-e10s when last frame connection is closed r=ochameau

Comment 6

[Cristian Brindusan [:cbrindusan]]

Backed out changeset c300840573f5 (Bug 1528276) for dt failures at browser_target_server_compartment.js.

Backout: https://hg.mozilla.org/integration/autoland/rev/6e4eb864eeb65002d6e7f8632dc65b40be838859

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c300840573f5548de1b1aeb68c6e60ef1d1487bc&selectedJob=230569587

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=230569587&repo=autoland&lineNumber=7054

Comment 7

[Julian Descottes [:jdescottes]]

Sorry about that, two separate issues here:

1/ leak on DebuggerServer started in the Main process. we add a listener on "connectionchange" but never remove it. Easy to fix.

2/ timeout with browser_target_server_compartment.js.

The patch here is not really playing well with Bug 1515290. We explicitly create a DebuggerServer in the main process but in a separate compartment, in order to debug a chrome tab. I think this kind of server should still be destroyed when the last connection closes. So the question is, can we still detect the servers that should be closed without having a flag?

I think an alternate approach here would be to say "if the server was started by a frame script, it should be terminated when the last connection drops. Otherwise we should keep the server alive.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d4dd2195766e
Do not destroy the DebuggerServer in non-e10s when last frame connection is closed r=ochameau
https://hg.mozilla.org/integration/autoland/rev/64aa07723c84
Add test for DebuggerServer.keepAlive;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/de6dc58f6f51
Set DebuggerServer.keepAlive for RemoteDebugger and GeckoViewRemoteDebugger;r=ochameau

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/d4dd2195766e
https://hg.mozilla.org/mozilla-central/rev/64aa07723c84
https://hg.mozilla.org/mozilla-central/rev/de6dc58f6f51

Comment 10

[Julian Descottes [:jdescottes]]

Attachment: bug1528276-part1-beta.patch

Comment 11

[Julian Descottes [:jdescottes]]

Attachment: bug1528276-part2-beta.patch

Comment 12

[Julian Descottes [:jdescottes]]

Attachment: bug1528276-part3-beta.patch

Comment 13

[Julian Descottes [:jdescottes]]

Comment on attachment 9047356 [details] [diff] [review]
bug1528276-part1-beta.patch

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Bug 1521052
• User impact if declined: We also need to uplift part2 and part3 together with this patch.
  Users who want remote debug Firefox For Android will need to reopen Firefox after each debugging session, otherwise DevTools will not be able to connect again.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: This is a DevTools fix for Android. Most of the STRs are about setting up remote debugging

1 - Build

• build and install Firefox For Android with the patches attached here

2 - Setup Remote Debugging

• enable USB debugging on the Android device
• enable USB debugging on Firefox For Android
• start a Desktop Firefox (either 66 or 67, it doesn't matter if the Desktop Firefox has this fix or not)
• start WebIDE
• install ADB if needed
• connect the Android device to the computer via USB
  => You should see your device in WebIDE. Click refresh devices if you don't
  (https://developer.mozilla.org/en-US/docs/Tools/WebIDE/Troubleshooting for help)

3 - Verify the fix

• click on Firefox Beta for the Android Device in WebIDE
• (accept the prompt on the phone if any)
• click on "Disconnect" in WebIDE (right sidebar)
• refresh devices
  => Firefox Beta should still be in the list
• click on Firefox Beta again
  => You should successfully connect to Firefox Beta on Android again

Without the fix, after click on "Disconnect" you need to kill Firefox Beta on the device and start it again.

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple javascript fix, impacting only the DebuggerServer started for Android devices.
  And the behavior of those DebuggerServer instances simply goes back to what it was before Bug 1521052, so not an untested situation.
• String changes made/needed:

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

It is a little bit late in beta to uplift, but we can do it.
Oana or Sorina is this something you can verify before Monday so we can get this into the next Fennec build?

Comment 15

[Sorina Florean [:sflorean]]

Hello,
I was able to reproduce this issue on an affected Nightly build and I can confirm that is fixed on Nightly 67.0a1 from 01/03. Following the steps, Firefox Nightly was still in the list after I clicked on "disconnect" and I was able to connect again without closing and opening Nightly again.

Keeping the flag qe-verify+ to verify on Beta as well.

Comment 16

[Julian Descottes [:jdescottes]]

Attachment: bug1528276-part2-beta.v2.patch

(first version had linting issues)

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9047356 [details] [diff] [review]
bug1528276-part1-beta.patch

Fix for remote debugging regression in 66, reverting to previous behavior.
OK for beta 13 uplift.

Comment 18

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e089d9413974
https://hg.mozilla.org/releases/mozilla-beta/rev/a8cd0839de6b
https://hg.mozilla.org/releases/mozilla-beta/rev/f50d84563bc5