Closed Bug 1258925 Opened 8 years ago Closed 8 years ago

[e10s] Browser window is resized when click

Categories

(Core :: DOM: Core & HTML, defect)

48 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s m9+ ---
firefox47 --- wontfix
firefox48 --- fixed

People

(Reporter: over68, Assigned: haik)

References

()

Details

(Keywords: regression, Whiteboard: btpp-active)

Attachments

(1 file, 5 obsolete files)

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)
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
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>
Assignee: nobody → haftandilian
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
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 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 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)
Attached patch GetTabCount WIP (obsolete) — Splinter Review
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...
Flags: needinfo?(gwright)
Flags: needinfo?(bugs)
Whiteboard: btpp-active
Attached patch Using new RecvTabCount Message (obsolete) — Splinter Review
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 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 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?
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)
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)
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 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+
(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
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
Attached patch Adding r=Splinter Review
Forgot to add the r= before.
Attachment #8741505 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0416c442aab8
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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.
Depends on: 1266484
The other bug will take some more time, could you please uplift this to aurora in the meanwhile?
Flags: needinfo?(haftandilian)
(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)
(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)
Depends on: 1350642
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.