1262806 - Close button doesn't work for the RDM tool if tab is detached from the current window

Status: VERIFIED FIXED

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

Description

[Mihai Boldan, Desktop QA [:mboldan]]

[Note]:
- Logged based on Bug 1239464 Comment 48

[Affected versions]:
- Firefox 48.0a1 (2016-04-06)

[Affected platforms]:
- Windows 10 x86, Ubuntu 12.04 x 86 and Mac OS X 10.11

[Steps to reproduce]:
1. Launch Firefox.
2. From about:config, enable the devtools.responsive.html.enabled pref.
3. Open RDM.
4. Drag the tab in order to detached it from the current window.
5. Click 'X' button from the Responsive Design Mode tool. 

[Expected result]:
- RDM closes.

[Actual result]:
- Nothing happens.

[Regression range]:
- This is not a regression since this is a new implementation.

[Additional notes]:
- 'TypeError: tab.linkedBrowser is null| manager.js:215:9' error is thrown in the console.

Comment 1

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

The state has changed slightly now.  After moving the tab to a new window, RDM closes automatically.  You can open it again, which does work, but then it's locked on and can't be closed.

Comment 2

[Matteo Ferretti [:zer0] [:matteo]]

So, I dig a bit and I found that the issue is the following assumption:

    // If our tab is about to be closed, there's not enough time to exit
    // gracefully, but that shouldn't be a problem since the tab will go away.
    // So, skip any yielding when we're about to close the tab.

(See: https://dxr.mozilla.org/mozilla-central/source/devtools/client/responsive.html/manager.js#349-351)

Basically, when the user is dragging the tab out of the window, Firefox closes the tab, and reopen it in a new window, but the document is still the same because the docshell's swap. It means,`resposivedesign-child.js` is still active. That's why if we open again the RDM, it's not possible to close anymore, because internally the `resposivedesign-child.js` script aborts if we try to activate it twice.

A simple workaround is resolve the promise in that case. However, that means that if the user is not going to open the RDM again on that document, we'll have the `resposivedesign-child.js` still running.

I'm currently investigating on a way to remove such script, but so far no luck – anything I tried ends up to the original RDM issue, the new window opens a blank document instead of the previous one.

Comment 3

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: Bug 1262806 - Close button doesn't work for the RDM tool if tab is detached from the current window;

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

Comment 4

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

Comment on attachment 8803315 [details]
Bug 1262806 - Close button doesn't work for the RDM tool if tab is detached from the current window;

https://reviewboard.mozilla.org/r/87628/#review86660

Thanks for working on this, I know these cases get pretty tricky!

::: devtools/client/responsive.html/test/browser/browser_exit_button.js:25
(Diff revision 1)
> +
> +  // Detach the tab with RDM open.
> +  let newWindow = gBrowser.replaceTabWithWindow(tab);
> +
> +  // Waiting the tab is detached.
> +  yield Promise.all([

To avoid intermittents, let's listen for these _before_ detaching and save the `Promise.all` result in some variable.  Then `yield` on that after `replaceTabWithWindow`.

::: devtools/client/responsivedesign/responsivedesign-child.js:45
(Diff revision 1)
>  
>    function startResponsiveMode({data:data}) {
>      debug("START");
>      if (active) {
> -      debug("ALREADY STARTED, ABORT");
> +      debug("ALREADY STARTED");
> +      sendAsyncMessage("ResponsiveMode:Start:Done");

What about if we change `ResponsiveUI.destroy` to _always_ call `message.request(this.toolWindow, "stop-frame-script")`, but only _`yield`_ on it if `!isTabContentDestroying`?  Does that help to at least stop the frame script for this case (which seems better than leaving it alive)?

Either way, I think it makes sense to take the change you've made here.  If the script is already started, it should reply like you've done here.

Comment 5

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8803315 [details]
Bug 1262806 - Close button doesn't work for the RDM tool if tab is detached from the current window;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87628/diff/1-2/

Comment 6

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8803315 [details]
Bug 1262806 - Close button doesn't work for the RDM tool if tab is detached from the current window;

https://reviewboard.mozilla.org/r/87628/#review86660

> To avoid intermittents, let's listen for these _before_ detaching and save the `Promise.all` result in some variable.  Then `yield` on that after `replaceTabWithWindow`.

It shouldn't be needed, since we're in the same process – the other tests in our codebase do that too –; but I did the changes anyway; it doesn't hurt.

Comment 7

[Matteo Ferretti [:zer0] [:matteo]]

Here the try build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8676270af0bf&selectedJob=29848767

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7bd25d3fff74
Close button doesn't work for the RDM tool if tab is detached from the current window; r=jryans

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7bd25d3fff74

Comment 10

[Saheda Reza [:Antora]]

I have successfully reproduced this bug on firefox nightly according to(2016-04-07)

Fixing bug is verified on Latest Nightly--Build ID:(20161030030204),User Agent: Mozilla/5.0 (Windows NT 10.0; rv:52.0) Gecko/20100101 Firefox/52.0

it seems oke for latest nightly.

Tested OS-- Windows10 32bit

Comment 11

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this bug using Fx 48.0a1, build ID: 20160407030234, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 52.0a1, build ID: 20161110030211 on Windows 10 x64, Mac OS X 10.11 and Ubuntu 14.04 LTS.

Cheers!