Closed
Bug 1258925
Opened 8 years ago
Closed 8 years ago
[e10s] Browser window is resized when click
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: over68, Assigned: haik)
References
()
Details
(Keywords: regression, Whiteboard: btpp-active)
Attachments
(1 file, 5 obsolete files)
7.21 KB,
patch
|
Details | Diff | Splinter Review |
Steps to reproduce: 1. Go to https://dl.dropboxusercontent.com/u/95157096/85f61cf7/m6caw9mfvu.htm. 2. Click on "Visit page". Actual results: The browser window is resized when click on "Visit page".
Flags: needinfo?(alice0775)
Regression range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6446c26b45f9&tochange=93f526d85b47
Comment 2•8 years ago
|
||
Yes, it only occurs with e10s. Regressed by: 50981656f6b7 George Wright — Bug 1082127 - Implement TabChild::SetDimensions so that window.moveTo/resizeTo work with e10s r=smaug
Blocks: 1082127
Status: UNCONFIRMED → NEW
Component: General → DOM
Ever confirmed: true
Flags: needinfo?(gwright)
Flags: needinfo?(bugs)
Flags: needinfo?(alice0775)
Keywords: regression
Comment 3•8 years ago
|
||
testcase: data:text/html,<button onclick="window.open('data:text/html,<script>function run(){window.resizeBy(-300,-300)}</script><body onload=\'run()\'>')" >click to open a tab</button>
Updated•8 years ago
|
Assignee: nobody → haftandilian
Assignee | ||
Comment 4•8 years ago
|
||
Are we missing checks similar to what are found in nsGlobalWindow::CanMoveResizeWindows() [1]? If that's the case I think we need to add code to RecvSetDimensions to find the window for the tab and then check if it is one of the cases where a resize is allowed. i.e., the window has a single tab and it was created with window.open. From the old Firefox 7 release notes, "window.resizeTo, window.resizeBy, window.moveTo , and window.moveBy no longer apply to the main window." [2] And the Window.resizeBy() doc has "You can't resize a window or tab that wasn’t created by window.open. You can't resize a window or tab when it’s in a window with more than one tab." [3] [1] https://dxr.mozilla.org/mozilla-central/rev/d9f50aa0a1aaf90499b85c31e0f329b762e80fdd/dom/base/nsGlobalWindow.cpp#6426 [2] https://developer.mozilla.org/en-US/Firefox/Releases/7 [3] https://developer.mozilla.org/en-US/docs/Web/API/Window/resizeBy
Assignee | ||
Comment 5•8 years ago
|
||
I've done some manual testing and this patch prevents the resize for the two test cases provided in the bug from comment #1 and comment #3. I don't understand the hierarchy of window objects involved here, but looked at the nsGlobalWindow::CanMoveResizeWindows to get an idea for how to do this. I tried getting the nsGlobalWindow and re-using CanMoveResizeWindows but that did not work in part because its call to GetTargetableShellCount always returned 0. I unset the open the "open new windows in a tab" setting and used this code for extra testing. var foo = window.open('about:blank','_blank',''); foo.moveTo(0,0); foo.resizeBy(-500,-500) Testing the fix with unchecked "open new windows in a tab", I could continue to resize the new window from the original window's web console. If I added tabs to the new window, but didn't load any content in them, that didn't change the GetBrowserCount value and resizing still worked. Once I loaded content in those tabs, GetBrowserCount increased due to the stack below and resizing was prevented. XUL`nsWindowRoot::AddBrowser(mozilla::dom::TabParent*) XUL`mozilla::dom::TabParent::SetOwnerElement(mozilla::dom::Element*)+0x15d XUL`mozilla::dom::ContentParent::CreateBrowserOrApp(mozilla::dom::TabContext const&, mozilla::dom::Element*, mozilla::dom::ContentParent*)+0x80b ... With Firefox 45, adding extra tabs in the newly opened window didn't have to load content in order to block the resizing. This just applies to the case when "open new windows in a tab" was unchecked.
Attachment #8738800 -
Flags: feedback?(mconley)
Comment 6•8 years ago
|
||
Comment on attachment 8738800 [details] [diff] [review] Work in progress patch, just manual tests run so far Review of attachment 8738800 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/TabParent.cpp @@ +915,3 @@ > > bool > TabParent::RecvSetDimensions(const uint32_t& aFlags, My biggest problem with this patch is that nsGlobalWindow::CanMoveResizeWindows takes an aCallerIsChrome argument, that indicates if the operation was from privileged script or not, and transferring that to TabChild::SetDimensions that sends the SetDimension message can be really hard. The other approach is that I'm working on is sending up a sync message from the child from CanMoveResizeWindows, getting the tab count and make the decisions there. If we decide that we want to do this approach I can prepare a patch for the messaging part that you can continue. @@ +935,3 @@ > NS_ENSURE_TRUE(mFrameElement, true); > + curTopLevelWin = nsContentUtils::GetWindowRoot(mFrameElement->OwnerDoc()); > + size_t browserCount = curTopLevelWin->GetBrowserCount(); I'm not sure that we know from C++ the number of tabs if e10s is on... Which is quite sad and we should fix it. One possible way to do it can be: "felipe> gabor: if there's currently no easy way, I think you can add something to XULBrowserWindow which is a JS interface exposed to C++" I tried to add a counter for targetable tab parents but did not work out :( so I don't have any better ideas right now Problem is that this might be slow... and we need to get this info while we're blocking the content process (as I explained above)... it's not a super hot code but still I hope we can find something faster. Mike, do you have any better ideas? Or do you think this is acceptable?
Comment 8•8 years ago
|
||
Comment on attachment 8738800 [details] [diff] [review] Work in progress patch, just manual tests run so far Review of attachment 8738800 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/TabParent.cpp @@ +935,3 @@ > NS_ENSURE_TRUE(mFrameElement, true); > + curTopLevelWin = nsContentUtils::GetWindowRoot(mFrameElement->OwnerDoc()); > + size_t browserCount = curTopLevelWin->GetBrowserCount(); I think I like felipe's idea. Adding something to nsIXULBrowserWindow should be straight forward, and I agree not a hot-path by any means. Having the method in nsIXULBrowserWindow return gBrowser.tabs.length should be sufficient.
Attachment #8738800 -
Flags: feedback?(mconley)
Comment 9•8 years ago
|
||
Since I already have this part you could continue from here. And then in RecvGetTabCount you could do the nsIXULBrowserWindow bits. About the testing part I think we already have test coverage for all this just they are turned off for e10s. Since I'm trying to turn them back on in bug 1255138, you could finish up this patch, do the manual testing, then I can finish up my patches there re-based on your patch and then we could land / uplift them together. Unless you want to write more tests...
Updated•8 years ago
|
Flags: needinfo?(gwright)
Flags: needinfo?(bugs)
Updated•8 years ago
|
Whiteboard: btpp-active
Assignee | ||
Comment 10•8 years ago
|
||
Updated patch derived from gabor's GetTabCount WIP. The patch adds a new message GetTabCount letting the content process ask chrome for the number of tabs. It adds another IPC round trip to the resize code path. I thought it might be better to do this in just one message so that a compromised content process couldn't do resizes in disallowed situations, but we already rely on the content process to determine if the restrictions apply because they only to web content and the content process determines if the resize is from web content or not. If we wanted to eliminate the extra round trip, we could pass an additional flag to the existing RecvSetDimensions message indicating if the resize checks need to apply, but I thought it's better to keep all the security check logic in nsGlobalWindow::CanMoveResizeWindows.
Attachment #8738800 -
Attachment is obsolete: true
Attachment #8739381 -
Attachment is obsolete: true
Attachment #8740175 -
Flags: review?(gkrizsanits)
Comment 11•8 years ago
|
||
Comment on attachment 8740175 [details] [diff] [review] Using new RecvTabCount Message Review of attachment 8740175 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, thanks!
Attachment #8740175 -
Flags: review?(gkrizsanits) → review+
Comment 12•8 years ago
|
||
Comment on attachment 8740175 [details] [diff] [review] Using new RecvTabCount Message Review of attachment 8740175 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsGlobalWindow.cpp @@ +19,5 @@ > #include "nsIDOMStorageManager.h" > #include "mozilla/dom/DOMStorage.h" > #include "mozilla/dom/StorageEvent.h" > #include "mozilla/dom/StorageEventBinding.h" > +#include "mozilla/dom/TabChild.h" I pushed this patch to try with my patch on top of it and I think this include on windows includes windows.h somehow and that kills the build... https://treeherder.mozilla.org/#/jobs?repo=try&revision=598d086ff13c&selectedJob=19421051 @@ +6439,5 @@ > + if (XRE_IsContentProcess()) { > + nsCOMPtr<nsIDocShell> docShell = GetDocShell(); > + if (docShell) { > + nsCOMPtr<nsITabChild> child = docShell->GetTabChild(); > + static_cast<TabChild*>(child.get())->SendGetTabCount(&itemCount); Actually we should do a null check here... I thought instead of figuring out what causes the include error I mentioned above you should just add a GetTabChild function to nsITabChild and make that send the message. Then we can avoid the include and this ugly static_cast... Would you mind updating the patch?
Comment 13•8 years ago
|
||
Sorry for not noticing these things earlier, could you update the patch and flag for me again for a quick check? Or should I do it?
Flags: needinfo?(haftandilian)
Assignee | ||
Comment 14•8 years ago
|
||
Thanks for the tip and review. I ran into the Windows build issue too and have been trying to understand what the #include path is to windows.h. I'll try your suggestion and update the patch and then flag you again.
Flags: needinfo?(haftandilian)
Assignee | ||
Comment 15•8 years ago
|
||
Updated the patch. Created a new method in nsITabChild named SendGetTabCount and added NULL check for the docShell->GetTabChild() call in nsGlobalWindow::CanMoveResizeWindows. Was that what you had in mind, gabor?
Attachment #8740175 -
Attachment is obsolete: true
Attachment #8741268 -
Flags: review?(gkrizsanits)
Comment 16•8 years ago
|
||
Comment on attachment 8741268 [details] [diff] [review] Updated patch using new method in nsITabChild and added NULL check. Review of attachment 8741268 [details] [diff] [review]: ----------------------------------------------------------------- Yes r=me with the nits I've added. Did this fix the build errors on windows? ::: dom/base/nsGlobalWindow.cpp @@ +6444,5 @@ > + } > + } > + } else { > + nsCOMPtr<nsIDocShellTreeOwner> treeOwner = GetTreeOwner(); > + if (treeOwner) Actually you are right, for one liner if's unlike in JS/xpconnect in DOM code we want to use {} like you did above for |child|. Although this file is not terrible consistent about it... Could you add them here too? @@ +6447,5 @@ > + nsCOMPtr<nsIDocShellTreeOwner> treeOwner = GetTreeOwner(); > + if (treeOwner) > + treeOwner->GetTargetableShellCount(&itemCount); > + } > + if (itemCount > 1) And here. ::: dom/interfaces/base/nsITabChild.idl @@ +18,5 @@ > attribute nsIWebBrowserChrome3 webBrowserChrome; > > [notxpcom] void sendRequestFocus(in boolean canFocus); > > + [notxpcom] void sendGetTabCount(out uint32_t tabCount); If you add a new method you should change the iid of nsITabChild interface.
Attachment #8741268 -
Flags: review?(gkrizsanits) → review+
Assignee | ||
Comment 17•8 years ago
|
||
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #16) > Comment on attachment 8741268 [details] [diff] [review] > Updated patch using new method in nsITabChild and added NULL check. > > Review of attachment 8741268 [details] [diff] [review]: > ----------------------------------------------------------------- > > Yes r=me with the nits I've added. Did this fix the build errors on windows? Thanks and yes this fixed the build problem on windows. Try run here https://treeherder.mozilla.org/#/jobs?repo=try&revision=5225d02c860e > ::: dom/base/nsGlobalWindow.cpp > @@ +6444,5 @@ > > + } > > + } > > + } else { > > + nsCOMPtr<nsIDocShellTreeOwner> treeOwner = GetTreeOwner(); > > + if (treeOwner) > > Actually you are right, for one liner if's unlike in JS/xpconnect in DOM > code we want to use {} like you did above for |child|. Although this file is > not terrible consistent about it... Could you add them here too? > > @@ +6447,5 @@ > > + nsCOMPtr<nsIDocShellTreeOwner> treeOwner = GetTreeOwner(); > > + if (treeOwner) > > + treeOwner->GetTargetableShellCount(&itemCount); > > + } > > + if (itemCount > 1) > > And here. OK to both. > ::: dom/interfaces/base/nsITabChild.idl > @@ +18,5 @@ > > attribute nsIWebBrowserChrome3 webBrowserChrome; > > > > [notxpcom] void sendRequestFocus(in boolean canFocus); > > > > + [notxpcom] void sendGetTabCount(out uint32_t tabCount); > > If you add a new method you should change the iid of nsITabChild interface. There was a thread back in January about no longer needing to do this. See the "PSA: Please stop revving UUIDs when changing XPIDL interface" thread: https://groups.google.com/forum/#!topic/mozilla.dev.platform/HE1_qZhPj1I
Assignee | ||
Comment 18•8 years ago
|
||
Added the braces. Here's the try run. (Not re-running tests due to the only change being superficial.) https://treeherder.mozilla.org/#/jobs?repo=try&revision=5225d02c860e
Attachment #8741268 -
Attachment is obsolete: true
Assignee | ||
Comment 19•8 years ago
|
||
Forgot to add the r= before.
Attachment #8741505 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 20•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0416c442aab8
Keywords: checkin-needed
Comment 21•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0416c442aab8
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 22•8 years ago
|
||
I assume we're not going to uplift this fix to Beta 47, so I'm marking this bug status-firefox47=wontfix to remove this bug from our query of potential e10s uplifts.
status-firefox47:
--- → wontfix
Comment 23•8 years ago
|
||
The other bug will take some more time, could you please uplift this to aurora in the meanwhile?
Flags: needinfo?(haftandilian)
Assignee | ||
Comment 24•8 years ago
|
||
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #23) > The other bug will take some more time, could you please uplift this to > aurora in the meanwhile? This one is already in Aurora48 and 1266484 is targeting Aurora48. Did I misunderstand your comment?
Flags: needinfo?(haftandilian) → needinfo?(gkrizsanits)
Comment 25•8 years ago
|
||
(In reply to Haik Aftandilian [:haik] from comment #24) > (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #23) > > The other bug will take some more time, could you please uplift this to > > aurora in the meanwhile? > > This one is already in Aurora48 and 1266484 is targeting Aurora48. Did I > misunderstand your comment? Sorry for the big lag on this one, but I've been struggling to decide if this should be uplifted to beta or not, possibly combined with the related patches... but I think it's fine to leave it on 48.
Flags: needinfo?(gkrizsanits)
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•