1867483 - Responsive design mode shortcut freezes upon repeated use

Status: VERIFIED FIXED

(DevTools :: Responsive Design Mode, defect, P2)

Description

[kjti0x6145e]

Steps to reproduce:

• Go to any website
• Repeatedly press ctrl+shift+m in quick succession
  (Alternatively, just hold down m while holding ctrl+shift, as that sends a lot of m keystrokes quite quickly)

Actual results:

The tab got stuck in responsive design mode and both the x button which should return it back to normal and the shortcut ctrl+shift+m stopped working for that tab

Expected results:

It should have continued switching between responsive design and normal mode, or if that is not possible, at least inform me of that

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'DevTools::Responsive Design Mode' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Julian Descottes [:jdescottes]]

Reproduced on windows, :ochameau will add a stacktrace

Comment 3

[Alexandre Poirot [:ochameau]]

Reproduced on Windows and got the following exception:

TypeError: can't access property "isDestroyed", this.responsiveFront is undefined
    destroy resource://devtools/client/responsive/ui.js:269
    closeIfNeeded resource://devtools/client/responsive/manager.js:195
    _toggleForTab resource://devtools/client/responsive/manager.js:102
    toggle resource://devtools/client/responsive/manager.js:95
    onKeyShortcut resource://devtools/client/framework/devtools-browser.js:298
    onKey resource:///modules/DevToolsStartup.sys.mjs:890
    xulKey resource:///modules/DevToolsStartup.sys.mjs:808

Comment 5

[Alexandre Poirot [:ochameau]]

Let me try to have a look as I'm able to reproduce.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1867483 - [devtools] Stop async redux/react code after RDM is closed.

Unmounting the top level React component will help avoid trigerring React updates
when async thunk actions keep dispatching new actions after the tool is closed.
This was leading to various exceptions in many React components.

Also avoid a race condition where responsiveFront could be undefined when closing
RDM while it is still initializing.

Finally, ensure registering the ResponsiveUI immediately in activeTabs to prevent
race condition in openIfNeeded.

Adding some test coverage for such race condition, but can't enable this on verify.
In chaos mode, there is too many exceptions at the test teardown.
Ideally, RDM should:

• only close the RDP client and let the server restore the default value on its own.
  i.e. we shouldn't do any request on closing.
• RDM close requests should wait for previous initialization request to be done.
  This would avoid zillion of possible exceptions because initialization is very async
  and each step may throw because of things are already being destroyed.
  But this wouldn't address race condition when the tab is closed.
  So RDM should probably try to have a more resilient initialization sequence as well.

Comment 7

[Julian Descottes [:jdescottes]]

Can you check if the patch is ready for review?

Comment 9

[Alexandre Poirot [:ochameau]]

I'm struggling with the mochitest, still failing on debug builds.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e8522cd39ca8
[devtools] Stop async redux/react code after RDM is closed. r=devtools-reviewers,nchevobbe

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/e8522cd39ca8

Comment 12

[Giorgia Nichita, Release Desktop QA]

Reproduced this issue on an affected Nightly build from 2023-11-30, on Win 10.
Verified as fixed on Firefox 123.0b2 (20240124102023) across the following platforms: Win 10, Win 11, Ubuntu 22 and macOS 10.15. The tab does not get stuck anymore in responsive design mode when using Ctrl+Shift+M (or Cmd+Opt+M on Mac) in quick succession.

Comment 13

[kjti0x6145e]

Hi, I'd just like to thank all of you for fixing the problem this fast. I expected this bug to sit in limbo for half a year at the very least. Thank you and great job! :D