1407830 - RDM breaks after opening/closing a few times

Status: VERIFIED FIXED

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

Description

[:Harald Kirschner :digitarald]

Reproduced on both Google Slides & Google Docs.

STR:
- Open/close RDM with shortcut a few times (tried 

AR:
- Tab content after a few refreshs the RDM viewport doesn't render.
- Then disabling RDM again removes the RDM but the whole page stays white and doesn't react to anything (shortcuts, pressing refresh button). Focusing the awesomebar and hitting enter doesn't do anything.
- RDM shortcut still works, switches back between RDM/No-RDM, always showing white content

Browser console:

TypeError: zoom is undefined[Learn More]  viewZoomOverlay.js:47:5
TypeError: win.gBrowser is undefined[Learn More]  ZoomUI.jsm:69:1
this.browser is null ext-tabs-base.js:298
TypeError: win.gBrowser is undefined[Learn More]  ZoomUI.jsm:69:1
TypeError: zoom is undefined[Learn More]  viewZoomOverlay.js:47:5
TypeError: win.gBrowser is undefined[Learn More]  ZoomUI.jsm:69:1
this.browser is null ext-tabs-base.js:298
TypeError: win.gBrowser is undefined[Learn More]  ZoomUI.jsm:69:1

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Thanks for filing!  Have you noticed this only on Windows, or other platforms too?

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Okay, I can reproduce on macOS as well.

It's not necessary to use a complex web app.  A static site like https://www.mozilla.org/en-US/ is fine as well.

The strange part is that (for both macOS and Windows), I can only reproduce this with official builds.  If I build locally (even from an artifact build), I _cannot_ reproduce.

Still not sure what's going on yet, but at least there's a configuration that reproduces for me.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1407830 - Add diagnostics to RDM swap.

Add some (disabled by default) logging to the RDM swap process to speed up
future investigations.

Review commit: https://reviewboard.mozilla.org/r/189384/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/189384/

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1407830 - Verify that RDM swaps take place.

It is possible for the frame loader swap within `gBrowser._swapBrowserDocShells`
to fail when various frame state is either not ready yet or doesn't match
between the two browsers you're trying to swap.  However, such errors are
currently caught and silenced in the browser, because they are apparently
expected in certain cases.  So, here we do our own check to verify that the swap
actually did in fact take place, making it much easier to track such errors when
they happen.

Review commit: https://reviewboard.mozilla.org/r/189386/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/189386/

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1407830 - Wait for browser during RDM startup.

Before the browser is mounted, listen for `remote-browser-shown` so that we know
when the browser is fully ready.  Without waiting for an event such as this, we
don't know whether all frame state for the browser is fully initialized (since
some happens async after the element is added), and swapping browsers can fail
if this state is not ready.

Review commit: https://reviewboard.mozilla.org/r/189388/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/189388/

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Okay, I think I've got a fix for this issue.

I believe the main difference between local vs. official builds is that I typically use `--enable-debug-js-modules` locally, which means the dev build of React (among other things).  This appears to be slow enough that it's quite hard to trigger the problem.  

Removing that setting makes it more likely with a local build.  (I have now triggered it locally on both Windows and macOS.)

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f85ed8c4ca7043017d1296bcc04a672ec89c49b5

Comment 8

[Alexandre Poirot [:ochameau]]

Comment on attachment 8918522 [details]
Bug 1407830 - Add diagnostics to RDM swap.

https://reviewboard.mozilla.org/r/189384/#review194780

Comment 9

[Alexandre Poirot [:ochameau]]

Comment on attachment 8918523 [details]
Bug 1407830 - Verify that RDM swaps take place.

https://reviewboard.mozilla.org/r/189386/#review194782

::: devtools/client/responsive.html/browser/swap.js:86
(Diff revision 1)
> +  // So, here we do our own check to verify that the swap actually did in fact take place,
> +  // making it much easier to track such errors when they happen.
> +  let swapBrowserDocShells = (ourTab, otherBrowser) => {
> +    // The verification step here assumes both browsers are remote.
> +    if (!ourTab.linkedBrowser.isRemoteBrowser || !otherBrowser.isRemoteBrowser) {
> +      throw new Error("Both browsers should be remote before swapping.");

I thought everyting had to be remote anyway?
Don't we check this before, or is it because it could be lazy?

::: devtools/client/responsive.html/browser/swap.js:92
(Diff revision 1)
> +    }
> +    let contentTabId = ourTab.linkedBrowser.frameLoader.tabParent.tabId;
> +    gBrowser._swapBrowserDocShells(ourTab, otherBrowser);
> +    if (otherBrowser.frameLoader.tabParent.tabId != contentTabId) {
> +      // Bug 1408602: Try to unwind to save tab content from being lost.
> +      throw new Error("Swapping tab content between browsers failed.");

Do you still have STR to make it crash after you apply your third patch?

Comment 10

[Alexandre Poirot [:ochameau]]

Comment on attachment 8918524 [details]
Bug 1407830 - Wait for browser during RDM startup.

https://reviewboard.mozilla.org/r/189388/#review194792

Crash tested and haven't seen anything wrong except this exception:
  TypeError: d.contentViewer is null  content.js:147:11
Which leads to broken state where you can no longer switch RDM off.
I reproduced this by toggling RDM during page reload. (or reloading while RDM while toggling, hard to say)
But I also reproduce without this patch.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8918523 [details]
Bug 1407830 - Verify that RDM swaps take place.

https://reviewboard.mozilla.org/r/189386/#review194782

> I thought everyting had to be remote anyway?
> Don't we check this before, or is it because it could be lazy?

Yes, RDM already requires remote content in general, and that is checked at an earlier step.  I just thought it was helpful to clearly assert that this function expects both browsers to be remote, since the swap process involves various browsers in remote and non-remote modes (the tool UI is loaded non-remote).  Non-remote browsers won't have the `tabParent`, so even though they can be swapped in general, this function as written won't verify the swap correctly in that case.

> Do you still have STR to make it crash after you apply your third patch?

No, the third patch covers the only the failure case I know of.  It seems like it would still be nice to unwind where possible, as a precaution against any new problems that might crop up, but not a requirement.

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8918524 [details]
Bug 1407830 - Wait for browser during RDM startup.

https://reviewboard.mozilla.org/r/189388/#review194792

I can't seem to reproduce that issue locally, but it does seem like something separate.  If you have a clear STR, please let me know / file a new bug!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8403c3cf71a
Add diagnostics to RDM swap. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/4bb7df3fad2d
Verify that RDM swaps take place. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/7c0cbd30ce36
Wait for browser during RDM startup. r=ochameau

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b8403c3cf71a
https://hg.mozilla.org/mozilla-central/rev/4bb7df3fad2d
https://hg.mozilla.org/mozilla-central/rev/7c0cbd30ce36

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:digitarald, can you please update to the latest Nightly and re-test to see if the issue is now resolved?  If possible, please use the same machine and configuration as you did when you first noticed the issue since it can be timing dependent.

Comment 17

[:Harald Kirschner :digitarald]

WFM, could not break it on the same machine.

Comment 18

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8918524 [details]
Bug 1407830 - Wait for browser during RDM startup.

Approval Request Comment
[Feature/Bug causing the regression]: Bug in new Responsive Design Mode, most likely since it's been around (52+)
[User impact if declined]: If declined, it's possible for a tab opened in RDM to get wedged in a broken state, causing the tab to become non-responsive.  User may have to close the browser window and start over.  There is no risk of crash, but work may be lost when a tab / window becomes non-responsive.
[Is this code covered by automated tests?]: Yes, this case now triggers an error will cause tests to fail.
[Has the fix been verified in Nightly?]: Yes, reporter verified and another reporter also verified in duplicate bug 1307711.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: All patches in this bug.
[Is the change risky?]: No
[Why is the change risky/not risky?]: I believe it's low risk because it only impacts tabs opened in RDM, and the change is well contained.  The change fixes a race condition by explicitly waiting for the RDM viewport to be ready.
[String changes made/needed]: None

Comment 19

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8918523 [details]
Bug 1407830 - Verify that RDM swaps take place.

Approval Request Comment

See comment 18.

Comment 20

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8918522 [details]
Bug 1407830 - Add diagnostics to RDM swap.

Approval Request Comment

See comment 18.

Comment 21

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8918522 [details]
Bug 1407830 - Add diagnostics to RDM swap.

Approval Request Comment

See comment 18.

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8918522 [details]
Bug 1407830 - Add diagnostics to RDM swap.

It's not a new regression, not a common scenario, has a workaround (not ideal but better than none). I'd prefer to let this fix ride the 58 train.