Closed Bug 1528276 Opened 7 months ago Closed 7 months ago

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

Categories

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

defect

Tracking

(firefox65 unaffected, firefox66+ fixed, firefox67 verified)

RESOLVED FIXED
Firefox 67
Tracking Status
firefox65 --- unaffected
firefox66 + fixed
firefox67 --- verified

People

(Reporter: jdescottes, Assigned: jdescottes)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [qa-triaged])

Attachments

(6 files, 1 obsolete file)

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.

Priority: -- → P2
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Priority: P2 → P1

Regression from Bug 1521052

Blocks: 1521052

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.

Attachment #9045953 - Attachment is obsolete: true
Attachment #9045955 - Attachment is obsolete: true
Attachment #9045951 - Attachment description: Bug 1528276 - Add a keepAlive flag on DebuggerServer;r=ochameau → Bug 1528276 - Do not destroy the DebuggerServer in non-e10s when last frame connection is closed
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c300840573f5
Do not destroy the DebuggerServer in non-e10s when last frame connection is closed r=ochameau

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.

Flags: needinfo?(jdescottes)
Attachment #9045955 - Attachment is obsolete: false
Attachment #9045953 - Attachment is obsolete: false
Blocks: 1531055
Pushed by jdescottes@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
Attached patch bug1528276-part2-beta.patch (obsolete) — Splinter Review

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:
Attachment #9047356 - Flags: approval-mozilla-beta?

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?

Flags: qe-verify+
Flags: needinfo?(sorina.florean)
Flags: needinfo?(oana.horvath)
Whiteboard: [qa-triaged]

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.

Flags: needinfo?(sorina.florean)
Flags: needinfo?(oana.horvath)

(first version had linting issues)

Attachment #9047357 - Attachment is obsolete: true
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.
Attachment #9047356 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9047718 - Flags: approval-mozilla-beta+
Attachment #9047359 - Flags: approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.