Cross-process postMessage from an unload handler may not reach its target frame due to race with SwapOut ACK |
||
Issue descriptionRepro steps: 1. Go to http://csreis.github.io/tests/cross-site-iframe.html 2. In main frame, run: window.onmessage = (e) => console.log("message: " + e.data); 3. Click "Go cross-site (simple page)", and in the subframe, run: window.addEventListener("unload", function(e) { parent.postMessage("unload","*"); }) 4. Click "Go same-site" Expected: console output shows message: unload Actual: no output See https://bugs.chromium.org/p/chromium/issues/detail?id=851882#c12 for the full explanation. Basically, in this case the unload handler runs as part of RenderFrameImpl::OnSwapOut, and at that time the parent proxy through which the postMessage is sent still exists. However, because the postMessage is scheduled due to r550769 from issue 828529, its IPC is delayed and sent after the FrameHostMsg_SwapOut_ACK is sent at the end of OnSwapOut(). When the browser process receives the SwapOut ACK, it destroys the pending-delete RFH and all the remaining proxies, as this was the last active frame in the process. Then, the incoming postMessage IPC is dropped because the RenderFrameProxyHost to which it was addressed was just deleted. I've confirmed that if I add another frame at https://csreis.github.io to the main frame and then repeat this experiment, the postMessage does get delivered properly. To ensure that the postMessage is still delivered here, we could consider sending postMessages right away if they're sent from an unload handler, which should be a simple fix, though it'd make postMessage scheduling harder to reason about. Or, maybe we can just schedule the SwapOut_ACK itself, so it always goes out after any scheduled postMessage IPCs are sent.
,
Jun 29 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5c21f3ef93cfe2c7b7578e9726ff3f730db77037 commit 5c21f3ef93cfe2c7b7578e9726ff3f730db77037 Author: Alex Moshchuk <alexmos@chromium.org> Date: Fri Jun 29 21:05:09 2018 Delay SwapOut ACK to fix postMessages sent from subframe unload handlers. After r550769, cross-process postMessages are scheduled with PostTask, so that the actual postMessage IPC goes out on next iteration of the event loop. This is problematic when postMessage is executed from an unload handler that is triggered on a local-to-remote frame swap in RenderFrameImpl::OnSwapOut: the SwapOut ACK ends up being sent before any postMessage IPCs, and if this was the last active frame in the process, the browser process, upon receiving the SwapOut ACK, might destroy proxies that a postMessage was targeting. To prevent this, schedule the SwapOut ACK to ensure that it gets sent after any postMessage IPCs. Bug: 857274 Change-Id: I9e7339c0abc409fd201e7f73927e871c0f0d3b95 Reviewed-on: https://chromium-review.googlesource.com/1119391 Reviewed-by: Charlie Reis <creis@chromium.org> Commit-Queue: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#571625} [modify] https://crrev.com/5c21f3ef93cfe2c7b7578e9726ff3f730db77037/content/browser/frame_host/render_frame_host_manager_browsertest.cc [modify] https://crrev.com/5c21f3ef93cfe2c7b7578e9726ff3f730db77037/content/renderer/render_frame_impl.cc
,
Jun 29 2018
Thanks for the fix! Once we verify it in a Canary with r571625, we should likely merge this to M68 to help address some of the unload regressions with Site Isolation.
,
Jul 2
The NextAction date has arrived: 2018-07-02
,
Jul 3
Verified that this issue is fixed in Mac canary 69.0.3479.0, which includes r571625, using repro steps above. Note though that there was also a test that became flaky and was disabled around the same time - see issue 859315. FindIt also posted a comment on https://chromium-review.googlesource.com/c/chromium/src/+/1119391 that there's a chance this CL might've caused it. I'll try to track down the source of flakiness, though I'm also busy with beforeunload and disappearing for a few days after Jul 3, so if anyone can help out, that's be great! In any case, I'll let this bake for a couple more days before requesting merge to 68.
,
Jul 9
I can also confirm that the bug is fixed in Windows Canary 69.0.3486.0. However, the FindIt results do seem to suggest it might have caused some of the tests to become racy: https://findit-for-me.appspot.com/waterfall/flake/flake-culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyQwsSDEZsYWtlQ3VscHJpdCIxY2hyb21pdW0vNWMyMWYzZWY5M2NmZTJjN2I3NTc4ZTk3MjZmZjNmNzMwZGI3NzAzNww SitePerProcessBrowserTest.CrossProcessMainFrameNavigationDoesNotOverwriteHistory RenderFrameHostManagerTest.AllowTargetedNavigationsAfterSwap The first test fails like this, where a back navigation doesn't end up on the correct URL in the test: ../../content/browser/site_per_process_browsertest.cc:9537: Failure Expected equality of these values: bar_url Which is: http://bar.com:42379/title2.html web_contents()->GetMainFrame()->GetLastCommittedURL() Which is: http://foo.com:42379/title1.html The second test fails like this: ../../content/browser/frame_host/render_frame_host_manager_browsertest.cc:712: Failure Value of: NavigateToURLInSameBrowsingInstance(new_shell, cross_site_url) Actual: false Expected: true I'm not sure how posting the SwapOut ack task would cause either of those, but I'm hesitant to merge this to M68 until we know more. I'll leave this as M69 until Alex has a chance to take a closer look. |
||
►
Sign in to add a comment |
||
Comment 1 by alex...@chromium.org
, Jun 28 2018