Propagate inert state to OOPIFs when a modal dialog is active

third_party/WebKit/Source/core/frame/LocalFrame.cpp

Index: third_party/WebKit/Source/core/frame/LocalFrame.cpp
diff --git a/third_party/WebKit/Source/core/frame/LocalFrame.cpp b/third_party/WebKit/Source/core/frame/LocalFrame.cpp
index 6e6ca09fc502cf82686a4eef346e100a1c46d0c3..b085308c2cfc19accecf6c75e3bf9250edb145d4 100644
--- a/third_party/WebKit/Source/core/frame/LocalFrame.cpp
+++ b/third_party/WebKit/Source/core/frame/LocalFrame.cpp
@@ -591,6 +591,21 @@ void LocalFrame::DidChangeVisibilityState() {
   Frame::DidChangeVisibilityState();
 }
 
+void LocalFrame::SetIsInert(bool inert) {
+  is_inert_ = inert;
+  PropagateInertToChildFrames();

[alexmos, 2017/06/16 02:17:57]

Should this also only be done if |is_inert_| was different from |inert|?

[kenrb, 2017/06/19 19:26:23]

No, because sometimes the propagation to child frames is needed even when the
current frame's bit doesn't change; i.e. because an element in the current frame
changed. In a previous iteration I had checked the child's bit from
PropagateInertToChildFrame() and only propagated if it would change, but removed
it because it required an extra method on Frame.h and it seemed okay to be
slightly conservative.

[alexmos, 2017/06/20 18:46:10]

Acknowledged.

+}
+
+void LocalFrame::PropagateInertToChildFrames() {
+  for (Frame* child = Tree().FirstChild(); child;
+       child = child->Tree().NextSibling()) {

[alexmos, 2017/06/16 02:17:57]

How does this walk work for nested RemoteFrames?  I.e., for a(b(c)), seems like
we'll only call SetIsInert on b but not on c.  Or do you rely on b.com
LocalFrame sending the message once it receives it?  If the latter, could we
just walk the whole hierarchy here and send the message to all remote frame
descendants, to avoid extra roundtrips?

[kenrb, 2017/06/19 19:26:23]

From within Blink, we don't know if nested OOPIFs exist. If (c) is same-process
under (b), then the update will happen properly within the child frame's
renderer. If (c) is remote from (b), then there will be an extra round trip, as
you note. If we want to optimize that then it would have to be within the
browser process, but would add complexity (because (b) would need to know to
*not* update (c)) and I'm not sure the trade-off is worth it.

[alexmos, 2017/06/20 18:46:10]

I was thinking that if we just send the IPC to all remote frames, and each
RenderFrameProxyHost forwards it to whatever process is hosting that actual
frame, then we don't really need any extra knowledge, right?  I.e., for a(b(c)),
we could send the IPC here to both (b) and (c) proxies, which would forward them
to b.com and c.com frames.  For a case like a(b(b(b))), we'd send three IPCs
instead of one with your approach, so it's a tradeoff.  I'm fine with your
approach though - I wouldn't expect inertness to be used often enough for us to
care about # of IPCs or roundtrips involved.

[kenrb, 2017/06/22 15:33:35]

There is another problem here. Think about a(b(c)) where a and b frames both
contain elements with the inert attribute. If a's element removes the inert
attribute then it sends messages to b and c to stop being inert. But b still
wants c to be inert, so it now has to send a message to c to become inert again,
which is racing with a's message. This is complicated to resolve.

[alexmos, 2017/06/22 17:09:27]

Acknowledged - I agree that's a good reason to keep your current approach.

+    if (child->Owner()) {

[alexmos, 2017/06/16 02:17:57]

Is there any case where this could be null, given that these are child frames?

[kenrb, 2017/06/19 19:26:23]

Maybe not. I've removed it and layout tests seem to pass.

+      child->SetIsInert(is_inert_ ||
+                        ToHTMLFrameOwnerElement(child->Owner())->IsInert());

[alexmos, 2017/06/16 02:17:57]

When would is_inert_ be different from
ToHTMLFrameOwnerElement(child->Owner())->IsInert()?

[kenrb, 2017/06/19 19:26:23]

If child->Owner()->IsInert() returns true, it means there is an element in this
Frame that is causing other elements to become inert, including the child Frame
element. is_inert_ being true means that we are inheriting inertness from a
parent frame's context.

[alexmos, 2017/06/20 18:46:10]

Thanks for clearing up my confusion. :)  It might be helpful to add a short
comment along those lines.

[kenrb, 2017/06/22 15:33:35]

Done.

+    }
+  }
+}
+
 LocalFrame& LocalFrame::LocalFrameRoot() const {
   const LocalFrame* cur_frame = this;
   while (cur_frame && cur_frame->Tree().Parent() &&
@@ -925,6 +940,7 @@ inline LocalFrame::LocalFrame(LocalFrameClient* client,
       page_zoom_factor_(ParentPageZoomFactor(this)),
       text_zoom_factor_(ParentTextZoomFactor(this)),
       in_view_source_mode_(false),
+      is_inert_(false),
       interface_provider_(interface_provider),
       interface_registry_(interface_registry) {
   if (FrameResourceCoordinator::IsEnabled()) {
@@ -935,6 +951,7 @@ inline LocalFrame::LocalFrame(LocalFrameClient* client,
     probe_sink_ = new CoreProbeSink();
     performance_monitor_ = new PerformanceMonitor(this);
   } else {
+    UpdateInertIfPossible();

[alexmos, 2017/06/16 02:17:57]

Perhaps a comment about why this is not needed for local roots?

[kenrb, 2017/06/19 19:26:23]

Done.

     probe_sink_ = LocalFrameRoot().probe_sink_;
     performance_monitor_ = LocalFrameRoot().performance_monitor_;
   }