1615418 - Glitchy disconnect behavior on about:debugging

Status: RESOLVED FIXED

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

Description

[Markus Stange [:mstange]]

Attachment: 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.

Comment 1

[:Harald Kirschner :digitarald]

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

Comment 2

[:Harald Kirschner :digitarald]

Attachment: browser-console.log

Comment 3

[:Harald Kirschner :digitarald]

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

Comment 4

[:Harald Kirschner :digitarald]

ni? for the aforementioned comment.

Comment 5

[:Harald Kirschner :digitarald]

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

Comment 6

[Julian Descottes [:jdescottes] (PTO 20 oct -> 25 oct)]

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.

Comment 7

[Julian Descottes [:jdescottes] (PTO 20 oct -> 25 oct)]

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.

Comment 8

[Julian Descottes [:jdescottes] (PTO 20 oct -> 25 oct)]

Attachment: Bug 1615418 - Remove FF69 backward compatibility code in devtools workers-listener

Comment 9

[Julian Descottes [:jdescottes] (PTO 20 oct -> 25 oct)]

Attachment: Bug 1615418 - Use a real client in disconnect aboutdebugging test

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

Comment 10

[Julian Descottes [:jdescottes] (PTO 20 oct -> 25 oct)]

Attachment: Bug 1615418 - Do not throw when calling watch/unwatchFront on destroyed Fronts

Depends on D62891

Comment 11

[Julian Descottes [:jdescottes] (PTO 20 oct -> 25 oct)]

Attachment: Bug 1615418 - Remove destroyed fronts from devtools workers-listener

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

Comment 12

[Julian Descottes [:jdescottes] (PTO 20 oct -> 25 oct)]

Attachment: Bug 1615418 - Do not throw when calling watch/unwatchFront on destroyed Fronts

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

Comment 13

[Julian Descottes [:jdescottes] (PTO 20 oct -> 25 oct)]

Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=df5056433e8b7634f7b51f7c6ea0649540e4f0f0

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/824d89212177
https://hg.mozilla.org/mozilla-central/rev/b06ca9139a18
https://hg.mozilla.org/mozilla-central/rev/a0cc5557495d
https://hg.mozilla.org/mozilla-central/rev/bcc19bc12369

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Julian Descottes [:jdescottes] (PTO 20 oct -> 25 oct)]

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.

Comment 18

[Pascal Chevrel:pascalc]

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?

Comment 19

[Julian Descottes [:jdescottes] (PTO 20 oct -> 25 oct)]

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.