Last Comment Bug 1262806 - Close button doesn't work for the RDM tool if tab is detached from the current window
: Close button doesn't work for the RDM tool if tab is detached from the curren...
Status: VERIFIED FIXED
[multiviewport] [reserve-rdm] [qe-rdm]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Responsive Design Mode (show other bugs)
: 48 Branch
: All All
P1 normal (vote)
: Firefox 52
Assigned To: Matteo Ferretti [:zer0] [:matteo]
: Mihai Boldan, QA [:mboldan]
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
Depends on:
Blocks: 1237360 1239464
  Show dependency treegraph
 
Reported: 2016-04-07 05:40 PDT by Mihai Boldan, QA [:mboldan]
Modified: 2017-01-23 09:26 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard: [testday-20161028]
Iteration: 52.3 - Nov 14
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
unaffected
unaffected
affected
verified

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
Bug 1262806 - Close button doesn't work for the RDM tool if tab is detached from the current window; (58 bytes, text/x-review-board-request)
2016-10-21 05:05 PDT, Matteo Ferretti [:zer0] [:matteo]
jryans: review+
Details | Review

Description User image Mihai Boldan, QA [:mboldan] 2016-04-07 05:40:30 PDT
[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 User image J. Ryan Stinnett [:jryans] (use ni?) 2016-08-18 18:36:00 PDT
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 User image Matteo Ferretti [:zer0] [:matteo] 2016-10-04 11:25:07 PDT
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 User image Matteo Ferretti [:zer0] [:matteo] 2016-10-21 05:05:32 PDT Comment hidden (mozreview-request)
Comment 4 User image J. Ryan Stinnett [:jryans] (use ni?) 2016-10-21 09:53:41 PDT
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 User image Matteo Ferretti [:zer0] [:matteo] 2016-10-25 05:41:54 PDT Comment hidden (mozreview-request)
Comment 6 User image Matteo Ferretti [:zer0] [:matteo] 2016-10-25 05:44:00 PDT
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 User image Matteo Ferretti [:zer0] [:matteo] 2016-10-25 05:45:05 PDT
Here the try build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8676270af0bf&selectedJob=29848767
Comment 8 User image Pulsebot 2016-10-25 07:51:41 PDT
Pushed by cbook@mozilla.com:
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 User image Phil Ringnalda (:philor) 2016-10-25 21:58:58 PDT
https://hg.mozilla.org/mozilla-central/rev/7bd25d3fff74
Comment 10 User image Saheda Reza [:Antora] 2016-10-30 12:59:25 PDT
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 User image Cristian Comorasu [:CristiComo] 2016-11-11 04:04:57 PST
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!

Note You need to log in before you can comment on or make changes to this bug.