I'm still fairly new to the implementation details of MUS so there might be something I am not fully understanding.

> "In mus either the parent or the child could be responsible for controlling the visibility, we just need to make sure both aren't (otherwise it'll be racy)."

This is the visibility of the child's window? Would it be possible to let both have control, and use a logical AND of the values provided? That is basically the way it works now, because when RenderWidgetHostViewChildFrame::WasShown is called (for e.g., from the tab activating into the foreground), it first checks if the parent renderer allows it to be visible, in case the iframe is current hidden, in the parent frame.

Possibly that could solve the problem of the main frame forcing itself visible when it shouldn't be.

That's in the browser though, I would like to control the visibility from the renderer. Is that acceptable?

I don't have a problem with that. In general I agree that parent windows should control the visibility of child windows.

Do we route visibility changes to the parent now? Correct me if I'm wrong, but it seems as though visibility changes are sent by way of ViewMsg_WasShown/WasHidden, which go to the real renderer, which is the child in this case.

+ekaramad who has worked on some OOPIF visibility issues in r495861 and r491066.  sky@: those CLs have some pointers for how OOPIF visibility changes are propagated.

#6: That is correct. The parent does have some control over the visibility of its child frame (FrameHostMsg_VisibilityChanged), but when the browser sends ViewMsg_WasShown/WasHidden to all RenderWidgets, the embedder doesn't explicitly know that the child is receiving it.

I still don't entirely understand the problem here. All frames, whether main or OOPIFs, need to know if they are visible or not, but don't necessarily need to control their own visibility wrt compositing (currently they do, because RenderWidget owns the compositor, but in MUS-world that won't be true). So it seems right for Window tree parents to control visibility of their children, and presumably (?) when a user changes active tabs, the browser can tell the main frame's parent to hide or show the main frame for each WebContents as appropriate. I wouldn't expect the main frame to be able to override that, just as OOPIFs shouldn't be able to override their parent frame telling them that the iframe element has visibility:hidden.

The underlying problem is each frame in mus has a Window associated with it. At some point the visibility of that window will impact which frame gets events, so it's important the visibility is correct. The current notification of visibility changes is sent to the (child) renderer rendering into the frame/window (ViewMsg_WasShown/WasHidden). I could make the child responsible for updating the visibility of the window, but shouldn't the parent really be the one controlling this? Additionally having the child update the visibility makes for raciness with the browser, who also wants to control the visibility of the main frame. AFAICT we don't send notifications of child visibility to the parent (I haven't fully investigated the patches mentioned in comment #7, so maybe I'm missing something).

Feel free to ping me if this still doesn't make sense. I'm investigating a couple of other mus related OOPIF issues and will return to this once they are straightened out.

This is my understanding:

 1 Let's say the browser is A, the renderer for the main-page is B, and the renderer for the OOPIF is C.
 2 When an OOPIF is hidden from the main page (e.g. display:none set on the iframe), this notification goes from B, through A, to C.
   2.a In non-mus case, this happens by first B sending FrameHostMsg_VisibilityChanged to A, and then A sending ViewMsg_WasShown/Hidden to C.
   2.b In mus-case, B has a handle to the Window C lives in. So B can (and should, although probably does not yet) update the visiblity of that window.
 3 It sounds like there are some scenario where A can send a ViewMsg_WasShown/Hidden to C, without a preceding FrameHostMsg_VisibilityChanged from B.
   3.a In non-mus case, this is probably OK.
   3.b In mus case, since B has the handle to the window C lives in, and not A, A is unable to set the visibility of that window. So the window has incorrect visibility state.

The problem this bug is trying to address is 3b. Is that correct? Assuming it is, the interesting question I think is: when does 3 happen? Are we sure that it is happening because it is expected to happen, or is that a bug?

sadrul@: #3 happens whenever RenderWidgetHostViewChildFrame::Hide() or Show() are called. These are exposed as part of the content public API, although I don't think any place in our code calls them on an OOPIF's view (and that maybe doesn't make sense to do anyway?). Concretely, the frame is hidden when the RenderFrame is first created and shown when it commits; I don't know how important this is, new RWHVs have always done that, but nowadays nothing should try to render before a BeginMainFrame so it could be vestigial. Also there is a Show() or Hide() when the entire tab becomes shown or hidden. For the last, the WebContents broadcasts ViewMsg_WasShown/Hidden to all RenderWidgets in the tab.

That notification still needs to occur because Blink wants to know if its content is visible or not (e.g. Page::page_visibility_state_ should be accurate). In the case of the whole Page being shown or hidden, it could feasibly be the case that RenderFrameProxies in the renderer process tell the Window Tree that the page has been hidden so children frames should be hidden also.

When switching tabs: I think it makes sense to explicitly hide all the RenderWidgets within that tab. The difficulty with mus here is that since the browser (A) does not know about the windows for the OOPIFs (since they are managed by their parent renderers), it cannot hide those windows. So the visibility of the mus-window and the render-widget gets out of sync. One possible fix here is to explicitly hide all child windows in a renderer when the renderer itself is hidden [1] (of course, if the render-widget just became visible, then we need to set the OOPIF window to be visible only of the associated render-frame is also visible). I think that would work?

In all the cases where we hide an OOPIF from browser, if we are also hiding the main-renderer (which is the case for tab-switching), then even if the window's visibility state goes out of sync, it still shouldn't affect the hit-testing/event-dispatch, right? Because the top-most renderer window will be hidden, and so no window in that sub-tree should receive events anyway.

[1] https://cs.chromium.org/chromium/src/content/renderer/mus/renderer_window_tree_client.cc?l=261

To Sadrul's comments in #10. This bug is against both 2 and 3. C's visibility is currently never updated. For 2, I'm not sure if B should immediately change the visibility of C, or if it's better for C to do it in ViewMsg_WasShown/Hidden. If B updates the visibility I'm not sure if messages are going to get out of order.

3b sure makes it sound easier of C controls the visibility. That is, when 3b gets a ViewMsg_WasShown/Hidden it changes the visibility directly.

I had originally wanted to make C control it's own visibility. That is, when any renderer gets a ViewMsg_WasShown/Hidden it updates the visibility of the window, but we do *not* want that to happen for the main frame. We only want the browser to change the visibility of the main frame, otherwise we end up with a race.

How about I add a parameter to ViewMsg_WasShown/Hidden to indicate if the frame is the main frame. For mus if the frame is not the main frame the visibility of the mus window is updated.

To comment #12, does it matter if we don't hide all the windows in that case? While the windows would still be visible, they are not drawn. If the renderer cares about this case it can detect it via OnWindowParentDrawnStateChanged [1]

[1] https://chromium.googlesource.com/chromium/src/+/master/services/ui/public/interfaces/window_tree.mojom#461

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fc1a895a2a6e8ab8f63dd19fcc7186e29ccf0772

commit fc1a895a2a6e8ab8f63dd19fcc7186e29ccf0772
Author: Scott Violet <sky@chromium.org>
Date: Mon Oct 16 16:23:58 2017

chromeos: makes renderer frames set the visibility of the mus window

This also adds an option when embedding such that the embedded client
can't change the visibility. This way for the main frame only the
browser can change the visibility.

BUG= 772101 
TEST=covered by tests

Change-Id: Ib8ba92b2ec427e7a0e39de477d8bc259b1dcbadb
Reviewed-on: https://chromium-review.googlesource.com/719478
Reviewed-by: Elliot Glaysher <erg@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509073}
[modify] https://crrev.com/fc1a895a2a6e8ab8f63dd19fcc7186e29ccf0772/content/browser/renderer_host/render_widget_host_view_aura.cc
[modify] https://crrev.com/fc1a895a2a6e8ab8f63dd19fcc7186e29ccf0772/content/renderer/mus/renderer_window_tree_client.cc
[modify] https://crrev.com/fc1a895a2a6e8ab8f63dd19fcc7186e29ccf0772/services/ui/public/interfaces/window_tree_constants.mojom
[modify] https://crrev.com/fc1a895a2a6e8ab8f63dd19fcc7186e29ccf0772/services/ui/ws/window_server.cc
[modify] https://crrev.com/fc1a895a2a6e8ab8f63dd19fcc7186e29ccf0772/services/ui/ws/window_tree.cc
[modify] https://crrev.com/fc1a895a2a6e8ab8f63dd19fcc7186e29ccf0772/services/ui/ws/window_tree.h
[modify] https://crrev.com/fc1a895a2a6e8ab8f63dd19fcc7186e29ccf0772/services/ui/ws/window_tree_unittest.cc