Closed Bug 1615418 Opened 1 year ago Closed 1 year ago

Glitchy disconnect behavior on about:debugging

Categories

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

defect

Tracking

(firefox-esr68 unaffected, firefox73 wontfix, firefox74 wontfix, firefox75 fixed)

RESOLVED FIXED
Firefox 75
Tracking Status
firefox-esr68 --- unaffected
firefox73 --- wontfix
firefox74 --- wontfix
firefox75 --- fixed

People

(Reporter: mstange, Assigned: jdescottes)

References

(Regression)

Details

(Keywords: regression)

Attachments

(6 files, 1 obsolete file)

Attached video screen recording

Reproduced with Firefox 75.0a1 20200213035745

See the attached video. Disconnecting from a USB device on about:debugging shows random and inconsistent behavior:

  • Sometimes, after disconnecting, the "This Nightly" pane is selected.
  • Other times, after disconnecting, the pane on the right becomes empty and nothing is selected. In that state, clicking "This Nightly" doesn't do anything. And clicking Connect on the USB device immediately selects the device pane, but doesn't show the disconnect button. Selecting the device pane again adds the missing buttons.

There is hopefully something in Browser Console that points to a frontend error.

Priority: -- → P2

Julian, I have not seen this before in about:debugging. Might recent protocol work have regressed this – or did people just not click Disconnect?

ni? for the aforementioned comment.

Flags: needinfo?(jdescottes)

The same issue happens when the device gets disconnected from USB. Reconnecting fixes it again.

Seems to be a regression from Firefox 73. We do have a mochitest for this, but sadly it doesn't reproduce this issue because it relies on mocks.

Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Flags: needinfo?(jdescottes)

We have a shared helper to watch for worker updates.
Bug 1604142 started to properly call unwatchFront on all monitored targets (or rather target descriptors). But unwatchFront will throw if the corresponding descriptor was already destroyed.

Regressed by: 1604142

Depends on D62890
Using a real local client allows to cover more codepaths than using a complete mock here.

Attachment #9126698 - Attachment is obsolete: true

Depends on D62891

This will avoid part of the exceptions thrown when disconnecting a remote runtime.
However the rootFront unwatchFront calls will still throw because the root front is already gone at this point

Depends on D62893

The issue here is that we are trying to destroy the workers-listener after the target was destroyed,
and calling unwatchFront on a destroyed Front throws.

Most of the fronts monitored in workers-listener are handled by the watchFront API, so properly adding
onDestroyed handlers fixes some issues. However the rootFront cannot be handled with a similar pattern
at the moment.

In general, I think making watchFront/unwatchFront safer to call makes sense, but I could also check
if the rootFront is already destroyed in workers-listener's destroy

Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/824d89212177
Remove FF69 backward compatibility code in devtools workers-listener r=daisuke
https://hg.mozilla.org/integration/autoland/rev/b06ca9139a18
Remove destroyed fronts from devtools workers-listener r=daisuke
https://hg.mozilla.org/integration/autoland/rev/a0cc5557495d
Do not throw when calling watch/unwatchFront on destroyed Fronts r=daisuke,ochameau
https://hg.mozilla.org/integration/autoland/rev/bcc19bc12369
Use a real client in disconnect aboutdebugging test r=daisuke

Is this a common enough workflow that we should consider uplifting to Beta or can this ride 75 to release?

Flags: needinfo?(jdescottes)

Thanks for the ping! I am keeping the needinfo for now, I would like this to bake for a few more days on Nightly, especially to see if the dead code removed in the first patch doesn't trigger any unexpected issue.

Julian, given that we are nearing the end of the beta cycle for 74 do you intend to request an uplift to beta or can we just let it go with 75?

Thought about it some more, and I think we should ride the trains for this. I would feel uneasy uplifting some of the non-backward compatible changes so close to the end of the cycle.

Thanks.

Flags: needinfo?(jdescottes)
You need to log in before you can comment on or make changes to this bug.