I'm also seeing this during my regular browsing on many sites (e.g. theguardian.com, arstechnica, theverge...).

Nasko's OOO.  We haven't been actively testing on Android, but Ken may have some thoughts.  Also adding Nick for TDI.

Did this work on an earlier version on this device, or was it always broken?  Does it work in --top-document-isolation?  (We've seen OOPIFs work on Android in the past, so maybe it's a regression?)

With --top-document-isolation the iframe is not rendered, either..

The same device was used for testing when TDI was being developed and it worked at that time, so it's a regression.

Yes, this has been very much neglected. We are aware that there will be catch-up work to be done on Android before shipping TDI, see  bug 471850 .

It's probably not a regression then.

Now that we have FrameBlamer info in the trace, Xiaocheng and I were hoping to do some A/B trace analysis. And doing so on Android felt more exciting than desktop platforms.

Let us know if we should just try to do A/Bs on desktop platform in the meantime.

Yes, doing tests on desktop probably makes sense for now.  Alternatively, I'm sure we'd love help getting it to work on Android.  :)

Here's what I found:

The main cause of the bug is that RenderWidgetHostViewAndroid does not use unified begin frame, it implements its own BeginFrame in https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_view_android.cc?l=1751.

enne@ suggested that the BeginFrameSource should go through DelegatedFrameHostAndroid, and there's a TODO to hook it up: https://cs.chromium.org/chromium/src/ui/android/delegated_frame_host_android.cc?l=231.

Also, even though DelegatedFrameHostAndroid is a SurfaceFactoryClient, it doesn't register itself in the SurfaceManager::RegisterSurfaceFactoryClient, so that would also need to be fixed.

I've also tried a hacky solution of forwarding BeginFrame from RenderWidgetHostViewAndroid through WebContentsImpl (patch attached), and that works, though it's not a good solution and we should get a proper fix.

Deleted: patch.txt 3.0 KB

patch.txt 3.0 KB View Download

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

commit a487a27cc88695723d102e43592b14a5958ad4e3
Author: enne <enne@chromium.org>
Date: Fri Sep 30 19:56:18 2016

Fix OOPIFs on Android

RenderWidgetHostViewAndroid / DelegatedFrameHostAndroid don't
participate correctly in unified begin frame.  They listen to
vsync messages from the window android themselves and then
send begin frames to the renderer.

This patch hooks up RWHVA into the surface client namespace hierarchy in
surface manager by registering its client id and its association with
the CompositorImpl's client id.  Because RenderWidgetHostViewChildFrame
attaches itself to the RWHVAndroid's client id, this allows child frames
to get the begin frame sources that they need to drive themselves.

In the future, RWHVAs will no longer be WindowAndroidObservers
and will instead get their begin frame source via the surface manager.
However, thorny issues around begin frame source pausing need to be
addressed first before this can be removed.  So, this patch is only
a halfway step to make sure that begin frames flow from
CompositorImpl -> RWHVAndroid -> RWHVChildFrame even if
RWHVAndroid ignores it and uses its own.

OOPIFs were broken when begin frame scheduling was turned on in
https://codereview.chromium.org/1939253002.  Because the child frames
weren't hooked up to the surface hierarchy, they never got a begin frame
source, never ticked, and thus never rendered at all.

BUG= 621835 

Review-Url: https://codereview.chromium.org/2369643002
Cr-Commit-Position: refs/heads/master@{#422193}

[modify] https://crrev.com/a487a27cc88695723d102e43592b14a5958ad4e3/content/browser/renderer_host/compositor_impl_android.cc
[modify] https://crrev.com/a487a27cc88695723d102e43592b14a5958ad4e3/content/browser/renderer_host/compositor_impl_android.h
[modify] https://crrev.com/a487a27cc88695723d102e43592b14a5958ad4e3/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/a487a27cc88695723d102e43592b14a5958ad4e3/ui/android/delegated_frame_host_android.cc
[modify] https://crrev.com/a487a27cc88695723d102e43592b14a5958ad4e3/ui/android/delegated_frame_host_android.h
[modify] https://crrev.com/a487a27cc88695723d102e43592b14a5958ad4e3/ui/android/window_android_compositor.h

This is fixed, right?