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

VERIFIED FIXED in Firefox 52

Status

()

Firefox
Developer Tools: Responsive Design Mode
P1
normal
VERIFIED FIXED
a year ago
7 months ago

People

(Reporter: mboldan, Assigned: zer0)

Tracking

48 Branch
Firefox 52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 unaffected, firefox46 unaffected, firefox47 unaffected, firefox48 affected, firefox52 verified)

Details

(Whiteboard: [multiviewport] [reserve-rdm] [qe-rdm])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
[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.
Whiteboard: [multiviewport] → [multiviewport] [triage]
(Reporter)

Updated

a year ago
Blocks: 1239464
Whiteboard: [multiviewport] [triage] → [multiviewport]
(Reporter)

Updated

a year ago
Whiteboard: [multiviewport] → [multiviewport] [triage]
Flags: qe-verify+
Priority: -- → P3
Whiteboard: [multiviewport] [triage] → [multiviewport] [reserve-rdm]
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.
Priority: P3 → P2

Updated

11 months ago
Priority: P2 → P1

Updated

11 months ago
Priority: P1 → P3
(Assignee)

Comment 2

11 months ago
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.
Blocks: 1308297

Updated

11 months ago
Assignee: nobody → zer0
Status: NEW → ASSIGNED
Iteration: --- → 52.2 - Oct 17
Priority: P3 → P1

Updated

10 months ago
Iteration: 52.2 - Oct 17 → 52.3 - Nov 7
Comment hidden (mozreview-request)

Comment 4

10 months ago
mozreview-review
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.
Attachment #8803315 - Flags: review?(jryans) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 6

10 months ago
mozreview-review-reply
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.
(Assignee)

Comment 7

10 months ago
Here the try build:

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

Comment 8

10 months ago
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
Keywords: checkin-needed

Comment 9

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7bd25d3fff74
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52

Comment 10

10 months ago
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
QA Whiteboard: [testday-20161028]
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!
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
Flags: qe-verify+
Whiteboard: [multiviewport] [reserve-rdm] → [multiviewport] [reserve-rdm] [qe-rdm]
No longer blocks: 1308297
You need to log in before you can comment on or make changes to this bug.