Closed
Bug 1262806
Opened 8 years ago
Closed 8 years ago
Close button doesn't work for the RDM tool if tab is detached from the current window
Categories
(DevTools :: Responsive Design Mode, defect, P1)
Tracking
(firefox45 unaffected, firefox46 unaffected, firefox47 unaffected, firefox48 affected, firefox52 verified)
Tracking | Status | |
---|---|---|
firefox45 | --- | unaffected |
firefox46 | --- | unaffected |
firefox47 | --- | unaffected |
firefox48 | --- | affected |
firefox52 | --- | verified |
People
(Reporter: mboldan, Assigned: zer0)
References
Details
(Whiteboard: [multiviewport] [reserve-rdm] [qe-rdm])
Attachments
(1 file)
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
|
jryans
:
review+
|
Details |
[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.
Updated•8 years ago
|
Whiteboard: [multiviewport] → [multiviewport] [triage]
Reporter | ||
Updated•8 years ago
|
Blocks: 1239464
Whiteboard: [multiviewport] [triage] → [multiviewport]
Reporter | ||
Updated•8 years ago
|
Whiteboard: [multiviewport] → [multiviewport] [triage]
Updated•8 years ago
|
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•8 years ago
|
Priority: P3 → P2
Updated•8 years ago
|
Priority: P2 → P1
Updated•8 years ago
|
Priority: P1 → P3
Assignee | ||
Comment 2•8 years 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: rdm-top
Updated•8 years ago
|
Assignee: nobody → zer0
Status: NEW → ASSIGNED
Iteration: --- → 52.2 - Oct 17
Priority: P3 → P1
Updated•8 years ago
|
Iteration: 52.2 - Oct 17 → 52.3 - Nov 7
Comment hidden (mozreview-request) |
Comment 4•8 years 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•8 years 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•8 years ago
|
||
Here the try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8676270af0bf&selectedJob=29848767
Keywords: checkin-needed
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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7bd25d3fff74
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Comment 10•8 years 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]
Comment 11•8 years ago
|
||
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
Flags: qe-verify+
Whiteboard: [multiviewport] [reserve-rdm] → [multiviewport] [reserve-rdm] [qe-rdm]
No longer blocks: rdm-top
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•