beforeunload handlers don't run in OOPIFs |
||||||||||||||
Issue descriptionRepro steps: 1. With --site-per-process enabled, go to http://csreis.github.io/tests/cross-site-iframe.html 2. Open devtools, switch to the subframe's context, and install a beforeunload handler: window.onbeforeunload = function(e){ return "foo"; }; 3. Attempt to close the tab by clicking on the "x", and notice the beforeunload prompt appears. Cancel the close. 4. Now click "Go cross-site (simple)" 5. Add the same beforeunload handler in the subframe. 6. Attempt to close the tab by clicking on the "x". The tab closes without waiting for the subframe to run its beforeunload handler. Analysis: When a tab (or set of tabs) is closed, TabStripModel::CloseWebContentses() uses FastShutdownIfPossible() to shut down the closing renderer processes that don't have any beforeunload or unload handlers as quickly as possible. However, it only does this for each tab's main frame process: https://cs.chromium.org/chromium/src/chrome/browser/ui/tabs/tab_strip_model.cc?l=1311&rcl=2baa7afe761df394d1ce0fab07dc36cdaa680bd6 With OOPIFs, I think this will ignore beforeunload and unload handlers in subframe processes. Note that if the main frame process ends up doing fast shutdown, the subframe processes will die as a result of FrameTreeNode::ResetForNewProcess(), and after the fix for issue issue 852204 , they will delay shutdown to give unload handlers a chance to run. But it seems beforeunload will never happen. The actual beforeunload/unload checking happens in RenderProcessHostImpl::FastShutdownIfPossible, which returns early if !SuddenTerminationAllowed(), which is set when the renderer process installs a beforeunload or unload handler for any of its frames. It seems that TabStripModel::CloseWebContentses() should be checking all processes on a tab rather than just the main frame process for fast shutdown eligibility, though I haven't checked whether that's enough to fix this. It's actually worth considering whether (cross-site?) iframes should be able to prevent a tab from closing. Avi, any thoughts? This is a behavioral difference with OOPIFs and could lead to data loss in forms inside iframes, but on the other hand, nobody has complained yet despite our increasingly wide deployment of site isolation. (Found as a result of discussion on (internal) site isolation+unload doc: https://docs.google.com/document/d/1XiNu0iAnYpxyObsdwStxTNYq83BoPW0lwjG5xbb1l88/edit?usp=sharing)
,
Jun 17 2018
Is this a complicated fix? beforeunload handlers are already unreliable on mobile and even on desktop we have bugs with them not running. In most cases, I can imagine that the main frame is the one doing the work and grabbing the beforeunload event, so it's not visible much. I have no big position here.
,
Jun 19 2018
I believe this bug is causing a large problem for Microsoft Office Online. Our application always runs in an iframe on another domain, and we wire up beforeunload handlers so that we can close sessions on our servers when the user navigates or otherwise closes the browser tab. This bug causes those handlers to not run, so we wind up with sessions on our servers that don’t close when they should. This is a major problem from our perspective – it breaks our application in a pretty fundamental way. Users remain locked out of their files for a few minutes until our service decides the session ended unexpectedly, and the final changes they made might not get saved.
,
Jun 19 2018
Comment 3: Thanks for the report. Can you provide some context on why beforeunload is being used for that, rather than unload? Beforeunload runs in cases even where the user doesn't leave the page, like when the user says "Stay" or if the navigation turns out to be a download or 204 response. Unload handlers seem like a better fit for tasks like that. The fix for unload handlers in issue 852204 has just been merged to M68 and may even be possible to get into M67, if it's possible for you to use that instead (since it seems like a better fit at first glance).
,
Jun 20 2018
Comment 1: Another relevant TODO is in RenderFrameImpl::OnBeforeUnload: // TODO(clamy): Ensure BeforeUnload is dispatched to all subframes, even when // --site-per-process is enabled. |dispatchBeforeUnloadEvent| will only // execute the BeforeUnload event in this frame and local child frames. It // should also be dispatched to out-of-process child frames. bool proceed = frame_->DispatchBeforeUnloadEvent(is_reload); The point is that the renderer process is the one traversing the frame tree to trigger beforeunload handlers, but it should be notifying other processes when other acks are required as well. Chances are, the whole management should be moved to the browser process so it can notify each process involved (unclear if frame order matters enough that it should be frame-by-frame), collecting enough acks until it's ready to proceed with the navigation (or until a timeout occurs). This is non-trivial. (See also issue 365039, where we've had trouble skipping beforeunload in cases that no handler is defined.) It may be possible to easily notify OOPIFs to run their beforeunload handlers in the background without letting them put up dialogs, if that offers a short-term quick fix to the problem in comment 3. It would only help in cases where code needs to run and not when the user needs to make a decision about leaving, though.
,
Jun 20 2018
Comment 3: Is it possible to share a link that shows the specific Office Online problem, so that we can test it on our side? We're evaluating options for fixing this (potentially prioritizing that use case for a faster fix), and it would help to have a page to verify against. Thanks!
,
Jun 21 2018
Comment 4: I wasn't as clear as I should have been. We use both OnUnload and OnBeforeUnload for different purposes. OnBeforeUnload is used in some cases to prompt the user prior to a navigation, and OnUnload is used to send our final "the user's leaving" requests. In our testing on Chrome 67, we're not getting *either* of those two events reliably. Comment 6: I'll see if there's a simpler way to illustrate the problem, but the quick and dirty way to test this is to use Dropbox. Open a Microsoft Word file for editing (you may need to approve the Office Online app - typical OAuth flow) in Word Online, then navigate away or close the tab. Then re-open the same document. You'll notice that you'll show up in the document as two users - this is because we didn't get the "user is leaving" request. I'll see if we can provide a simpler example or repro.
,
Jun 21 2018
Comment 7: Thanks for your response! I think this aligns nicely with how we expect these handlers to be used. It is correct that in Chrome 67, both beforeunload and unload events do not reliably run in cross-site iframes. The unload part of the problem has been recently fixed in issue 852204 (available in Canary versions 69.0.3461.2+) and merged to M68. It's available in the Beta release (68.0.3440.33) which just went out today. Would you be able to check if your problem is solved on that version? For beforeunload, since your use case does require prompting the user, it wouldn't benefit from the short-term fix idea in #5 of running beforeunload handlers in background without letting them put up dialogs.
,
Jun 21 2018
Agreed, thanks for clarifying! Let us know if the unload fix in Chrome 68.0.3440.33 is working for you, and we'll prioritize work on the beforeunload dialog for prompting users.
,
Jun 21 2018
Comment 8/9: I haven't done exhaustive testing yet, but my initial testing of 68.0.3440.33 looks good. The OnUnload handlers seem to be triggered as expected. I'll continue to test and if I note anything related to OnUnload I'll add details to the other bug ( issue 852204 ). When is 68 going to release?
,
Jun 21 2018
Minor bit tangentisl question: Tyler could you confirm that you are not using sync XHR in unload / beforeunload, we are trying to disallow it: https://groups.google.com/a/chromium.org/d/msg/blink-dev/LnqwTCiT9Gs/tO0IBO4PAwAJ
,
Jun 21 2018
Glad to hear. Chrome 68 is scheduled to reach stable around July 24: https://www.chromium.org/developers/calendar We're looking into whether we can get the fix merged to Chrome 67 just in case there's a respin (and your confirmation of the fix helps). That said, there are currently no plans for an updated Chrome 67 release.
,
Jun 21 2018
Comment 12: Ouch - July 24 is quite a long time without a workaround. Is there such a workaround we could implement? If there's any info I can provide to help justify porting the fix to 67, please let me know. We are highly motivated to address this ASAP, since we're treating it as a service issue (and customers are understandably frustrated). Comment 11: We do rely on synchronous XHR at the moment but we are aware of the deprecation plans. If I remember correctly, navigator.sendBeacon() is the replacement for our use. I don't want to further derail this thread since it's not directly related, but feel free to ping me if you want to chat more about this.
,
Jun 21 2018
+govind@ for FYI. Comment 13: I'm starting a conversation about M67 possibilities and other options. Unfortunately, we do not know of a workaround for unload handlers. Any merge decisions will be made on issue 852204 , so I'll CC you there.
,
Jun 21 2018
Comment 13: We're starting to plan an M67 respin in issue 852204 . Please do let us know if you can verify that the current fix in M68 is sufficient for the time being. (We'll continue to look into the beforeunload dialog case in this issue.) Thanks!
,
Jun 22 2018
,
Jun 22 2018
,
Jun 22 2018
Issue 855280 has been merged into this issue.
,
Jun 29 2018
Status update: I'm working on a fix for running beforeunload handlers in OOPIFs, where the browser process will fire off the missing beforeunload requests and collect ACKs from all subframe renderers before letting a navigation or tab close proceed. The fix is going to be non-trivial, but I'm still aiming for it to be mergeable to M68. I hope to have something ready for review in the next few days. There are still some tricky corner cases to work through: - Ensuring we show at most one dialog per navigation. This is currently implemented on the renderer side in https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/dom/document.cc?l=3469&rcl=0c770d185f97bbbcce9d24b65ca244225ce4b664, and we'd need this on the browser side. I was thinking also that we might need to ensure a dialog from a main frame is shown before subframes, but thanks to avi@'s work in issue 587940 , this shouldn't matter, as the dialogs look all the same now and don't contain any custom strings. So I think we can just wait for the first one and ignore the rest. - Ensuring all the ways to start beforeunload work (browser-initiated navigation, renderer-initiated navigation, tab close, reload). - Figuring out how to best split the work between browser and renderers, i.e. do renderers keep running beforeunload in all local descendants, or do they only do one frame per IPC? - There are a bunch of renderer-side metrics around beforeunload. If they are too complicated to move right away, we might consider postponing them to a separate patch which won't necessarily need to be merged to M68.
,
Jun 29 2018
I don't think we'll be able to get this into M67, so updating the target to M68.
,
Jul 3
Note that we also had a report of this issue on chromium-dev: https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/d590d145-c1bb-483c-95e2-757c00b9e975%40chromium.org
,
Jul 13
Quick status update - I've got a CL for fixing this up at https://chromium-review.googlesource.com/c/chromium/src/+/1121586. It's reviewed and will hopefully land later today, and we can see how it does on canary over the weekend. Also, given that this was a non-trivial fix, I wrote a design doc with implementation details and issues I hit along the way: https://docs.google.com/document/d/1XmwjGNUl_PF9OgDTs0JsRe0iuGrtEk6mzDYGkkbqHKg/edit?usp=sharing
,
Jul 14
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6fcaca758f6cea737e39ff09f3c1621f1e7a2c5b commit 6fcaca758f6cea737e39ff09f3c1621f1e7a2c5b Author: Alex Moshchuk <alexmos@chromium.org> Date: Sat Jul 14 02:13:59 2018 Support beforeunload handlers for OOPIFs. Currently, when a frame navigates, or a tab closes, we only run beforeunload handlers in the affected frame, but not in its cross-process subframes. This can cause data loss in those subframes. For example, if a subframe contains an editable Google Doc or Office Online doc with unsaved changes, it uses beforeunload to prompt the user about losing those changes. This CL introduces support to dispatch beforeunload handlers to cross-process subframes. We now dispatch multiple FrameMsg_BeforeUnload IPCs and wait for all of them to receive beforeunload ACKs before proceeding with a navigation or tab close. This is done for browser-initiated navigations, renderer-initiated navigations, and when closing a tab or browser. For more details and design discussions, see https://docs.google.com/document/d/1XmwjGNUl_PF9OgDTs0JsRe0iuGrtEk6mzDYGkkbqHKg/edit# A few notable tradeoffs and compromises in this initial CL: - We send one IPC per local root (roughly) and let Blink process same-site descendants. - Cross-site beforeunload handlers run in parallel; we only guarantee ordering among same-site frames. This is similar to how we handle unload handlers. - For now, we always send the beforeunload IPC to a navigating frame. For subframes, we only send it if the subframe has registered a beforeunload handler. - Renderer-initiated navigations still execute the navigating frame's beforeunload before notifying the browser process about the navigation. The browser process then additionally executes any cross-site subframe beforeunload handlers, if needed. This avoids an extra IPC roundtrip. - beforeunload in window.close() is not yet supported. This is essentially a renderer-initiated tab close, and it expects to synchronously set window.closed after executing beforeunload handlers in all subframes. Supporting this is left to a future CL. - Blink currently implements an intervention to limit the number of beforeunload dialogs shown to at most one per navigation. To support this with OOPIFs, similar support is added on the browser side in RFHI::OnRunBeforeUnloadConfirm(). More followup work will be needed here with renderer-initiated navigations and with recording metrics. Change-Id: I0b1f86c119a548ee16d34eff229705748f9560be Bug: 853021 Reviewed-on: https://chromium-review.googlesource.com/1121586 Commit-Queue: Lei Zhang <thestig@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Reviewed-by: Lei Zhang <thestig@chromium.org> Cr-Commit-Position: refs/heads/master@{#575133} [modify] https://crrev.com/6fcaca758f6cea737e39ff09f3c1621f1e7a2c5b/chrome/browser/chrome_site_per_process_browsertest.cc [modify] https://crrev.com/6fcaca758f6cea737e39ff09f3c1621f1e7a2c5b/chrome/browser/ui/tabs/tab_strip_model.cc [modify] https://crrev.com/6fcaca758f6cea737e39ff09f3c1621f1e7a2c5b/chrome/browser/unload_browsertest.cc [modify] https://crrev.com/6fcaca758f6cea737e39ff09f3c1621f1e7a2c5b/content/browser/frame_host/navigator_impl.cc [modify] https://crrev.com/6fcaca758f6cea737e39ff09f3c1621f1e7a2c5b/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/6fcaca758f6cea737e39ff09f3c1621f1e7a2c5b/content/browser/frame_host/render_frame_host_impl.h [modify] https://crrev.com/6fcaca758f6cea737e39ff09f3c1621f1e7a2c5b/content/browser/frame_host/render_frame_host_impl_browsertest.cc [modify] https://crrev.com/6fcaca758f6cea737e39ff09f3c1621f1e7a2c5b/content/browser/renderer_host/render_view_host_impl.cc [modify] https://crrev.com/6fcaca758f6cea737e39ff09f3c1621f1e7a2c5b/content/browser/web_contents/web_contents_impl.cc [modify] https://crrev.com/6fcaca758f6cea737e39ff09f3c1621f1e7a2c5b/content/browser/web_contents/web_contents_impl.h [modify] https://crrev.com/6fcaca758f6cea737e39ff09f3c1621f1e7a2c5b/content/public/browser/web_contents.h [modify] https://crrev.com/6fcaca758f6cea737e39ff09f3c1621f1e7a2c5b/content/renderer/render_frame_impl.cc
,
Jul 16
I verified that the fix in r575133 is working in 69.0.3493.0 Mac Canary using the following steps: 1. With --site-per-process, go to http://alexmos17.github.io/tests/embedded-doc.html 2. Start typing a new draft comment in the embedded doc, but don't save it. 3. Try to close the old doc by one of the following steps: a. Type "http://csreis.github.io" in the omnibox (browser-initiated navigation) b. Open DevTools and type "location = 'http://www.asdf.com';" (renderer-initiated navigation) c. Close the tab by clicking on the x d. Close the window (command-W) e. Close the browser (command-Q, or from the menu, Chrome->Quit Google Chrome) All of these cases now result in a prompt "Leave site? Changes you made may not be saved", and you can either cancel or proceed. Prior to the fix in r575133, all the operations in 3 would proceed without the prompt, resulting in losing the draft comment.
,
Jul 16
There's some followup work for which I'll file separate bugs, but I think we can consider the main issue fixed at this point.
,
Jul 16
Great! I can confirm the fix on today's 69.0.3493.0 Windows Canary as well, using the steps in comment 24 on both http://alexmos17.github.io/tests/embedded-doc.html and http://alexmos17.github.io/tests/beforeunload.html (after adding a beforeunload handler in the subframe with the button). I also tested merging the fix to a local M68 branch. It hit a few small merge conflicts (mostly trivial, plus a moved block of code from r562250), but they were easy to resolve. It compiled and passed the new tests, and I verified the new behavior is working correctly. That draft CL is here if you'd like to look: https://chromium-review.googlesource.com/c/chromium/src/+/1137839 I didn't see any obvious crashes from this in 69.0.3492.0, and Alex said he would take a closer look. After that, we're likely going to request merge to M68, given the importance of this fix to prevent data loss when leaving pages with subframe beforeunload handlers. It's a non-trivial CL, but a lot of attention has gone into testing and verifying it. We think it's important to have in M68 since it's high priority for embedded cases of both Microsoft Office Online and Google Docs.
,
Jul 16
I verified that there aren't any new crashes in 69.0.3492.0 (which is where the fix initially appeared) and 69.0.3493.0. The only crashed that looked related to site isolation in the top 500 was for content::RenderFrameProxy::RenderFrameProxy, which is a separate issue 794625 and was there before my fix as well. We'll keep watching things closely to make sure nothing else comes up.
,
Jul 16
Great, thanks for double-checking, Alex. After chatting with Abdul, we agree this is an important fix to try to include in M68. As mentioned in comment 26, we think it should be safe given the extra attention we've given to testing and verifying it, and since it ties into the existing (mature) beforeunload dialog logic. I'll request the merge now, and we'll intend to land the merge Tuesday morning (just to give it a little extra bake time to be on the safe side).
,
Jul 16
Approving merge to M68. Branch:3440 As discussed, let's verify this further in today and tomorrow's canary, and if it looks good, please go ahead merge to m68 tomorrow.
,
Jul 17
,
Jul 17
Alex double checked and didn't see any new crashes from this today, so we're proceeding with the merge.
,
Jul 17
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8da446508bb2890542a71ea6814cc1ef24951a34 commit 8da446508bb2890542a71ea6814cc1ef24951a34 Author: Charlie Reis <creis@chromium.org> Date: Tue Jul 17 17:06:34 2018 Merge to M68: Support beforeunload handlers for OOPIFs. Currently, when a frame navigates, or a tab closes, we only run beforeunload handlers in the affected frame, but not in its cross-process subframes. This can cause data loss in those subframes. For example, if a subframe contains an editable Google Doc or Office Online doc with unsaved changes, it uses beforeunload to prompt the user about losing those changes. This CL introduces support to dispatch beforeunload handlers to cross-process subframes. We now dispatch multiple FrameMsg_BeforeUnload IPCs and wait for all of them to receive beforeunload ACKs before proceeding with a navigation or tab close. This is done for browser-initiated navigations, renderer-initiated navigations, and when closing a tab or browser. For more details and design discussions, see https://docs.google.com/document/d/1XmwjGNUl_PF9OgDTs0JsRe0iuGrtEk6mzDYGkkbqHKg/edit# A few notable tradeoffs and compromises in this initial CL: - We send one IPC per local root (roughly) and let Blink process same-site descendants. - Cross-site beforeunload handlers run in parallel; we only guarantee ordering among same-site frames. This is similar to how we handle unload handlers. - For now, we always send the beforeunload IPC to a navigating frame. For subframes, we only send it if the subframe has registered a beforeunload handler. - Renderer-initiated navigations still execute the navigating frame's beforeunload before notifying the browser process about the navigation. The browser process then additionally executes any cross-site subframe beforeunload handlers, if needed. This avoids an extra IPC roundtrip. - beforeunload in window.close() is not yet supported. This is essentially a renderer-initiated tab close, and it expects to synchronously set window.closed after executing beforeunload handlers in all subframes. Supporting this is left to a future CL. - Blink currently implements an intervention to limit the number of beforeunload dialogs shown to at most one per navigation. To support this with OOPIFs, similar support is added on the browser side in RFHI::OnRunBeforeUnloadConfirm(). More followup work will be needed here with renderer-initiated navigations and with recording metrics. Change-Id: I0b1f86c119a548ee16d34eff229705748f9560be Bug: 853021 Reviewed-on: https://chromium-review.googlesource.com/1121586 Commit-Queue: Lei Zhang <thestig@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Reviewed-by: Lei Zhang <thestig@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#575133} Reviewed-on: https://chromium-review.googlesource.com/1137839 Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/branch-heads/3440@{#694} Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733} [modify] https://crrev.com/8da446508bb2890542a71ea6814cc1ef24951a34/chrome/browser/chrome_site_per_process_browsertest.cc [modify] https://crrev.com/8da446508bb2890542a71ea6814cc1ef24951a34/chrome/browser/ui/tabs/tab_strip_model.cc [modify] https://crrev.com/8da446508bb2890542a71ea6814cc1ef24951a34/chrome/browser/unload_browsertest.cc [modify] https://crrev.com/8da446508bb2890542a71ea6814cc1ef24951a34/content/browser/frame_host/navigator_impl.cc [modify] https://crrev.com/8da446508bb2890542a71ea6814cc1ef24951a34/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/8da446508bb2890542a71ea6814cc1ef24951a34/content/browser/frame_host/render_frame_host_impl.h [modify] https://crrev.com/8da446508bb2890542a71ea6814cc1ef24951a34/content/browser/frame_host/render_frame_host_impl_browsertest.cc [modify] https://crrev.com/8da446508bb2890542a71ea6814cc1ef24951a34/content/browser/renderer_host/render_view_host_impl.cc [modify] https://crrev.com/8da446508bb2890542a71ea6814cc1ef24951a34/content/browser/web_contents/web_contents_impl.cc [modify] https://crrev.com/8da446508bb2890542a71ea6814cc1ef24951a34/content/browser/web_contents/web_contents_impl.h [modify] https://crrev.com/8da446508bb2890542a71ea6814cc1ef24951a34/content/public/browser/web_contents.h [modify] https://crrev.com/8da446508bb2890542a71ea6814cc1ef24951a34/content/renderer/render_frame_impl.cc
,
Jul 18
I verified that things are working in 68.0.3440.68. I did spot one case that isn't working properly and filed issue 865223 . If multiple frames have a beforeunload handler, then the dialog gets shown but Chrome proceeds through it after 1 second. That's hopefully rare compared to the case of a beforeunload handler in an OOPIF, so the fix here is still valuable. We'll try to get a fix for the multiple frame case soon. |
||||||||||||||
►
Sign in to add a comment |
||||||||||||||
Comment 1 by alex...@chromium.org
, Jun 15 2018