The default bug view has changed. See this FAQ.

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
2 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.

Updated

7 months ago
Priority: P3 → P2

Updated

6 months ago
Priority: P2 → P1

Updated

6 months ago
Priority: P1 → P3
(Assignee)

Comment 2

6 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

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

Updated

5 months ago
Iteration: 52.2 - Oct 17 → 52.3 - Nov 7
Comment hidden (mozreview-request)
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

5 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

5 months ago
Here the try build:

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

Comment 8

5 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

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7bd25d3fff74
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
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.