I think maybe this is the cause: 09017524c90ade8e722677f5b0b67d53efdb4a71? "Empty" iframes aren't compositing now (whereas they used to?), but maybe they should still call requestAnimationFrame? Again, the use case here is a controller iframe drawing into other iframes, I don't want to have to just go back to setTimeout(, 60).

FWIW in Windows I see no differences between Chrome 64, 65, 66, 67, 68, or Firefox.
Note, Chrome doesn't invoke rAF callbacks in invisible cross-origin iframes by design since v51, see  issue 516640 .

Perhaps it was a "bug" in Chrome 65 (Mac?), but I was certainly getting rAF in those same iframes before I updated to 66 yesterday. Just to make sure of what you're seeing on Chrome 64-68 and Firefox, the code samples on the right should include a "run" button (see attachment). If you don't see the run button, then that means the embed isn't loading and you're just seeing the fallback component. Could you let me know which of the two you see?

I see the run button Safari and Firefox for Mac, as well as the previous version of Chrome for Mac.

Deleted: with-run-button.png 111 KB

	with-run-button.png 111 KB View Download

I think this might be due to the site isolation experiment we're currently running.  I'm not seeing the run button on Mac canary 68.0.3400.0 with --site-per-process enabled, but I do see it when turning site isolation off. 

tolmasky@gmail.com, can you please confirm if things are fixed if you change chrome://flags/#site-isolation-trial-opt-out to "Opt-out (not recommended)"?  Also, can you post your variations from chrome://version?

Ken, can you please help triage this?

I do in fact see the button if I turn off the trial opt-out. I was actually already running "Strict site isolation" (which I also had to turn off).

Perhaps that is a useful data point: On the previous Chrome (65, Mac), the button was working with strict site isolation (not the trial, the normal setting).

Here is my info from versions (this is with the TRIAL TURNED OFF, but STRICT SITE ISOLATION TURNED ON -- hence once again in not working state, if you want me to change these and resend this info I'm happy to do so -- not sure if it changes):

c134752e-b8b72c88
5f419cc9-ca7d8d80
a6674cf-ca7d8d80
3095aa95-3f4a17df
d52c4ff7-d52c4ff7
47e5d3db-3d47f4f4
4dc30737-b8a5ea08
f9884634-659882c0
121ae2bc-ca7d8d80
57f575bb-3d47f4f4
ceff87ec-ca7d8d80
120703dd-3f4a17df
d0ecf1da-ca7d8d80
4b61504a-c9eb6633
ef05a96e-e2c3ac67
9773d3bd-f23d1dea
8e3b2dc5-93702590
9e5c75f1-59a27b6e
3de1fbf2-3d47f4f4
f79cb77b-3d47f4f4
4ea303a6-ecbb250e
2b33233e-d8253d6f
72606c4f-3f4a17df
58a025e3-c2b41702
2a32876a-ca7d8d80
4bc337ce-69465896
9a2f4e5b-d226bfeb
494d8760-52325d43
f47ae82a-746c2ad4
3ac60855-486e2a9c
f296190c-4ad60575
4442aae2-4ad60575
ed1d377-e1cc0f14
12e17bc5-e1cc0f14
75f0f0a0-6bdfffe7
e2b18481-cdc3d902
e7e71889-e1cc0f14
98426e68-ca7d8d80
94e68624-803f8fc4

I've tried bisecting a very wide range of builds (-g 65.0.3283.0 -b 67.0.3393.4) on Linux (always passing --site-per-process) and got back r540292 as the CL that introduced this regression.  I've confirmed the results of the bisect by building locally and reproing at a41e9d414caf98f688bb648d4ac2bbd9c1056cbf and a41e9d414caf98f688bb648d4ac2bbd9c1056cbf^.

I'm marking it as RB-Stable for M66, please feel free to edit if it's not the case.

I don't think this should be an RB for M66, it doesn't sound severe enough. It would block stable for 67 certainly. Charlie can you confirm?

Discussed with fsamuel offline. We are ignoring the zero-width frame, assuming that there is nothing to draw.

It's not clear to me that this is a bug per se. The web platform doesn't promise requestAnimationFrames in this case. 

https://developer.mozilla.org/en-US/docs/Web/API/window/requestAnimationFrame

"requestAnimationFrame() calls are paused in most browsers when running in background tabs or hidden <iframe>s in order to improve performance and battery life."

Sure, I guess that is part of the problem with vague definition of that sort (I don't know if "hidden" is well defined anywhere, and seems like might be subject to change). Its just that now this code will be suboptimal in Chrome vs. every other browser, since I'll just be doing something like reliableRequestAnimationFrame = isChrome ? x => setTimeout(x, something) : requestAnimationFrame;

Adding some more graphics folks for opinions.

Just for informational purposes, we use requestAnimationFrame in at least two ways in "invisible" iframes in RunKit:

1. The first is the demo linked above. Instead of having 20 React apps loaded into 20 iframes and thus shooting Chrome's memory through the roof, we load the React app once in an invisible iframe (width:0), which then renders to the 20 other iframes (memory went from 1GB+ to 300MB with this approach, load is much faster, etc). Here, the idea is that you can't rely on an iframe just drawing itself, it may be drawing somewhere else.

[IFRAME_CONTROLLER] - draw -> [IFRAME_1]
                    - draw -> [IFRAME_2]
                    - draw -> [IFRAME_3]

Example: http://nodejs-org-runkit-demo.com/buffer/

2. The second place we use it is when loading user content/html. We load the user content in one iframe by posting their HTML there, then we load another iframe at the same origin that periodically calculates the size of the contents of the first iframe, so that we can resize the enclosing iframe to fit. This way, we don't have to inject scripts into the user's content and observe the size changes onubtrusivelt. This seems to currently still be working (I guess some assortment of iframe properties still mark it "visible" despite not being seen). You can see this here: https://runkit.com/tolmasky/issue-834822-resize-example (hover over hello and you will see the enclosing iframe resizes to fit the contents):

[IFRAME_1: rAF(() => parent.postMessage("size", computeSize(IFRAME_2) ]
[IFRAME_2: user content ]

Thank you for the feedback, I'll restore the existing behavior for M67.

Nice, thank you!

Ok, it looks like the reason why this breaks currently in Chrome with Site Isolation is that the frames doing all the work are cross-site from the main frame. This means they get OOPIFs and that kicks in the logic from r540292. If they were all same-origin, rAFs would have worked.

Overall, this means that we have difference in behavior between same process and cross process iframes which we need to unify. Let's restore the behavior and follow up whether we want to fire rAFs for invisible frames separately.

One workaround also is to make the frame driving the animation 1px wide instead of zero. I just tried it by changing it while the page has already loaded and the buttons showed up almost immediately.

Thanks all for feedback. So this seems isolated to site isolation, and since we're running a limited experiment on 66, this shouldn't block ramp-up imo. I'm moving this from M66 to 67. Let me know if any objections. 

Thanks all!  That sounds like a good plan to me-- (1) don't block M66 since it should affect only a small percentage of users, (2) aim to restore previous rAF behavior for zero size iframes (including OOPIFs) in M67 with Fady's upcoming CL, and (3) separately consider whether there's a motivation to change behavior for invisible iframes (consistently for same and cross origin frames).

nasko@: Thanks for noting that there's a workaround in comment 19, just in case it's useful to deploy to users who have Site Isolation turned on (or are in the trial) in M66.

To follow up on the second example in #15, Nasko and I poked a bit at the two pages, since we were curious why things are broken in the first example but not the second, even though in both cases the cross-site iframe is zero-sized (0x100 in first page, 0x0 in second).  

One experiment I did was just to add a requestAnimationFrame(()=>{console.log("raf")}) in both zero-sized iframes.  On the first (broken) page, it never fires, as expected.  On the second page, it prints "raf" as soon as you hover over the "Hello", so it seems that a hover over the second visible iframe is triggering the rAF in the first (invisible) iframe (the two are different local roots in the same process) - is there a sensible explanation for that?

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

commit f29633128ee314cc95f1f222a337f27e06fd4001
Author: Fady Samuel <fsamuel@chromium.org>
Date: Fri Apr 20 16:04:44 2018

Surface synchronization: iframes of zero size have valid LocalSurfaceId.

Webapps occasionally use 0x0 iframes for requestAnimationFrame and so
expect to continue to receive BeginFrames even though they don't generate
CompositorFrames.

Thus, We should not defer commits in cc and thus those iframes should get a
valid viz::LocalSurfaceId. This CL restores the original behavior.

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Iec09293c5925a6440b857a8b4d28f5c92d0a10d1
Bug:  834822 ,  672962 
Reviewed-on: https://chromium-review.googlesource.com/1020299
Reviewed-by: Saman Sami <samans@chromium.org>
Commit-Queue: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552345}
[modify] https://crrev.com/f29633128ee314cc95f1f222a337f27e06fd4001/cc/trees/layer_tree_host.cc
[modify] https://crrev.com/f29633128ee314cc95f1f222a337f27e06fd4001/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/f29633128ee314cc95f1f222a337f27e06fd4001/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc

Just a heads up, I pushed a workaround for the time being as we are using this page for demo purposes. If you need me to put code back up that exhibits this behavior, please let me know and we'll spin up a server that serves the old version of the code (that we'd like to eventually return to).

As an FYI, I had to not only have the width be 1px, but also needed to have the controller iframe live in position:fixed; top:0; left:0; because otherwise if the iframe is scrolled away random emebds won't load.

Comment 25: Thanks for the heads up about the original URL, and glad you were able to deploy the workaround for now!

fsamuel@: Thanks for landing the fix!  Looks like that's in 68.0.3402.0.  Will you be able to verify it on a simpler repro page, since the original page no longer shows the bug?  Once we confirm the fix, let's request merge to M67.

Your change meets the bar and is auto-approved for M67. Please go ahead and merge the CL to branch 3396 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

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

commit cbae0cb8514f86123a10a95a08f5e7fb1f2c6d9e
Author: Fady Samuel <fsamuel@chromium.org>
Date: Tue Apr 24 19:24:53 2018

Surface synchronization: iframes of zero size have valid LocalSurfaceId.

Webapps occasionally use 0x0 iframes for requestAnimationFrame and so
expect to continue to receive BeginFrames even though they don't generate
CompositorFrames.

Thus, We should not defer commits in cc and thus those iframes should get a
valid viz::LocalSurfaceId. This CL restores the original behavior.

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Iec09293c5925a6440b857a8b4d28f5c92d0a10d1
Bug:  834822 ,  672962 
Reviewed-on: https://chromium-review.googlesource.com/1020299
Reviewed-by: Saman Sami <samans@chromium.org>
Commit-Queue: Fady Samuel <fsamuel@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#552345}(cherry picked from commit f29633128ee314cc95f1f222a337f27e06fd4001)
Reviewed-on: https://chromium-review.googlesource.com/1026490
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#261}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/cbae0cb8514f86123a10a95a08f5e7fb1f2c6d9e/cc/trees/layer_tree_host.cc
[modify] https://crrev.com/cbae0cb8514f86123a10a95a08f5e7fb1f2c6d9e/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/cbae0cb8514f86123a10a95a08f5e7fb1f2c6d9e/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc

M67 Stable promotion is coming soon. Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and request a merge into the release branch ASAP. Thank you.

Marking this fixed.  fsamuel@, let me know if there's anything more to do here.  

(If we want to consider changing rAF to exclude empty iframes in both same-origin and cross-origin cases, I'll suggest doing that in a separate issue.  Then again, it seems like there's a reasonable use case shown here, so maybe the better option is to clarify in the spec that it should work in empty iframes.)

Thanks!