AsyncBeforeUnload: Run beforeunload handlers asynchronously if possible [chromium/src : main]
Minoru Chikamune (Gerrit)
Minoru Chikamune added 1 comment![]( "Open in Gerrit")
Hi, I implemented the AsyncBeforeUnload feature. Still I'm adding tests, let me start gathering your opinions.
Related details
- Alex Moshchuk
- Charlie Reis
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AI Code Reviewer (Gerrit)
AI Code Reviewer added 2 comments![]( "Open in Gerrit")
bool force_to_proceed,Blink Style Guide: Prefer enums or StrongAliases to bare bools for function parameters. Consider using a base::StrongAlias<class ForceToProceedTag, bool> or an enum class for the 'force_to_proceed' parameter to improve readability and type safety, especially since it is adjacent to another boolean parameter.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
bool force_to_proceed,Blink Style Guide: Prefer enums or StrongAliases to bare bools for function parameters. Consider using a base::StrongAlias<class ForceToProceedTag, bool> or an enum class for the 'force_to_proceed' parameter to improve readability and type safety.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Related details
- Alex Moshchuk
- Charlie Reis
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Charlie Reis (Gerrit)
Charlie Reis added 1 comment![]( "Open in Gerrit")
Hi, I implemented the AsyncBeforeUnload feature. Still I'm adding tests, let me start gathering your opinions.
Thanks! I probably won't get through this today, but I'll try to review it soon.
Related details
- Alex Moshchuk
- Charlie Reis
- Minoru Chikamune
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minoru Chikamune (Gerrit)
Minoru Chikamune added 2 comments![]( "Open in Gerrit")
Blink Style Guide: Prefer enums or StrongAliases to bare bools for function parameters. Consider using a base::StrongAlias<class ForceToProceedTag, bool> or an enum class for the 'force_to_proceed' parameter to improve readability and type safety, especially since it is adjacent to another boolean parameter.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
OK But Won't Fix: `force_to_proceed` comes from the mojom interface.
Blink Style Guide: Prefer enums or StrongAliases to bare bools for function parameters. Consider using a base::StrongAlias<class ForceToProceedTag, bool> or an enum class for the 'force_to_proceed' parameter to improve readability and type safety.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
OK But Won't Fix: `force_to_proceed` comes from the mojom interface.
Related details
- Alex Moshchuk
- Charlie Reis
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 5 comments![]( "Open in Gerrit")
Thanks! I'm still trying to understand the whole beforeunload flow now, but I left some quick comments.. also can you please add a test?
!rfh->HasStickyUserActivation() && navigation_request &&From this [comment](https://docs.google.com/document/d/1QMAlEocmRVbTWkm5qoRMpn1T0Pb9qmfCSw4ydeZn1So/edit?disco=AAABxMe2Qco) should you be checking the whole subtree instead?
if (!for_legacy && force_to_proceed) {Can this actually be put earlier in the function before `before_unload_closure` to avoid nesting?
initiator->async_beforeunload_pending_replies_.erase(I wonder if this should be a member of the `NavigationRequest` instead. What happens if multiple navigations happen in the same FrameTree (e.g. a parent and a subframe independently triggered navigation), what does the async beforeunload tracking look like?
Related details
- Alex Moshchuk
- Charlie Reis
- Minoru Chikamune
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minoru Chikamune (Gerrit)
Minoru Chikamune added 3 comments![]( "Open in Gerrit")
!rfh->HasStickyUserActivation() && navigation_request &&From this [comment](https://docs.google.com/document/d/1QMAlEocmRVbTWkm5qoRMpn1T0Pb9qmfCSw4ydeZn1So/edit?disco=AAABxMe2Qco) should you be checking the whole subtree instead?
Oh, yeah. I actually changed my mind while I was writing this CL, because partially runnning beforeunload in the background also improve the performance. But yeah, I'll probably add a feature param so that we can try both.
initiator->async_beforeunload_pending_replies_.erase(I wonder if this should be a member of the `NavigationRequest` instead. What happens if multiple navigations happen in the same FrameTree (e.g. a parent and a subframe independently triggered navigation), what does the async beforeunload tracking look like?
Yeah, I wondered the same. I borrowed this idea from the existing `beforeunload_pending_replies_`, which waits for the all replies from the renderer, and it ended up with putting this on the initiator rfh.
Should we return early here?
Actually, it shouldn't. After running beforeunload in the renderer process, I want to proceed the navigation to start fetching the main resource below.
Related details
- Alex Moshchuk
- Charlie Reis
- Rakina Zata Amni
Minoru Chikamune (Gerrit)
Minoru Chikamune added 1 comment![]( "Open in Gerrit")
!rfh->HasStickyUserActivation() && navigation_request &&Minoru ChikamuneFrom this [comment](https://docs.google.com/document/d/1QMAlEocmRVbTWkm5qoRMpn1T0Pb9qmfCSw4ydeZn1So/edit?disco=AAABxMe2Qco) should you be checking the whole subtree instead?
Oh, yeah. I actually changed my mind while I was writing this CL, because partially runnning beforeunload in the background also improve the performance. But yeah, I'll probably add a feature param so that we can try both.
I found a bug from your review comment, Rakina. Thank you so much. Currently, The current CL only checks the current frame, but in renderer, FrameLoader::ShouldClose() checks all the descendant frames as well. I'll fix it.
Minoru Chikamune (Gerrit)
Minoru Chikamune added 1 comment![]( "Open in Gerrit")
The android-x86-rel failure is likely a pre-existing flake and unrelated to this CL. (See: https://crbug.com/480202119)
Related details
- Alex Moshchuk
- Charlie Reis
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 12 comments![]( "Open in Gerrit")
Sorry for the delayed review and many questions! (Still trying to understand the flow of beforeunload..)
// proceed navigation if running the previous page's beforeunload takes too
// long. (e.g. If the previous page's beforeunload handler has anOut of curiosity, do we have something like this for the sync version as well? What is the timeout value for that?
// while the navigation proceeds. This is only possible if no frame with ain the subtree? Also how does this interact with `subframes_only` (and maybe `send_ipc` too?)
// handler asynchronously when there is no user gesture on the frame. (see:subtree?
async_beforeunload_pending_replies_.clear();Why is this not in the `is_waiting_for_beforeunload_completion_` if clause?
// `this` into `beforeunload_pending_replies_` as a placeholder, letting theWhy do we need this placeholder? Also why don't we just put things directly in `async_beforeunload_pending_replies_` instead of swapping here?
SendBeforeUnloadAsync(is_reload, rfh->GetWeakPtr());Why do some callers call `SendBeforeUnloadAsync()` but some others call `SendBeforeUnload()` with `should_run_before_unload_asynchronously` to true? What's the difference and in which case should we call which ones? Is it possible to just have one function for async beforeunload?
!rfh->HasStickyUserActivation() && navigation_request &&Minoru ChikamuneFrom this [comment](https://docs.google.com/document/d/1QMAlEocmRVbTWkm5qoRMpn1T0Pb9qmfCSw4ydeZn1So/edit?disco=AAABxMe2Qco) should you be checking the whole subtree instead?
Minoru ChikamuneOh, yeah. I actually changed my mind while I was writing this CL, because partially runnning beforeunload in the background also improve the performance. But yeah, I'll probably add a feature param so that we can try both.
I found a bug from your review comment, Rakina. Thank you so much. Currently, The current CL only checks the current frame, but in renderer, FrameLoader::ShouldClose() checks all the descendant frames as well. I'll fix it.
Rakina Zata AmniIn the end, I ended up with implementing all-or-nothing. PTAL.
Acknowledged
Can this actually be put earlier in the function before `before_unload_closure` to avoid nesting?
Rakina Zata AmniDone
Acknowledged
Minoru ChikamuneShould we return early here?
Actually, it shouldn't. After running beforeunload in the renderer process, I want to proceed the navigation to start fetching the main resource below.
Rakina Zata AmniThank you so much. I ended up to change the logic. PTAL.
Acknowledged
if (initiator_rfh && rfh) {Is it possible to get a reply from an already deleted RFH? Will that cause the navigation to be deferred until the timeout? Is there a better way to handle that?
if (navigation_request) {I wonder if this check should be earlier. Are beforeunloads tied to the RFH, or to the navigation? If there's a navigation #1 that triggers a beforeunload async, but then the navigation gets cancelled, and there's a new navigation #2, would that trigger a new beforeunload? If the reply for the first beforeunload came and removed that second beforeunload, is that ok?
Related details
- Alex Moshchuk
- Charlie Reis
- Minoru Chikamune
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minoru Chikamune (Gerrit)
Minoru Chikamune added 2 comments![]( "Open in Gerrit")
Thank you for checking! I'll reply them tomorrow.
// proceed navigation if running the previous page's beforeunload takes too
// long. (e.g. If the previous page's beforeunload handler has anOut of curiosity, do we have something like this for the sync version as well? What is the timeout value for that?
`RenderFrameHostImpl::beforeunload_timeout_delay_` is doing it. The timeout value is 500ms. `UnloadTest.CrossSiteInfiniteBeforeUnloadAsync` tests it.
```
constexpr base::TimeDelta kUnloadTimeout = base::Milliseconds(500);
```
Related details
- Alex Moshchuk
- Charlie Reis
- Rakina Zata Amni
Minoru Chikamune (Gerrit)
Minoru Chikamune added 7 comments![]( "Open in Gerrit")
// while the navigation proceeds. This is only possible if no frame with aMinoru Chikamunein the subtree? Also how does this interact with `subframes_only` (and maybe `send_ipc` too?)
Sure. I'll add about it in the code comment.
// handler asynchronously when there is no user gesture on the frame. (see:Minoru Chikamunesubtree?
Yeah, the frame that is navigating away and its descendants. I'll update the comment.
async_beforeunload_pending_replies_.clear();Minoru ChikamuneWhy is this not in the `is_waiting_for_beforeunload_completion_` if clause?
It's because is_waiting_for_beforeunload_completion_ is already false when the beforeunload is asynchronously running.
// `this` into `beforeunload_pending_replies_` as a placeholder, letting theMinoru ChikamuneWhy do we need this placeholder? Also why don't we just put things directly in `async_beforeunload_pending_replies_` instead of swapping here?
Why do we need this placeholder?
This is the same behavior as L12018: `beforeunload_pending_replies_.insert(this);`. It adds the frame (navigation root) as a "placeholder", and `RenderFrameHostImpl::ProcessBeforeUnloadCompletedFromFrame()` consumes it (`beforeunload_pending_replies_.erase(frame);`).
Also why don't we just put things directly in async_beforeunload_pending_replies_ instead of swapping here?
`RenderFrameHostImpl::CheckOrDispatchBeforeUnloadForFrame()` algorithm relies on `beforeunload_pending_replies_` not to send duplicated request. So, `beforeunload_pending_replies_` should be kept as is until the `ForEachRenderFrameHostImplWithAction` ends.
Initially, I didn't understand it, and actually, the beforeunload handlers run twice before in the test.
SendBeforeUnloadAsync(is_reload, rfh->GetWeakPtr());Minoru ChikamuneWhy do some callers call `SendBeforeUnloadAsync()` but some others call `SendBeforeUnload()` with `should_run_before_unload_asynchronously` to true? What's the difference and in which case should we call which ones? Is it possible to just have one function for async beforeunload?
`should_run_before_unload_asynchronously` is decided only once for one navigation. After the decision, always use SendBeforeUnloadAsync() when `should_run_before_unload_asynchronously` is true.
if (initiator_rfh && rfh) {Minoru ChikamuneIs it possible to get a reply from an already deleted RFH? Will that cause the navigation to be deferred until the timeout? Is there a better way to handle that?
Is it possible to get a reply from an already deleted RFH?
Umm. Probably no. Probably I can get rid of this `&& rfh` condition.
if (navigation_request) {Minoru ChikamuneI wonder if this check should be earlier. Are beforeunloads tied to the RFH, or to the navigation? If there's a navigation #1 that triggers a beforeunload async, but then the navigation gets cancelled, and there's a new navigation #2, would that trigger a new beforeunload? If the reply for the first beforeunload came and removed that second beforeunload, is that ok?
Yeah... you are right. As you said before [1], async_beforeunload_pending_replies_ should be kept in navigation_request. Otherwise, the code can't handle the scenario you mentioned. Probably running beforeunload handlers asynchronously increases such situation. I'll update this CL.
[1] https://chromium-review.googlesource.com/c/chromium/src/+/7413835/comment/d54bf1ce_3c1a522e/
Minoru Chikamune (Gerrit)
Minoru Chikamune added 2 comments![]( "Open in Gerrit")
// while the navigation proceeds. This is only possible if no frame with aMinoru Chikamunein the subtree? Also how does this interact with `subframes_only` (and maybe `send_ipc` too?)
Sure. I'll add about it in the code comment.
Done
// handler asynchronously when there is no user gesture on the frame. (see:Minoru Chikamunesubtree?
Yeah, the frame that is navigating away and its descendants. I'll update the comment.
Done
Related details
- Alex Moshchuk
- Charlie Reis
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Charlie Reis (Gerrit)
Charlie Reis added 11 comments![]( "Open in Gerrit")
Sorry for the delay-- I'm glad you've been making progress with Rakina's review. Some initial questions and nits below as I start to go through this. (I haven't reviewed the render_frame_host_impl.cc implementation or tests yet.)
// too long, preventing the navigation from being hung indefinitely.Just curious-- do we need a separate timer for this if there's already [one in RFHI](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.h;drc=976a71ef2a509d4391d814b52b9f4700fdf35789;l=4756)? If RenderFrameHostImpl::BeforeUnloadTimeout() had a way of telling this condition to proceed, would that avoid the need for a second timer?
I mainly ask because it seems tricky to think through all the possibilities of which timer fires first, if at all. If there's a good way to think about how they relate to each other, that might be worth putting in the comment here.
// asynchronouslyminor nit: End with period.
class AsyncBeforeunloadCommitDeferringConditionminor nit: I think we usually capitalize the U in BeforeUnload, such as in [DispatchBeforeUnload](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/web_contents/web_contents_impl.h;drc=c19b719ae21c09c2052b30b3f8b56e3ce6f51dbe;l=481), NeedToFireBeforeUnloadOrUnloadEvents, BeforeUnloadCompleted, etc.
// to finish just before the navigation is ready to commit, ensuring anynit: before allowing the navigation to commit
// (due to lack of user gesture), this condition ensures we still wait for themMaybe add: "and thus an inability to show a dialog"?
// for a timeout to occur.Can you clarify in the comment which frames this might include? Is it "this frame and/or its descendant frames ... [excluding those] that don't have beforeunload handlers defined" like beforeunload_pending_replies_? I guess it's slightly trickier since this is on NavigationRequest instead of RFH, but maybe it still applies to the navigating frame and its descendants?
Hopefully we're careful about not leaving any dangling pointers in here if descendant frames are removed?
// sticky user activation, which guarantees that the renderer will not attempt
// to cancel navigation.Worth mentioning the dialog here.
base::WeakPtr<RenderFrameHostImpl> rfh);At first glance, I'm not sure what `rfh` is meant to be, since this is a function on RenderFrameHost already. Can you document that?
// The synchronously executed `beforeunload` handlers delay the user perceivedminor nit: Drop "The"
base::Milliseconds(500));Related to my earlier question about the separate timeout, why would we need a different timeout value if we did use one? That's probably worth documenting if we need to keep this.
Related details
- Alex Moshchuk
- Minoru Chikamune
- Rakina Zata Amni
Minoru Chikamune (Gerrit)
Minoru Chikamune added 15 comments![]( "Open in Gerrit")
// too long, preventing the navigation from being hung indefinitely.Just curious-- do we need a separate timer for this if there's already [one in RFHI](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.h;drc=976a71ef2a509d4391d814b52b9f4700fdf35789;l=4756)? If RenderFrameHostImpl::BeforeUnloadTimeout() had a way of telling this condition to proceed, would that avoid the need for a second timer?
I mainly ask because it seems tricky to think through all the possibilities of which timer fires first, if at all. If there's a good way to think about how they relate to each other, that might be worth putting in the comment here.
Currently, I don't have a clear answer, but I agree that it's better to use one timer instead of two timers. I'll check if I can use just one timer.
minor nit: End with period.
Done
minor nit: I think we usually capitalize the U in BeforeUnload, such as in [DispatchBeforeUnload](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/web_contents/web_contents_impl.h;drc=c19b719ae21c09c2052b30b3f8b56e3ce6f51dbe;l=481), NeedToFireBeforeUnloadOrUnloadEvents, BeforeUnloadCompleted, etc.
Sure. I'll update them all.
// to finish just before the navigation is ready to commit, ensuring anynit: before allowing the navigation to commit
Done
// (due to lack of user gesture), this condition ensures we still wait for themMaybe add: "and thus an inability to show a dialog"?
Done
// sticky user activation, which guarantees that the renderer will not attempt
// to cancel navigation.Worth mentioning the dialog here.
Done
At first glance, I'm not sure what `rfh` is meant to be, since this is a function on RenderFrameHost already. Can you document that?
Done
// The synchronously executed `beforeunload` handlers delay the user perceivedMinoru Chikamuneminor nit: Drop "The"
Done
// while the navigation proceeds. This is only possible if no frame with aMinoru Chikamunein the subtree? Also how does this interact with `subframes_only` (and maybe `send_ipc` too?)
Minoru ChikamuneSure. I'll add about it in the code comment.
Minoru ChikamuneDone
Done
// handler asynchronously when there is no user gesture on the frame. (see:Minoru Chikamunesubtree?
Minoru ChikamuneYeah, the frame that is navigating away and its descendants. I'll update the comment.
Done
Done
async_beforeunload_pending_replies_.clear();Why is this not in the `is_waiting_for_beforeunload_completion_` if clause?
It's because is_waiting_for_beforeunload_completion_ is already false when the beforeunload is asynchronously running.
In the new patch-set, I moved the async_before_unload_pending_replies from RFH to NavigationRequest. And I removed the previous reset code from RFH.
initiator->async_beforeunload_pending_replies_.erase(I wonder if this should be a member of the `NavigationRequest` instead. What happens if multiple navigations happen in the same FrameTree (e.g. a parent and a subframe independently triggered navigation), what does the async beforeunload tracking look like?
Yeah, I wondered the same. I borrowed this idea from the existing `beforeunload_pending_replies_`, which waits for the all replies from the renderer, and it ended up with putting this on the initiator rfh.
In the new patch-set, I moved the async_before_unload_pending_replies from RFH to NavigationRequest. Thank you for pointing this out.
Probably, the current intention to keep beforeunload_pending_replies_ on RFH was that the beforeunload dialog should be shown once when the user initiate multiple navigations in a short period of time.
In our case, when `SendBeforeUnloadAsync` code path is used, it does not show dialogs, but perhaps SendBeforeUnloadAsync runs more beforeunload handlers. I'm not super shure if this is ok or not.
Minoru ChikamuneIs it possible to get a reply from an already deleted RFH? Will that cause the navigation to be deferred until the timeout? Is there a better way to handle that?
Is it possible to get a reply from an already deleted RFH?
Umm. Probably no. Probably I can get rid of this `&& rfh` condition.
Removed the `&& rfh` condition.
Minoru ChikamuneI wonder if this check should be earlier. Are beforeunloads tied to the RFH, or to the navigation? If there's a navigation #1 that triggers a beforeunload async, but then the navigation gets cancelled, and there's a new navigation #2, would that trigger a new beforeunload? If the reply for the first beforeunload came and removed that second beforeunload, is that ok?
Yeah... you are right. As you said before [1], async_beforeunload_pending_replies_ should be kept in navigation_request. Otherwise, the code can't handle the scenario you mentioned. Probably running beforeunload handlers asynchronously increases such situation. I'll update this CL.
[1] https://chromium-review.googlesource.com/c/chromium/src/+/7413835/comment/d54bf1ce_3c1a522e/
In the new patch-set, I moved the async_before_unload_pending_replies from RFH to NavigationRequest.
base::Milliseconds(500));Related to my earlier question about the separate timeout, why would we need a different timeout value if we did use one? That's probably worth documenting if we need to keep this.
Ditto. Currently, I don't have a clear answer, but I agree that it's better to use one timer instead of two timers. I'll check if I can use just one timer.
Related details
- Alex Moshchuk
- Charlie Reis
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minoru Chikamune (Gerrit)
Minoru Chikamune added 2 comments![]( "Open in Gerrit")
Minoru ChikamuneThanks! I'm still trying to understand the whole beforeunload flow now, but I left some quick comments.. also can you please add a test?
Now we have content/browser/renderer_host/render_frame_host_impl_browsertest.cc.
The android-x86-rel failure is likely a pre-existing flake and unrelated to this CL. (See: https://crbug.com/480202119)
Done
Minoru Chikamune (Gerrit)
Minoru Chikamune voted and added 1 comment![]( "Open in Gerrit")
Votes added by Minoru Chikamune
| Commit-Queue | +1 |
1 comment
// for a timeout to occur.Can you clarify in the comment which frames this might include? Is it "this frame and/or its descendant frames ... [excluding those] that don't have beforeunload handlers defined" like beforeunload_pending_replies_? I guess it's slightly trickier since this is on NavigationRequest instead of RFH, but maybe it still applies to the navigating frame and its descendants?
Hopefully we're careful about not leaving any dangling pointers in here if descendant frames are removed?
Can you clarify in the comment which frames
I updated the code comment.
not leaving any dangling pointers in here if descendant frames are removed?
Oh, you are right. Probably I need to do something similar to the following logic in the destructor of RFH. I'll update the code.
```
// If another frame is waiting for a beforeunload completion callback from
// this frame, simulate it now.
RenderFrameHostImpl* beforeunload_initiator = GetBeforeUnloadInitiator();
if (beforeunload_initiator && beforeunload_initiator != this) {
base::TimeTicks approx_renderer_start_time = send_before_unload_start_time_;
beforeunload_initiator->ProcessBeforeUnloadCompletedFromFrame(
/*proceed=*/true, /*treat_as_final_completion_callback=*/false, this,
/*is_frame_being_destroyed=*/true, approx_renderer_start_time,
base::TimeTicks::Now(), /*for_legacy=*/false);
}
```
Related details
- Alex Moshchuk
- Charlie Reis
- Minoru Chikamune
- Rakina Zata Amni
Alex Moshchuk (Gerrit)
Alex Moshchuk added 12 comments![]( "Open in Gerrit")
Thanks, adding a few comments as I also try to understand how everything fits together a bit more.
blocked.Can you also mention why user activation matters? Namely, the lack of user activation implies that beforeunload handlers can't show dialogs that users may use to cancel the navigation.
Probably also worth noting that this optimization affects only navigations. Beforeunload handlers for closing or detaching frames are not affected.
std::set<raw_ptr<RenderFrameHostImpl, SetExperimental>>Given the possibility that descendant RFHs might be deleted before we get a beforeunload response from them, I wonder if a safer design (improving on beforeunload_pending_replies_) would be GlobalRenderFrameHostIds that we can resolve if needed? Maybe the RFH destruction issue that Charlie mentioned would be easier to handle with such a design. (Also, I'm not sure if we actually need to resolve and call methods on these RFHs, vs just having a way to track whether each one has finished running beforeunload.)
// See the fixme comment in `FrameLoader::ShouldClose` function: "There is not
// yet any way to dispatch events to out-of-process frames."Oh, that comment is super-old and probably predates the OOPIF beforeunload support that we added in M63. It can probably be removed (since we do dispatch beforeunload to OOPIFs, just requiring them all to complete before kicking off the navigation), and I'm not sure it's worth mentioning here.
DCHECK(send_ipc);nit: I think we're supposed to prefer CHECKs in all new code.
SendBeforeUnloadAsync(is_reload, rfh->GetWeakPtr());Why do some callers call `SendBeforeUnloadAsync()` but some others call `SendBeforeUnload()` with `should_run_before_unload_asynchronously` to true? What's the difference and in which case should we call which ones? Is it possible to just have one function for async beforeunload?
`should_run_before_unload_asynchronously` is decided only once for one navigation. After the decision, always use SendBeforeUnloadAsync() when `should_run_before_unload_asynchronously` is true.
+1 to Rakina's question here, as I'm also not sure I understand yet why we need both helpers when it seems like SendBeforeUnload can support both modes (since it takes the `should_run_before_unload_asynchronously` flag). Is it because the ack processing in SendBeforeUnloadAsync is sufficiently different that it'll make SendBeforeUnload too complex if we wanted to fold it in there?
Also, maybe it would be helpful to walk through an example like your test - how would SendBeforeUnload/SendBeforeUnloadAsync end up getting invoked in that case?
frame_tree_node_->navigation_request()->GetWeakPtr();Just trying to understand: looks like this is called from CheckOrDispatchBeforeUnloadForFrame for each frame that needs to send a beforeunload IPC to the renderer. Why are we looking up the navigation request from each frame's FTN here? Shouldn't it always be retrieved from the ancestor frame that's navigating and triggering beforeunload handlers in all of its descendants? (I.e., the equivalent of what GetBeforeUnloadInitiator() does.)
bool proceed, base::TimeTicks renderer_before_unload_start_time,
base::TimeTicks renderer_before_unload_end_time,Are we going to need to process these for metrics, similarly how they're handled on the old path?
On that note, how are the NavigationRequest::MaybeRecordTraceEventsAndHistograms() trace events and metrics affected by AsyncBeforeUnload? Will they need to be updated, and if so, will that happen here or in a separate CL?
NotifyUserActivation(rfh_a);This test is named *WithoutUserActivation, so I'm confused why we're granting user activation to `rfh_a` here? Why doesn't it cause async beforeunload to be bypassed regardless of AsyncBeforeUnload feature? (More comments to explain the expected flow of the test would be great.)
We'll probably want at least a few other tests eventually, e.g. for renderer-initiated navigations, for ensuring that tab close doesn't trigger the optimization, for ensuring that user activation in one of the frames turns off the optimization, etc.
base::Milliseconds(500));Minoru ChikamuneRelated to my earlier question about the separate timeout, why would we need a different timeout value if we did use one? That's probably worth documenting if we need to keep this.
Ditto. Currently, I don't have a clear answer, but I agree that it's better to use one timer instead of two timers. I'll check if I can use just one timer.
I'd expect that by the time we get to WillCommitNavigation, the beforeunload handlers have been running for some time (since the start of navigation). So I wonder if the timer in AsyncBeforeUnloadCommitDeferringCondition::WillCommitNavigation should be set to the remaining time, so that beforeunload handlers still get 500ms total?
Related details
- Charlie Reis
- Minoru Chikamune
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minoru Chikamune (Gerrit)
Minoru Chikamune voted and added 11 comments![]( "Open in Gerrit")
Votes added by Minoru Chikamune
| Commit-Queue | +1 |
Can you also mention why user activation matters? Namely, the lack of user activation implies that beforeunload handlers can't show dialogs that users may use to cancel the navigation.
Done
Probably also worth noting that this optimization affects only navigations. Beforeunload handlers for closing or detaching frames are not affected.
Done
std::set<raw_ptr<RenderFrameHostImpl, SetExperimental>>Given the possibility that descendant RFHs might be deleted before we get a beforeunload response from them, I wonder if a safer design (improving on beforeunload_pending_replies_) would be GlobalRenderFrameHostIds that we can resolve if needed? Maybe the RFH destruction issue that Charlie mentioned would be easier to handle with such a design. (Also, I'm not sure if we actually need to resolve and call methods on these RFHs, vs just having a way to track whether each one has finished running beforeunload.)
Oh nice! GlobalRenderFrameHostId is easy way to manage this field. Thank you so much for your advice. I updated the code.
// for a timeout to occur.Minoru ChikamuneCan you clarify in the comment which frames this might include? Is it "this frame and/or its descendant frames ... [excluding those] that don't have beforeunload handlers defined" like beforeunload_pending_replies_? I guess it's slightly trickier since this is on NavigationRequest instead of RFH, but maybe it still applies to the navigating frame and its descendants?
Hopefully we're careful about not leaving any dangling pointers in here if descendant frames are removed?
Can you clarify in the comment which frames
I updated the code comment.
not leaving any dangling pointers in here if descendant frames are removed?
Oh, you are right. Probably I need to do something similar to the following logic in the destructor of RFH. I'll update the code.
```
// If another frame is waiting for a beforeunload completion callback from
// this frame, simulate it now.
RenderFrameHostImpl* beforeunload_initiator = GetBeforeUnloadInitiator();
if (beforeunload_initiator && beforeunload_initiator != this) {
base::TimeTicks approx_renderer_start_time = send_before_unload_start_time_;
beforeunload_initiator->ProcessBeforeUnloadCompletedFromFrame(
/*proceed=*/true, /*treat_as_final_completion_callback=*/false, this,
/*is_frame_being_destroyed=*/true, approx_renderer_start_time,
base::TimeTicks::Now(), /*for_legacy=*/false);
}
```
Done. I switched to use GlobalRenderFrameHostId, and created `NavigationRequest::MaybeResumeAsyncBeforeUnloadCommit()`, and now the destructor of RFHI calls it, just like the normal beforeunload handling.
// See the fixme comment in `FrameLoader::ShouldClose` function: "There is not
// yet any way to dispatch events to out-of-process frames."Oh, that comment is super-old and probably predates the OOPIF beforeunload support that we added in M63. It can probably be removed (since we do dispatch beforeunload to OOPIFs, just requiring them all to complete before kicking off the navigation), and I'm not sure it's worth mentioning here.
I see. But still this code relies on this assumption. The browser process calls the mojo API for each renderers.
e.g. For RFH tree A(B(A), C(B)), SendBeforeUnloadAsync() is called from CheckOrDispatchBeforeUnloadForFrame for A, B, C if A, B, C is hosted in a separated renderer.
nit: I think we're supposed to prefer CHECKs in all new code.
Done
SendBeforeUnloadAsync(is_reload, rfh->GetWeakPtr());Minoru ChikamuneWhy do some callers call `SendBeforeUnloadAsync()` but some others call `SendBeforeUnload()` with `should_run_before_unload_asynchronously` to true? What's the difference and in which case should we call which ones? Is it possible to just have one function for async beforeunload?
Alex Moshchuk`should_run_before_unload_asynchronously` is decided only once for one navigation. After the decision, always use SendBeforeUnloadAsync() when `should_run_before_unload_asynchronously` is true.
+1 to Rakina's question here, as I'm also not sure I understand yet why we need both helpers when it seems like SendBeforeUnload can support both modes (since it takes the `should_run_before_unload_asynchronously` flag). Is it because the ack processing in SendBeforeUnloadAsync is sufficiently different that it'll make SendBeforeUnload too complex if we wanted to fold it in there?
Also, maybe it would be helpful to walk through an example like your test - how would SendBeforeUnload/SendBeforeUnloadAsync end up getting invoked in that case?
I've merged `SendBeforeUnloadAsync` into `SendBeforeUnload` as suggested.
In the new combined `SendBeforeUnload`, the control flow works as follows:
1. When `should_run_before_unload_asynchronously` is true, the call to `SendBeforeUnload` (via `CheckOrDispatchBeforeUnloadForFrame`) initiates the asynchronous execution using a specialized callback that notifies the `NavigationRequest` via `MaybeResumeAsyncBeforeUnloadCommit`.
2. After the dispatch loop (from `ForEachRenderFrameHostImplWithAction` and `CheckOrDispatchBeforeUnloadForFrame`), `CheckOrDispatchBeforeUnloadForSubtree` moves the frame IDs from the synchronous `beforeunload_pending_replies_` to the `NavigationRequest`'s `async_before_unload_pending_replies_`.
3. Finally, an additional call to `SendBeforeUnload` (at the end of `CheckOrDispatchBeforeUnloadForSubtree`) is made to continue the navigation process. This call uses the standard path (the `for_legacy == true` path) but acknowledges the async state, allowing the navigation to proceed while the background handlers are still running.
After merging `SendBeforeUnloadAsync` into `SendBeforeUnload`, The `if (navigation_request->get_async_before_unload_pending_replies().empty())` check is required to distinguish the initial async dispatch phase from the subsequent navigation continuation.
frame_tree_node_->navigation_request()->GetWeakPtr();Just trying to understand: looks like this is called from CheckOrDispatchBeforeUnloadForFrame for each frame that needs to send a beforeunload IPC to the renderer. Why are we looking up the navigation request from each frame's FTN here? Shouldn't it always be retrieved from the ancestor frame that's navigating and triggering beforeunload handlers in all of its descendants? (I.e., the equivalent of what GetBeforeUnloadInitiator() does.)
In `SendBeforeUnloadAsync`, `this` refers to the RenderFrameHost that initiated the navigation (the root of the navigating subtree), while the `rfh` parameter represents the specific frame (which could be a descendant) that needs to run the `beforeunload` handler.
Therefore, `frame_tree_node_->navigation_request()` (accessing `this->frame_tree_node_`) correctly retrieves the `NavigationRequest` associated with the navigation root, regardless of which descendant frame is being asked to run its handler. I added a comment to make this relationship clearer.
bool proceed, base::TimeTicks renderer_before_unload_start_time,
base::TimeTicks renderer_before_unload_end_time,Are we going to need to process these for metrics, similarly how they're handled on the old path?
On that note, how are the NavigationRequest::MaybeRecordTraceEventsAndHistograms() trace events and metrics affected by AsyncBeforeUnload? Will they need to be updated, and if so, will that happen here or in a separate CL?
Are we going to need to process these for metrics, similarly how they're handled on the old path?
I thought these TimeTicks are no longer important because they are no longer on the critical path.
On that note, how are the NavigationRequest::MaybeRecordTraceEventsAndHistograms() trace events and metrics affected by AsyncBeforeUnload? Will they need to be updated, and if so, will that happen here or in a separate CL?
I thought MaybeRecordTraceEventsAndHistograms does not need to be updated because running beforeunload does not block navigation anymore.
NotifyUserActivation(rfh_a);This test is named *WithoutUserActivation, so I'm confused why we're granting user activation to `rfh_a` here? Why doesn't it cause async beforeunload to be bypassed regardless of AsyncBeforeUnload feature? (More comments to explain the expected flow of the test would be great.)
I'm sorry. The test name is misleading. Renamed.
base::Milliseconds(500));Minoru ChikamuneRelated to my earlier question about the separate timeout, why would we need a different timeout value if we did use one? That's probably worth documenting if we need to keep this.
Alex MoshchukDitto. Currently, I don't have a clear answer, but I agree that it's better to use one timer instead of two timers. I'll check if I can use just one timer.
I'd expect that by the time we get to WillCommitNavigation, the beforeunload handlers have been running for some time (since the start of navigation). So I wonder if the timer in AsyncBeforeUnloadCommitDeferringCondition::WillCommitNavigation should be set to the remaining time, so that beforeunload handlers still get 500ms total?
- Alex Moshchuk
- Charlie Reis
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minoru Chikamune (Gerrit)
Minoru Chikamune added 1 comment![]( "Open in Gerrit")
base::Milliseconds(500));Minoru ChikamuneRelated to my earlier question about the separate timeout, why would we need a different timeout value if we did use one? That's probably worth documenting if we need to keep this.
Alex MoshchukDitto. Currently, I don't have a clear answer, but I agree that it's better to use one timer instead of two timers. I'll check if I can use just one timer.
Minoru ChikamuneI'd expect that by the time we get to WillCommitNavigation, the beforeunload handlers have been running for some time (since the start of navigation). So I wonder if the timer in AsyncBeforeUnloadCommitDeferringCondition::WillCommitNavigation should be set to the remaining time, so that beforeunload handlers still get 500ms total?
still get 500ms total
Yeah, I agree. I'll address it.
I ended up to add `navigation_request->StartAsyncBeforeUnloadTimer()` to start the timer when the beforeunload handlers are started.
Related details
- Alex Moshchuk
- Charlie Reis
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minoru Chikamune (Gerrit)
Minoru Chikamune added 1 comment![]( "Open in Gerrit")
I think I addressed most of the review comments. PTAL.
Related details
- Alex Moshchuk
- Charlie Reis
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Charlie Reis (Gerrit)
Charlie Reis added 1 comment![]( "Open in Gerrit")
I think I addressed most of the review comments. PTAL.
Thanks! I'm starting to take another look (as Alex is OOO), and I'll try to send comments by tomorrow.
Minoru Chikamune (Gerrit)
Minoru Chikamune added 1 comment![]( "Open in Gerrit")
Charlie ReisI think I addressed most of the review comments. PTAL.
Thanks! I'm starting to take another look (as Alex is OOO), and I'll try to send comments by tomorrow.
Thank you!
Charlie Reis (Gerrit)
Charlie Reis added 7 comments![]( "Open in Gerrit")
Charlie ReisI think I addressed most of the review comments. PTAL.
Minoru ChikamuneThanks! I'm starting to take another look (as Alex is OOO), and I'll try to send comments by tomorrow.
Thank you!
Thanks for your patience! I wasn't able to go through the whole algorithm, but I wanted to leave some comments before disappearing. Hopefully Rakina and Alex can continue to help while I'm OOO. Thanks!
dialogs that users may use to cancel the navigation. If even a single
frame has been interacted with, the browser reverts to synchronouswith a beforeunload handler
(Correct? It sounds like it might be ok to proceed if a frame on the page has a user activation but no beforeunload handler?)
Not sure how easy it is to test a case like this (and the simpler case where a frame with an activation and a handler prevents the optimization), but that might be worth considering, if it's not already in the CL.
// chance to complete.It might be worth adding a bit of reasoning so that we don't try to remove this later without realizing the risks. Something like:
This prevents the risk that beforeunload handlers may attempt observable actions after another page has committed and the page is in a pending deletion state.
// too long, preventing the navigation from being hung indefinitely.Minoru ChikamuneJust curious-- do we need a separate timer for this if there's already [one in RFHI](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.h;drc=976a71ef2a509d4391d814b52b9f4700fdf35789;l=4756)? If RenderFrameHostImpl::BeforeUnloadTimeout() had a way of telling this condition to proceed, would that avoid the need for a second timer?
I mainly ask because it seems tricky to think through all the possibilities of which timer fires first, if at all. If there's a good way to think about how they relate to each other, that might be worth putting in the comment here.
Charlie ReisCurrently, I don't have a clear answer, but I agree that it's better to use one timer instead of two timers. I'll check if I can use just one timer.
Thanks. I left another comment about this since I see we still have two timers, and I'm not sure how to reason about them.
// navigation from being hung indefinitely.As before, can we document how this interacts with the [other beforeunload timer](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;drc=97c0a7967c365a7092885334a52c68d1c43efb91;l=11958), if we need to have both? Do we have a guarantee which one will fire first, or do we need to reason about what happens if either one fires first?
I'm still not sure I understand why we need two timers, especially if they're set around the same time and have the same duration. Maybe that can be made clearer.
// only the highest ancestor among them is added to this set. This ancestorIs this a well-defined concept? Normally we refer to local frame roots.
What would happen in a page like A(B1, B2), where B1 and B2 are siblings in the same SiteInstanceGroup? Would both be in the set? I assume so, based on the mentions of "subtree" and "descendants," and I just got a bit confused by the "multiple frames in the same SiteInstanceGroup/process" phrasing.
bool proceed, base::TimeTicks renderer_before_unload_start_time,
base::TimeTicks renderer_before_unload_end_time,Minoru ChikamuneAre we going to need to process these for metrics, similarly how they're handled on the old path?
On that note, how are the NavigationRequest::MaybeRecordTraceEventsAndHistograms() trace events and metrics affected by AsyncBeforeUnload? Will they need to be updated, and if so, will that happen here or in a separate CL?
Are we going to need to process these for metrics, similarly how they're handled on the old path?
I thought these TimeTicks are no longer important because they are no longer on the critical path.
On that note, how are the NavigationRequest::MaybeRecordTraceEventsAndHistograms() trace events and metrics affected by AsyncBeforeUnload? Will they need to be updated, and if so, will that happen here or in a separate CL?
I thought MaybeRecordTraceEventsAndHistograms does not need to be updated because running beforeunload does not block navigation anymore.
Hmm, we may want to think about this a bit more.
(1) Beforeunload can still block navigation if the commit deferring condition runs, and it would probably be good to have metrics for how much of an effect that has in practice. Hopefully we don't hit it much.
(2) The beforeunload handlers are still running and could be shown in a separate (non-nested) trace event / metric if we wanted to. That's maybe less important, but might explain more about how the events overlap in a trace (which would be educational). Maybe a metric wouldn't be required for that, though, unless there's a reason we're curious about it. (It might help explain what portion of the time ended up in the commit deferring condition, for example.)
Related details
- Alex Moshchuk
- Minoru Chikamune
- Rakina Zata Amni
Minoru Chikamune (Gerrit)
Minoru Chikamune added 11 comments![]( "Open in Gerrit")
dialogs that users may use to cancel the navigation. If even a single
frame has been interacted with, the browser reverts to synchronouswith a beforeunload handler
(Correct? It sounds like it might be ok to proceed if a frame on the page has a user activation but no beforeunload handler?)
Not sure how easy it is to test a case like this (and the simpler case where a frame with an activation and a handler prevents the optimization), but that might be worth considering, if it's not already in the CL.
with a beforeunload handler
Yes, correct. I updated the CL description.
Not sure how easy it is to test a case like this (and the simpler case where a frame with an activation and a handler prevents the optimization), but that might be worth considering, if it's not already in the CL.
I think the current `RenderFrameHostImplAsyncBeforeUnloadBrowserTest.AsyncExecution` covers the half of it.
A(B,C):
- B,C has beforeunload handler, A doesn't.
- A has an activation, B,C don't.
- And confirms that the beforeunload handler runs asynchronously.
I added `RenderFrameHostImplAsyncBeforeUnloadBrowserTest.SyncFallbackWithUserActivation` to confirm the case where this optimization is not triggered.
A(B,C):
- B,C has beforeunload handler, A doesn't.
- A,B has an activation, C doesn't.
// chance to complete.It might be worth adding a bit of reasoning so that we don't try to remove this later without realizing the risks. Something like:
This prevents the risk that beforeunload handlers may attempt observable actions after another page has committed and the page is in a pending deletion state.
Thank you! Updated.
// navigation from being hung indefinitely.As before, can we document how this interacts with the [other beforeunload timer](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;drc=97c0a7967c365a7092885334a52c68d1c43efb91;l=11958), if we need to have both? Do we have a guarantee which one will fire first, or do we need to reason about what happens if either one fires first?
I'm still not sure I understand why we need two timers, especially if they're set around the same time and have the same duration. Maybe that can be made clearer.
Ah, sorry. I should have addressed this. I updated the code to explicitly stop RFHI::beforeunload_timeout_ before starting NavigationRequest::async_before_unload_timeout_. I hope this makes it clear which timer is responsible for each phase:
1. `RFHI::beforeunload_timeout_`: Manages the synchronous phase that blocks the navigation from starting.
2. `NavigationRequest::async_before_unload_timeout_`: Manages the asynchronous phase where handlers run in the background and only block the navigation from committing.
By stopping the first timer before starting the second, we ensure they never overlap or race against each other.
// only the highest ancestor among them is added to this set. This ancestorIs this a well-defined concept? Normally we refer to local frame roots.
What would happen in a page like A(B1, B2), where B1 and B2 are siblings in the same SiteInstanceGroup? Would both be in the set? I assume so, based on the mentions of "subtree" and "descendants," and I just got a bit confused by the "multiple frames in the same SiteInstanceGroup/process" phrasing.
Thanks! The previous code comment was not accurate. I've updated the comment to use "local frame roots".
std::set<raw_ptr<RenderFrameHostImpl, SetExperimental>>Minoru ChikamuneGiven the possibility that descendant RFHs might be deleted before we get a beforeunload response from them, I wonder if a safer design (improving on beforeunload_pending_replies_) would be GlobalRenderFrameHostIds that we can resolve if needed? Maybe the RFH destruction issue that Charlie mentioned would be easier to handle with such a design. (Also, I'm not sure if we actually need to resolve and call methods on these RFHs, vs just having a way to track whether each one has finished running beforeunload.)
Oh nice! GlobalRenderFrameHostId is easy way to manage this field. Thank you so much for your advice. I updated the code.
Done
// for a timeout to occur.Minoru ChikamuneCan you clarify in the comment which frames this might include? Is it "this frame and/or its descendant frames ... [excluding those] that don't have beforeunload handlers defined" like beforeunload_pending_replies_? I guess it's slightly trickier since this is on NavigationRequest instead of RFH, but maybe it still applies to the navigating frame and its descendants?
Hopefully we're careful about not leaving any dangling pointers in here if descendant frames are removed?
Minoru ChikamuneCan you clarify in the comment which frames
I updated the code comment.
not leaving any dangling pointers in here if descendant frames are removed?
Oh, you are right. Probably I need to do something similar to the following logic in the destructor of RFH. I'll update the code.
```
// If another frame is waiting for a beforeunload completion callback from
// this frame, simulate it now.
RenderFrameHostImpl* beforeunload_initiator = GetBeforeUnloadInitiator();
if (beforeunload_initiator && beforeunload_initiator != this) {
base::TimeTicks approx_renderer_start_time = send_before_unload_start_time_;
beforeunload_initiator->ProcessBeforeUnloadCompletedFromFrame(
/*proceed=*/true, /*treat_as_final_completion_callback=*/false, this,
/*is_frame_being_destroyed=*/true, approx_renderer_start_time,
base::TimeTicks::Now(), /*for_legacy=*/false);
}
```
Done. I switched to use GlobalRenderFrameHostId, and created `NavigationRequest::MaybeResumeAsyncBeforeUnloadCommit()`, and now the destructor of RFHI calls it, just like the normal beforeunload handling.
Done
async_beforeunload_pending_replies_.clear();Why is this not in the `is_waiting_for_beforeunload_completion_` if clause?
Minoru ChikamuneIt's because is_waiting_for_beforeunload_completion_ is already false when the beforeunload is asynchronously running.
In the new patch-set, I moved the async_before_unload_pending_replies from RFH to NavigationRequest. And I removed the previous reset code from RFH.
Done
initiator->async_beforeunload_pending_replies_.erase(I wonder if this should be a member of the `NavigationRequest` instead. What happens if multiple navigations happen in the same FrameTree (e.g. a parent and a subframe independently triggered navigation), what does the async beforeunload tracking look like?
Minoru ChikamuneYeah, I wondered the same. I borrowed this idea from the existing `beforeunload_pending_replies_`, which waits for the all replies from the renderer, and it ended up with putting this on the initiator rfh.
In the new patch-set, I moved the async_before_unload_pending_replies from RFH to NavigationRequest. Thank you for pointing this out.
Probably, the current intention to keep beforeunload_pending_replies_ on RFH was that the beforeunload dialog should be shown once when the user initiate multiple navigations in a short period of time.
In our case, when `SendBeforeUnloadAsync` code path is used, it does not show dialogs, but perhaps SendBeforeUnloadAsync runs more beforeunload handlers. I'm not super shure if this is ok or not.
Done
bool proceed, base::TimeTicks renderer_before_unload_start_time,
base::TimeTicks renderer_before_unload_end_time,Minoru ChikamuneAre we going to need to process these for metrics, similarly how they're handled on the old path?
On that note, how are the NavigationRequest::MaybeRecordTraceEventsAndHistograms() trace events and metrics affected by AsyncBeforeUnload? Will they need to be updated, and if so, will that happen here or in a separate CL?
Charlie ReisAre we going to need to process these for metrics, similarly how they're handled on the old path?
I thought these TimeTicks are no longer important because they are no longer on the critical path.
On that note, how are the NavigationRequest::MaybeRecordTraceEventsAndHistograms() trace events and metrics affected by AsyncBeforeUnload? Will they need to be updated, and if so, will that happen here or in a separate CL?
I thought MaybeRecordTraceEventsAndHistograms does not need to be updated because running beforeunload does not block navigation anymore.
Hmm, we may want to think about this a bit more.
(1) Beforeunload can still block navigation if the commit deferring condition runs, and it would probably be good to have metrics for how much of an effect that has in practice. Hopefully we don't hit it much.
(2) The beforeunload handlers are still running and could be shown in a separate (non-nested) trace event / metric if we wanted to. That's maybe less important, but might explain more about how the events overlap in a trace (which would be educational). Maybe a metric wouldn't be required for that, though, unless there's a reason we're curious about it. (It might help explain what portion of the time ended up in the commit deferring condition, for example.)
(1) Beforeunload can still block navigation if the commit deferring condition runs, and it would probably be good to have metrics for how much of an effect that has in practice. Hopefully we don't hit it much.
I see. Currently, the `commit deferring condition` duration is included in `ReceiveResponseToCommit`.
e.g. Tracelog of `RenderFrameHostImplAsyncBeforeUnloadBrowserTest.AsyncBeforeUnload`:
https://ui.perfetto.dev/#!/?s=97a4c641f9ab5ecd206ddccc3dcea9ce98c6c3b8
(2) The beforeunload handlers are still running and could be shown in a separate (non-nested) trace event / metric if we wanted to. That's maybe less important, but might explain more about how the events overlap in a trace (which would be educational). Maybe a metric wouldn't be required for that, though, unless there's a reason we're curious about it. (It might help explain what portion of the time ended up in the commit deferring condition, for example.)
OK, I'll add a non-nested trace event in a follow-up CL.
NotifyUserActivation(rfh_a);Minoru ChikamuneThis test is named *WithoutUserActivation, so I'm confused why we're granting user activation to `rfh_a` here? Why doesn't it cause async beforeunload to be bypassed regardless of AsyncBeforeUnload feature? (More comments to explain the expected flow of the test would be great.)
I'm sorry. The test name is misleading. Renamed.
Done
We'll probably want at least a few other tests eventually, e.g. for renderer-initiated navigations, for ensuring that tab close doesn't trigger the optimization, for ensuring that user activation in one of the frames turns off the optimization, etc.
Sure. I added several tests, but I failed to add a test for ensuring that tab close doesn't trigger the optimization. TabClose seems tricky...
Related details
- Alex Moshchuk
- Charlie Reis
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minoru Chikamune (Gerrit)
Minoru Chikamune added 1 comment![]( "Open in Gerrit")
Minoru ChikamuneWe'll probably want at least a few other tests eventually, e.g. for renderer-initiated navigations, for ensuring that tab close doesn't trigger the optimization, for ensuring that user activation in one of the frames turns off the optimization, etc.
Sure. I added several tests, but I failed to add a test for ensuring that tab close doesn't trigger the optimization. TabClose seems tricky...
Now I added `TabCloseDoesNotUseOptimization` test. PTAL.
Related details
- Alex Moshchuk
- Charlie Reis
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 4 comments![]( "Open in Gerrit")
(thanks for the patience, I finally got through most of the beforeunload flow and starting to understand what's going on)
// proceed navigation if running the previous page's beforeunload takes too
// long. (e.g. If the previous page's beforeunload handler has anOut of curiosity, do we have something like this for the sync version as well? What is the timeout value for that?
`RenderFrameHostImpl::beforeunload_timeout_delay_` is doing it. The timeout value is 500ms. `UnloadTest.CrossSiteInfiniteBeforeUnloadAsync` tests it.
```
constexpr base::TimeDelta kUnloadTimeout = base::Milliseconds(500);
```
Acknowledged
// `this` into `beforeunload_pending_replies_` as a placeholder, letting theWhy do we need this placeholder? Also why don't we just put things directly in `async_beforeunload_pending_replies_` instead of swapping here?
Why do we need this placeholder?
This is the same behavior as L12018: `beforeunload_pending_replies_.insert(this);`. It adds the frame (navigation root) as a "placeholder", and `RenderFrameHostImpl::ProcessBeforeUnloadCompletedFromFrame()` consumes it (`beforeunload_pending_replies_.erase(frame);`).
Also why don't we just put things directly in async_beforeunload_pending_replies_ instead of swapping here?
`RenderFrameHostImpl::CheckOrDispatchBeforeUnloadForFrame()` algorithm relies on `beforeunload_pending_replies_` not to send duplicated request. So, `beforeunload_pending_replies_` should be kept as is until the `ForEachRenderFrameHostImplWithAction` ends.
Initially, I didn't understand it, and actually, the beforeunload handlers run twice before in the test.
Ah ok, hmm we do erase it from `ProcessBeforeUnloadCompletedFromFrame()` but does anything prevent us from just not adding anything at all? We don't actually check whether the RFH is in the list when erasing. But maybe it's not important, it's either confusing here or confusing there.
I'm wondering if we can just combine `beforeunload_pending_replies_` and `async_beforeunload_pending_replies_` by just adding a bool differentiating the type (async/sync) there so we can not do this swapping and placeholder and ensuring every time we want to clear `beforeunload_pending_replies_` the async ones get cleared as well.
But I guess you moved it to NavigationRequest because of my [comment](https://chromium-review.googlesource.com/c/chromium/src/+/7413835/comment/3c83d833_4b258612/) about the beforeunload being tied to the navigation instead of the RFH, which I kinda feel it should be. But having `async_beforeunload_pending_replies_` in NavigationRequest means we can trigger beforeunload multiple times for different navigations happening in different frames at the same time, but the sync version will only trigger the beforeunload once. We can't really move `beforeunload_pending_replies_` to NavigationRequest because that can also be used for frame deletion. Maybe it's fine to keep `async_beforeunload_pending_replies_` in RFH too to not differentiate the sync vs async behavior further? WDYT? I think I need @ale...@chromium.org's opinion on this too since he's more familiar with beforeunload :)
SendBeforeUnloadAsync(is_reload, rfh->GetWeakPtr());Minoru ChikamuneWhy do some callers call `SendBeforeUnloadAsync()` but some others call `SendBeforeUnload()` with `should_run_before_unload_asynchronously` to true? What's the difference and in which case should we call which ones? Is it possible to just have one function for async beforeunload?
Alex Moshchuk`should_run_before_unload_asynchronously` is decided only once for one navigation. After the decision, always use SendBeforeUnloadAsync() when `should_run_before_unload_asynchronously` is true.
Minoru Chikamune+1 to Rakina's question here, as I'm also not sure I understand yet why we need both helpers when it seems like SendBeforeUnload can support both modes (since it takes the `should_run_before_unload_asynchronously` flag). Is it because the ack processing in SendBeforeUnloadAsync is sufficiently different that it'll make SendBeforeUnload too complex if we wanted to fold it in there?
Also, maybe it would be helpful to walk through an example like your test - how would SendBeforeUnload/SendBeforeUnloadAsync end up getting invoked in that case?
I've merged `SendBeforeUnloadAsync` into `SendBeforeUnload` as suggested.
In the new combined `SendBeforeUnload`, the control flow works as follows:
1. When `should_run_before_unload_asynchronously` is true, the call to `SendBeforeUnload` (via `CheckOrDispatchBeforeUnloadForFrame`) initiates the asynchronous execution using a specialized callback that notifies the `NavigationRequest` via `MaybeResumeAsyncBeforeUnloadCommit`.
2. After the dispatch loop (from `ForEachRenderFrameHostImplWithAction` and `CheckOrDispatchBeforeUnloadForFrame`), `CheckOrDispatchBeforeUnloadForSubtree` moves the frame IDs from the synchronous `beforeunload_pending_replies_` to the `NavigationRequest`'s `async_before_unload_pending_replies_`.
3. Finally, an additional call to `SendBeforeUnload` (at the end of `CheckOrDispatchBeforeUnloadForSubtree`) is made to continue the navigation process. This call uses the standard path (the `for_legacy == true` path) but acknowledges the async state, allowing the navigation to proceed while the background handlers are still running.After merging `SendBeforeUnloadAsync` into `SendBeforeUnload`, The `if (navigation_request->get_async_before_unload_pending_replies().empty())` check is required to distinguish the initial async dispatch phase from the subsequent navigation continuation.
Thank you for the explanation! I think the hard to comprehend part is the part to continue the navigation (the if clause for `for_legacy` + now `should_run_before_unload_asynchronously`) that doesn't actually send anything to the renderer and just wants to continue the navigation (synchronously or asynchronously). That part doesn't matter at all for cases where we actually want to run the beforeunload in the renderer, and only matters for the additional call outside of the iteration that you mentioned.
Would it be possible to move that part to the caller of `SendBeforeUnload` that's outside of the iteration? We need the `before_unload_closure` but maybe we can just make that a member function? Then you also don't need the `get_async_before_unload_pending_replies()` check to differentiate the iteration vs the extra call?
Related details
- Alex Moshchuk
- Charlie Reis
- Minoru Chikamune
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minoru Chikamune (Gerrit)
Minoru Chikamune added 3 comments![]( "Open in Gerrit")
// chance to complete.Minoru ChikamuneIt might be worth adding a bit of reasoning so that we don't try to remove this later without realizing the risks. Something like:
This prevents the risk that beforeunload handlers may attempt observable actions after another page has committed and the page is in a pending deletion state.
Thank you! Updated.
Done
// only the highest ancestor among them is added to this set. This ancestorMinoru ChikamuneIs this a well-defined concept? Normally we refer to local frame roots.
What would happen in a page like A(B1, B2), where B1 and B2 are siblings in the same SiteInstanceGroup? Would both be in the set? I assume so, based on the mentions of "subtree" and "descendants," and I just got a bit confused by the "multiple frames in the same SiteInstanceGroup/process" phrasing.
Thanks! The previous code comment was not accurate. I've updated the comment to use "local frame roots".
Done
base::Milliseconds(500));Minoru ChikamuneRelated to my earlier question about the separate timeout, why would we need a different timeout value if we did use one? That's probably worth documenting if we need to keep this.
Alex MoshchukDitto. Currently, I don't have a clear answer, but I agree that it's better to use one timer instead of two timers. I'll check if I can use just one timer.
Minoru ChikamuneI'd expect that by the time we get to WillCommitNavigation, the beforeunload handlers have been running for some time (since the start of navigation). So I wonder if the timer in AsyncBeforeUnloadCommitDeferringCondition::WillCommitNavigation should be set to the remaining time, so that beforeunload handlers still get 500ms total?
Minoru Chikamunestill get 500ms total
Yeah, I agree. I'll address it.
I ended up to add `navigation_request->StartAsyncBeforeUnloadTimer()` to start the timer when the beforeunload handlers are started.
Done
Related details
- Alex Moshchuk
- Charlie Reis
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minoru Chikamune (Gerrit)
Minoru Chikamune added 4 comments![]( "Open in Gerrit")
// too long, preventing the navigation from being hung indefinitely.Just curious-- do we need a separate timer for this if there's already [one in RFHI](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.h;drc=976a71ef2a509d4391d814b52b9f4700fdf35789;l=4756)? If RenderFrameHostImpl::BeforeUnloadTimeout() had a way of telling this condition to proceed, would that avoid the need for a second timer?
I mainly ask because it seems tricky to think through all the possibilities of which timer fires first, if at all. If there's a good way to think about how they relate to each other, that might be worth putting in the comment here.
Charlie ReisCurrently, I don't have a clear answer, but I agree that it's better to use one timer instead of two timers. I'll check if I can use just one timer.
Thanks. I left another comment about this since I see we still have two timers, and I'm not sure how to reason about them.
I ended up using a separate timer, but I stopped the timer (RenderFrameHostImpl::beforeunload_timeout_) when we run AsyncBeforeUnload path to avoid having two timers running.
// `this` into `beforeunload_pending_replies_` as a placeholder, letting theMinoru ChikamuneWhy do we need this placeholder? Also why don't we just put things directly in `async_beforeunload_pending_replies_` instead of swapping here?
Rakina Zata AmniWhy do we need this placeholder?
This is the same behavior as L12018: `beforeunload_pending_replies_.insert(this);`. It adds the frame (navigation root) as a "placeholder", and `RenderFrameHostImpl::ProcessBeforeUnloadCompletedFromFrame()` consumes it (`beforeunload_pending_replies_.erase(frame);`).
Also why don't we just put things directly in async_beforeunload_pending_replies_ instead of swapping here?
`RenderFrameHostImpl::CheckOrDispatchBeforeUnloadForFrame()` algorithm relies on `beforeunload_pending_replies_` not to send duplicated request. So, `beforeunload_pending_replies_` should be kept as is until the `ForEachRenderFrameHostImplWithAction` ends.
Initially, I didn't understand it, and actually, the beforeunload handlers run twice before in the test.
Ah ok, hmm we do erase it from `ProcessBeforeUnloadCompletedFromFrame()` but does anything prevent us from just not adding anything at all? We don't actually check whether the RFH is in the list when erasing. But maybe it's not important, it's either confusing here or confusing there.
I'm wondering if we can just combine `beforeunload_pending_replies_` and `async_beforeunload_pending_replies_` by just adding a bool differentiating the type (async/sync) there so we can not do this swapping and placeholder and ensuring every time we want to clear `beforeunload_pending_replies_` the async ones get cleared as well.
But I guess you moved it to NavigationRequest because of my [comment](https://chromium-review.googlesource.com/c/chromium/src/+/7413835/comment/3c83d833_4b258612/) about the beforeunload being tied to the navigation instead of the RFH, which I kinda feel it should be. But having `async_beforeunload_pending_replies_` in NavigationRequest means we can trigger beforeunload multiple times for different navigations happening in different frames at the same time, but the sync version will only trigger the beforeunload once. We can't really move `beforeunload_pending_replies_` to NavigationRequest because that can also be used for frame deletion. Maybe it's fine to keep `async_beforeunload_pending_replies_` in RFH too to not differentiate the sync vs async behavior further? WDYT? I think I need @ale...@chromium.org's opinion on this too since he's more familiar with beforeunload :)
Thank you for your comment. This is important decision. I'll wait for alexmos@'s opinion.
SendBeforeUnloadAsync(is_reload, rfh->GetWeakPtr());Minoru ChikamuneWhy do some callers call `SendBeforeUnloadAsync()` but some others call `SendBeforeUnload()` with `should_run_before_unload_asynchronously` to true? What's the difference and in which case should we call which ones? Is it possible to just have one function for async beforeunload?
Alex Moshchuk`should_run_before_unload_asynchronously` is decided only once for one navigation. After the decision, always use SendBeforeUnloadAsync() when `should_run_before_unload_asynchronously` is true.
Minoru Chikamune+1 to Rakina's question here, as I'm also not sure I understand yet why we need both helpers when it seems like SendBeforeUnload can support both modes (since it takes the `should_run_before_unload_asynchronously` flag). Is it because the ack processing in SendBeforeUnloadAsync is sufficiently different that it'll make SendBeforeUnload too complex if we wanted to fold it in there?
Also, maybe it would be helpful to walk through an example like your test - how would SendBeforeUnload/SendBeforeUnloadAsync end up getting invoked in that case?
Rakina Zata AmniI've merged `SendBeforeUnloadAsync` into `SendBeforeUnload` as suggested.
In the new combined `SendBeforeUnload`, the control flow works as follows:
1. When `should_run_before_unload_asynchronously` is true, the call to `SendBeforeUnload` (via `CheckOrDispatchBeforeUnloadForFrame`) initiates the asynchronous execution using a specialized callback that notifies the `NavigationRequest` via `MaybeResumeAsyncBeforeUnloadCommit`.
2. After the dispatch loop (from `ForEachRenderFrameHostImplWithAction` and `CheckOrDispatchBeforeUnloadForFrame`), `CheckOrDispatchBeforeUnloadForSubtree` moves the frame IDs from the synchronous `beforeunload_pending_replies_` to the `NavigationRequest`'s `async_before_unload_pending_replies_`.
3. Finally, an additional call to `SendBeforeUnload` (at the end of `CheckOrDispatchBeforeUnloadForSubtree`) is made to continue the navigation process. This call uses the standard path (the `for_legacy == true` path) but acknowledges the async state, allowing the navigation to proceed while the background handlers are still running.After merging `SendBeforeUnloadAsync` into `SendBeforeUnload`, The `if (navigation_request->get_async_before_unload_pending_replies().empty())` check is required to distinguish the initial async dispatch phase from the subsequent navigation continuation.
Thank you for the explanation! I think the hard to comprehend part is the part to continue the navigation (the if clause for `for_legacy` + now `should_run_before_unload_asynchronously`) that doesn't actually send anything to the renderer and just wants to continue the navigation (synchronously or asynchronously). That part doesn't matter at all for cases where we actually want to run the beforeunload in the renderer, and only matters for the additional call outside of the iteration that you mentioned.
Would it be possible to move that part to the caller of `SendBeforeUnload` that's outside of the iteration? We need the `before_unload_closure` but maybe we can just make that a member function? Then you also don't need the `get_async_before_unload_pending_replies()` check to differentiate the iteration vs the extra call?
Sorry for my late reply; I somehow missed your comment.
Would it be possible to move that part to the caller of SendBeforeUnload
That makes sense. I'll update the patch to reflect this.
Minoru ChikamuneWe'll probably want at least a few other tests eventually, e.g. for renderer-initiated navigations, for ensuring that tab close doesn't trigger the optimization, for ensuring that user activation in one of the frames turns off the optimization, etc.
Minoru ChikamuneSure. I added several tests, but I failed to add a test for ensuring that tab close doesn't trigger the optimization. TabClose seems tricky...
Now I added `TabCloseDoesNotUseOptimization` test. PTAL.
- Alex Moshchuk
- Charlie Reis
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minoru Chikamune (Gerrit)
Minoru Chikamune added 1 comment![]( "Open in Gerrit")
SendBeforeUnloadAsync(is_reload, rfh->GetWeakPtr());Minoru ChikamuneWhy do some callers call `SendBeforeUnloadAsync()` but some others call `SendBeforeUnload()` with `should_run_before_unload_asynchronously` to true? What's the difference and in which case should we call which ones? Is it possible to just have one function for async beforeunload?
Alex Moshchuk`should_run_before_unload_asynchronously` is decided only once for one navigation. After the decision, always use SendBeforeUnloadAsync() when `should_run_before_unload_asynchronously` is true.
Minoru Chikamune+1 to Rakina's question here, as I'm also not sure I understand yet why we need both helpers when it seems like SendBeforeUnload can support both modes (since it takes the `should_run_before_unload_asynchronously` flag). Is it because the ack processing in SendBeforeUnloadAsync is sufficiently different that it'll make SendBeforeUnload too complex if we wanted to fold it in there?
Also, maybe it would be helpful to walk through an example like your test - how would SendBeforeUnload/SendBeforeUnloadAsync end up getting invoked in that case?
Rakina Zata AmniI've merged `SendBeforeUnloadAsync` into `SendBeforeUnload` as suggested.
In the new combined `SendBeforeUnload`, the control flow works as follows:
1. When `should_run_before_unload_asynchronously` is true, the call to `SendBeforeUnload` (via `CheckOrDispatchBeforeUnloadForFrame`) initiates the asynchronous execution using a specialized callback that notifies the `NavigationRequest` via `MaybeResumeAsyncBeforeUnloadCommit`.
2. After the dispatch loop (from `ForEachRenderFrameHostImplWithAction` and `CheckOrDispatchBeforeUnloadForFrame`), `CheckOrDispatchBeforeUnloadForSubtree` moves the frame IDs from the synchronous `beforeunload_pending_replies_` to the `NavigationRequest`'s `async_before_unload_pending_replies_`.
3. Finally, an additional call to `SendBeforeUnload` (at the end of `CheckOrDispatchBeforeUnloadForSubtree`) is made to continue the navigation process. This call uses the standard path (the `for_legacy == true` path) but acknowledges the async state, allowing the navigation to proceed while the background handlers are still running.After merging `SendBeforeUnloadAsync` into `SendBeforeUnload`, The `if (navigation_request->get_async_before_unload_pending_replies().empty())` check is required to distinguish the initial async dispatch phase from the subsequent navigation continuation.
Minoru ChikamuneThank you for the explanation! I think the hard to comprehend part is the part to continue the navigation (the if clause for `for_legacy` + now `should_run_before_unload_asynchronously`) that doesn't actually send anything to the renderer and just wants to continue the navigation (synchronously or asynchronously). That part doesn't matter at all for cases where we actually want to run the beforeunload in the renderer, and only matters for the additional call outside of the iteration that you mentioned.
Would it be possible to move that part to the caller of `SendBeforeUnload` that's outside of the iteration? We need the `before_unload_closure` but maybe we can just make that a member function? Then you also don't need the `get_async_before_unload_pending_replies()` check to differentiate the iteration vs the extra call?
Sorry for my late reply; I somehow missed your comment.
Would it be possible to move that part to the caller of SendBeforeUnload
That makes sense. I'll update the patch to reflect this.
I splitted SendBeforeUnload into SendBeforeUnload and ContinueNavigationAfterBeforeUnloadCheck. After that, I also tried to remove the code to run ContinueNavigationAfterBeforeUnloadCheck in SendBeforeUnload, but it requires test updates. I'm currently working on fixing tests.
Related details
- Alex Moshchuk
- Charlie Reis
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minoru Chikamune (Gerrit)
Minoru Chikamune added 1 comment![]( "Open in Gerrit")
I think this CL is ready to be reviewed. PTAL.
Related details
- Alex Moshchuk
- Charlie Reis
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minoru Chikamune (Gerrit)
Minoru Chikamune added 1 comment![]( "Open in Gerrit")
Now I fixed the tests (https://crrev.com/c/7616469), and I cleaned up the code related to this.
Related details
- Alex Moshchuk
- Charlie Reis
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minoru Chikamune (Gerrit)
Minoru Chikamune added 6 comments![]( "Open in Gerrit")
dialogs that users may use to cancel the navigation. If even a single
frame has been interacted with, the browser reverts to synchronousMinoru Chikamunewith a beforeunload handler
(Correct? It sounds like it might be ok to proceed if a frame on the page has a user activation but no beforeunload handler?)
Not sure how easy it is to test a case like this (and the simpler case where a frame with an activation and a handler prevents the optimization), but that might be worth considering, if it's not already in the CL.
with a beforeunload handler
Yes, correct. I updated the CL description.
Not sure how easy it is to test a case like this (and the simpler case where a frame with an activation and a handler prevents the optimization), but that might be worth considering, if it's not already in the CL.
I think the current `RenderFrameHostImplAsyncBeforeUnloadBrowserTest.AsyncExecution` covers the half of it.
A(B,C):
- B,C has beforeunload handler, A doesn't.
- A has an activation, B,C don't.
- And confirms that the beforeunload handler runs asynchronously.
I added `RenderFrameHostImplAsyncBeforeUnloadBrowserTest.SyncFallbackWithUserActivation` to confirm the case where this optimization is not triggered.
A(B,C):
- B,C has beforeunload handler, A doesn't.
- A,B has an activation, C doesn't.
Done
// too long, preventing the navigation from being hung indefinitely.Minoru ChikamuneJust curious-- do we need a separate timer for this if there's already [one in RFHI](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.h;drc=976a71ef2a509d4391d814b52b9f4700fdf35789;l=4756)? If RenderFrameHostImpl::BeforeUnloadTimeout() had a way of telling this condition to proceed, would that avoid the need for a second timer?
I mainly ask because it seems tricky to think through all the possibilities of which timer fires first, if at all. If there's a good way to think about how they relate to each other, that might be worth putting in the comment here.
Charlie ReisCurrently, I don't have a clear answer, but I agree that it's better to use one timer instead of two timers. I'll check if I can use just one timer.
Minoru ChikamuneThanks. I left another comment about this since I see we still have two timers, and I'm not sure how to reason about them.
I ended up using a separate timer, but I stopped the timer (RenderFrameHostImpl::beforeunload_timeout_) when we run AsyncBeforeUnload path to avoid having two timers running.
Done
Minoru ChikamuneAs before, can we document how this interacts with the [other beforeunload timer](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;drc=97c0a7967c365a7092885334a52c68d1c43efb91;l=11958), if we need to have both? Do we have a guarantee which one will fire first, or do we need to reason about what happens if either one fires first?
I'm still not sure I understand why we need two timers, especially if they're set around the same time and have the same duration. Maybe that can be made clearer.
Ah, sorry. I should have addressed this. I updated the code to explicitly stop RFHI::beforeunload_timeout_ before starting NavigationRequest::async_before_unload_timeout_. I hope this makes it clear which timer is responsible for each phase:
1. `RFHI::beforeunload_timeout_`: Manages the synchronous phase that blocks the navigation from starting.
2. `NavigationRequest::async_before_unload_timeout_`: Manages the asynchronous phase where handlers run in the background and only block the navigation from committing.By stopping the first timer before starting the second, we ensure they never overlap or race against each other.
Done
// See the fixme comment in `FrameLoader::ShouldClose` function: "There is not
// yet any way to dispatch events to out-of-process frames."Minoru ChikamuneOh, that comment is super-old and probably predates the OOPIF beforeunload support that we added in M63. It can probably be removed (since we do dispatch beforeunload to OOPIFs, just requiring them all to complete before kicking off the navigation), and I'm not sure it's worth mentioning here.
I see. But still this code relies on this assumption. The browser process calls the mojo API for each renderers.
e.g. For RFH tree A(B(A), C(B)), SendBeforeUnloadAsync() is called from CheckOrDispatchBeforeUnloadForFrame for A, B, C if A, B, C is hosted in a separated renderer.
Done
SendBeforeUnloadAsync(is_reload, rfh->GetWeakPtr());Minoru ChikamuneWhy do some callers call `SendBeforeUnloadAsync()` but some others call `SendBeforeUnload()` with `should_run_before_unload_asynchronously` to true? What's the difference and in which case should we call which ones? Is it possible to just have one function for async beforeunload?
Alex Moshchuk`should_run_before_unload_asynchronously` is decided only once for one navigation. After the decision, always use SendBeforeUnloadAsync() when `should_run_before_unload_asynchronously` is true.
Minoru Chikamune+1 to Rakina's question here, as I'm also not sure I understand yet why we need both helpers when it seems like SendBeforeUnload can support both modes (since it takes the `should_run_before_unload_asynchronously` flag). Is it because the ack processing in SendBeforeUnloadAsync is sufficiently different that it'll make SendBeforeUnload too complex if we wanted to fold it in there?
Also, maybe it would be helpful to walk through an example like your test - how would SendBeforeUnload/SendBeforeUnloadAsync end up getting invoked in that case?
Rakina Zata AmniI've merged `SendBeforeUnloadAsync` into `SendBeforeUnload` as suggested.
In the new combined `SendBeforeUnload`, the control flow works as follows:
1. When `should_run_before_unload_asynchronously` is true, the call to `SendBeforeUnload` (via `CheckOrDispatchBeforeUnloadForFrame`) initiates the asynchronous execution using a specialized callback that notifies the `NavigationRequest` via `MaybeResumeAsyncBeforeUnloadCommit`.
2. After the dispatch loop (from `ForEachRenderFrameHostImplWithAction` and `CheckOrDispatchBeforeUnloadForFrame`), `CheckOrDispatchBeforeUnloadForSubtree` moves the frame IDs from the synchronous `beforeunload_pending_replies_` to the `NavigationRequest`'s `async_before_unload_pending_replies_`.
3. Finally, an additional call to `SendBeforeUnload` (at the end of `CheckOrDispatchBeforeUnloadForSubtree`) is made to continue the navigation process. This call uses the standard path (the `for_legacy == true` path) but acknowledges the async state, allowing the navigation to proceed while the background handlers are still running.After merging `SendBeforeUnloadAsync` into `SendBeforeUnload`, The `if (navigation_request->get_async_before_unload_pending_replies().empty())` check is required to distinguish the initial async dispatch phase from the subsequent navigation continuation.
Minoru ChikamuneThank you for the explanation! I think the hard to comprehend part is the part to continue the navigation (the if clause for `for_legacy` + now `should_run_before_unload_asynchronously`) that doesn't actually send anything to the renderer and just wants to continue the navigation (synchronously or asynchronously). That part doesn't matter at all for cases where we actually want to run the beforeunload in the renderer, and only matters for the additional call outside of the iteration that you mentioned.
Would it be possible to move that part to the caller of `SendBeforeUnload` that's outside of the iteration? We need the `before_unload_closure` but maybe we can just make that a member function? Then you also don't need the `get_async_before_unload_pending_replies()` check to differentiate the iteration vs the extra call?
Minoru ChikamuneSorry for my late reply; I somehow missed your comment.
Would it be possible to move that part to the caller of SendBeforeUnload
That makes sense. I'll update the patch to reflect this.
Minoru ChikamuneI splitted SendBeforeUnload into SendBeforeUnload and ContinueNavigationAfterBeforeUnloadCheck. After that, I also tried to remove the code to run ContinueNavigationAfterBeforeUnloadCheck in SendBeforeUnload, but it requires test updates. I'm currently working on fixing tests.
Now I fixed the tests (https://crrev.com/c/7616469), and I cleaned up the code related to this.
Done
frame_tree_node_->navigation_request()->GetWeakPtr();Minoru ChikamuneJust trying to understand: looks like this is called from CheckOrDispatchBeforeUnloadForFrame for each frame that needs to send a beforeunload IPC to the renderer. Why are we looking up the navigation request from each frame's FTN here? Shouldn't it always be retrieved from the ancestor frame that's navigating and triggering beforeunload handlers in all of its descendants? (I.e., the equivalent of what GetBeforeUnloadInitiator() does.)
In `SendBeforeUnloadAsync`, `this` refers to the RenderFrameHost that initiated the navigation (the root of the navigating subtree), while the `rfh` parameter represents the specific frame (which could be a descendant) that needs to run the `beforeunload` handler.
Therefore, `frame_tree_node_->navigation_request()` (accessing `this->frame_tree_node_`) correctly retrieves the `NavigationRequest` associated with the navigation root, regardless of which descendant frame is being asked to run its handler. I added a comment to make this relationship clearer.
Done
Minoru Chikamune (Gerrit)
Minoru Chikamune added 1 comment![]( "Open in Gerrit")
// too long, preventing the navigation from being hung indefinitely.Minoru ChikamuneJust curious-- do we need a separate timer for this if there's already [one in RFHI](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.h;drc=976a71ef2a509d4391d814b52b9f4700fdf35789;l=4756)? If RenderFrameHostImpl::BeforeUnloadTimeout() had a way of telling this condition to proceed, would that avoid the need for a second timer?
I mainly ask because it seems tricky to think through all the possibilities of which timer fires first, if at all. If there's a good way to think about how they relate to each other, that might be worth putting in the comment here.
Charlie ReisCurrently, I don't have a clear answer, but I agree that it's better to use one timer instead of two timers. I'll check if I can use just one timer.
Minoru ChikamuneThanks. I left another comment about this since I see we still have two timers, and I'm not sure how to reason about them.
Minoru ChikamuneI ended up using a separate timer, but I stopped the timer (RenderFrameHostImpl::beforeunload_timeout_) when we run AsyncBeforeUnload path to avoid having two timers running.
Done
(reopen)
Alex Moshchuk (Gerrit)
Alex Moshchuk added 19 comments![]( "Open in Gerrit")
Thank you for your patience! I finally got to do another pass here, after our team was distracted with a reorg over the past several days.
The question about keeping replies on NavRequest vs RFH is indeed a complex one - I wrote down some considerations below, and we might want to discuss further in our upcoming CSA/Loading sync.
if (!navigation_request.IsAsyncBeforeUnloadTimerRunning() ||
navigation_request.async_before_unload_pending_replies().empty()) {Could we expose another helper on NavigationRequest to help determine if an async beforeunload is still in progress, so that code outside NavigationRequest doesn't need to know about the timer and the pending replies, which are more implementation details?
Separately, is it sufficient to only check async_before_unload_pending_replies().empty() here? Otherwise this seems to imply that the async beforeunload timer could be running while the async_before_unload_pending_replies() is empty - but that doesn't seem possible because you stop the timer after the replies become empty.
// have finished executing.nit: describe what the `acked_rfh_id` argument represents?
if (acked_rfh_id) {nit: mention that this might be nullopt when the timer expires?
// are tests and android WebView using NavigationThrottles to navigate fromminor nit: capitalize
// See the fixme comment in `FrameLoader::ShouldClose` function: "There is not
// yet any way to dispatch events to out-of-process frames."Minoru ChikamuneOh, that comment is super-old and probably predates the OOPIF beforeunload support that we added in M63. It can probably be removed (since we do dispatch beforeunload to OOPIFs, just requiring them all to complete before kicking off the navigation), and I'm not sure it's worth mentioning here.
I see. But still this code relies on this assumption. The browser process calls the mojo API for each renderers.
e.g. For RFH tree A(B(A), C(B)), SendBeforeUnloadAsync() is called from CheckOrDispatchBeforeUnloadForFrame for A, B, C if A, B, C is hosted in a separated renderer.
Agreed, and resolving since it looks like the new version of this comment doesn't mention the fixme.
node = FrameTreeNode::From(node->parent())) {It's generally more correct to walk the parent chain via RFH::GetParent() (since the FTN's current RFH might've changed - though this might not matter here), and also we've been [discouraging](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.h;l=4519-4521;drc=a1527fccf3804c7fc0f4fc4f474a260d4b248298) new uses of frame_tree_node_. So maybe it's worth changing the loop to use that and only converting to FTN when retrieving the NavigationRequest below?
On that note, one question here about another race condition: Suppose A navigates to B, and A is running unload handlers and is pending deletion. B has a beforeunload handler and starts to navigate to C, and we kick off async beforeunload for it. Before B's beforeunload ack arrives, A's unload handler finishes running and we get here to destroy A's RFH. We will look at the FTN (shared for A/B/C) and it seems that we will find the NavigationRequest for navigating B to C, and resume it potentially incorrectly. Do we need to guard against this happening? Or is this ok because A's ID won't be in async_before_unload_pending_replies_ and nothing really goes wrong in that case? Would it be possible to add a test for this?
// `this` into `beforeunload_pending_replies_` as a placeholder, letting theWhy do we need this placeholder? Also why don't we just put things directly in `async_beforeunload_pending_replies_` instead of swapping here?
Rakina Zata AmniWhy do we need this placeholder?
This is the same behavior as L12018: `beforeunload_pending_replies_.insert(this);`. It adds the frame (navigation root) as a "placeholder", and `RenderFrameHostImpl::ProcessBeforeUnloadCompletedFromFrame()` consumes it (`beforeunload_pending_replies_.erase(frame);`).
Also why don't we just put things directly in async_beforeunload_pending_replies_ instead of swapping here?
`RenderFrameHostImpl::CheckOrDispatchBeforeUnloadForFrame()` algorithm relies on `beforeunload_pending_replies_` not to send duplicated request. So, `beforeunload_pending_replies_` should be kept as is until the `ForEachRenderFrameHostImplWithAction` ends.
Initially, I didn't understand it, and actually, the beforeunload handlers run twice before in the test.
Minoru ChikamuneAh ok, hmm we do erase it from `ProcessBeforeUnloadCompletedFromFrame()` but does anything prevent us from just not adding anything at all? We don't actually check whether the RFH is in the list when erasing. But maybe it's not important, it's either confusing here or confusing there.
I'm wondering if we can just combine `beforeunload_pending_replies_` and `async_beforeunload_pending_replies_` by just adding a bool differentiating the type (async/sync) there so we can not do this swapping and placeholder and ensuring every time we want to clear `beforeunload_pending_replies_` the async ones get cleared as well.
But I guess you moved it to NavigationRequest because of my [comment](https://chromium-review.googlesource.com/c/chromium/src/+/7413835/comment/3c83d833_4b258612/) about the beforeunload being tied to the navigation instead of the RFH, which I kinda feel it should be. But having `async_beforeunload_pending_replies_` in NavigationRequest means we can trigger beforeunload multiple times for different navigations happening in different frames at the same time, but the sync version will only trigger the beforeunload once. We can't really move `beforeunload_pending_replies_` to NavigationRequest because that can also be used for frame deletion. Maybe it's fine to keep `async_beforeunload_pending_replies_` in RFH too to not differentiate the sync vs async behavior further? WDYT? I think I need @ale...@chromium.org's opinion on this too since he's more familiar with beforeunload :)
Thank you for your comment. This is important decision. I'll wait for alexmos@'s opinion.
This is a very good question, with pros and cons to both approaches and it feels like there's no ideal solution. :/
Advantages of reusing beforeunload_pending_replies_:
- No need to move the pending replies to another list.
- Centralizes beforeunload logic for both navigations and tab/frame close and minimizes differences between sync/async logic.
- Already tries to handle various races with races (e.g., navigation getting canceled/replaced while a beforeunload is running, navigation interleaving with tab close, etc.). One example: suppose we kicked off async beforeunload for a navigation, then the user initiates tab close before we receive the beforeunload ACK. Presumably we wouldn't want to execute beforeunload more than once? Do we now have to search all NavigationRequests for each frame to see if we're already running beforeunload in them, in addition to looking at RFH::beforeunload_pending_replies_ / is_waiting_for_beforeunload_completion_? And we could end up with a mix of handlers tracked differently in beforeunload_pending_replies_ vs async_beforeunload_pending_replies_?
Drawbacks of reusing beforeunload_pending_replies_ / advantanges of async_beforeunload_pending_replies_ in NavigationRequest:
- https://crbug.com/467325165 showed that beforeunload_pending_replies_ is potentially error-prone: RFHs in that list were potentially being unexpectedly freed with an experimental feature that postponed RFH destruction (the feature never shipped, though). Reusing it for async beforeunload would widen the time window during which bugs like this can happen. I like that async_beforeunload_pending_replies_ uses RFH IDs instead, and ideally we'd move beforeunload_pending_replies_ to that approach as well.
- As Rakina mentioned, keeping this on NavigationRequest "feels right" and minimizes chances of misattributing incoming beforeunload ACKs to incorrect navigations.
- beforeunload_pending_replies_ (and potentially other related beforeunload state on RFH) was not designed to be valid beyond the point at which we start the navigation and kick off DidStartNavigation, etc, so reusing it might increase chances of bugs.
In an ideal world, I feel we should have a BeforeUnloadManager, which keeps all beforeunload-related state, and which either RFH or NavigationRequest can own. But I realize that's a large refactor of the current implementation, so may not be feasible in the short term.
For the purposes of unblocking this CL - I'm going back and forth on this, but I think ultimately "async beforeunload" is a different enough concept where it makes sense to track it separately, rather than change lifetimes and expectations for all the existing RFH beforeunload state. E.g., is_waiting_for_beforeunload_completion_, has_shown_beforeunload_dialog_, and related state, all assume the sync beforeunload flow and timing, and I worry that we might get things wrong if we extend some (but not all) of that state to also work with the async flow, such as allowing dialogs after DidStartNavigation. I think this does risk potentially running beforeunload handlers multiple times in some of the race cases I mentioned, but maybe that's not that bad, and maybe beforeunload handlers should already be prepared to handle this. From this perspective, I think I'm also ok having a separate timer and ensuring it's mutually exclusive with the existing one - that might be simpler than sharing a timer and figuring out which callback path to take when it fires.
Also, to avoid moving over the replies, we could consider passing in a reference to the list of pending replies (either RFHI::beforeunload_pending_replies_ or NavRequest::async_beforeunload_pending_replies_) to CheckOrDispatchBeforeUnloadForFrame() from the start. That would probably involve converting beforeunload_pending_replies_ to use IDs as well, though, which is extra work but good cleanup (and could be done as a prerequisite).
Re: `beforeunload_pending_replies_.insert(this);`. The latest PS doesn't use `ProcessBeforeUnloadCompletedFromFrame()` for resuming async beforeunload, and instead uses `navigation_request->MaybeResumeAsyncBeforeUnloadCommit()`, which doesn't look at beforeunload_pending_replies_. So I want to check if it's still needed?
// to `SendBeforeUnload` immediately stops `beforeunload_timeout_` inContinueNavigationAfterBeforeUnloadCheck?
// handler. See description of SendBeforeUnload() for details on simulatingUpdate to ContinueNavigationAfterBeforeUnloadCheck?
// proceed on the browser process by checking thenit: I'm remove this part and instead append "in ShouldRunBeforeUnloadAsynchronously()" to the end of this sentence.
/*renderer_before_unload_end_time=*/base::TimeTicks::Now(), for_legacy);This isn't correct for the async beforeunload case, right? Is this something you plan to follow up on separately?
is_waiting_for_beforeunload_completion_ &&Where/when does this get reset for the async beforeunload flow? Would it happen as part of ProcessBeforeUnloadCompleted, similarly to the legacy beforeunload case?
IN_PROC_BROWSER_TEST_P(RenderFrameHostImplAsyncBeforeUnloadBrowserTest,Consider adding quick summaries of what each test covers, to easily determine what to expect for each one.
// Disable beforeunload timer to prevent flakiness.Should this helper turn off the new timer as well (for tests that don't expect it to run), to similarly prevent flakiness on slower bots?
dialog_manager()->Wait();After this line, could we verify that the NavigationRequest is in the proper state, i.e. that it hasn't moved past the sync beforeunload phase?
// BEFORE the browser is notified. Therefore, the async beforeunload
// optimization should not be triggered.What if we started on A(B) and did a renderer-initiated navigation to C? We would run A's beforeunload in the renderer, but B's beforeunload would be kicked off by the browser. Would that latter part be allowed to use async beforeunload or not? Could we cover that in a separate test?
// haven't interact with the frame. (See: https://crbug.com/475716933)nit: hasn't interacted
// for example, when beforeunload handlers are being run asynchronously.Maybe leave a link to crbug for more information here as well?
Related details
- Charlie Reis
- Minoru Chikamune
- Rakina Zata Amni
Minoru Chikamune (Gerrit)
Minoru Chikamune added 12 comments![]( "Open in Gerrit")
if (!navigation_request.IsAsyncBeforeUnloadTimerRunning() ||
navigation_request.async_before_unload_pending_replies().empty()) {Could we expose another helper on NavigationRequest to help determine if an async beforeunload is still in progress, so that code outside NavigationRequest doesn't need to know about the timer and the pending replies, which are more implementation details?
Separately, is it sufficient to only check async_before_unload_pending_replies().empty() here? Otherwise this seems to imply that the async beforeunload timer could be running while the async_before_unload_pending_replies() is empty - but that doesn't seem possible because you stop the timer after the replies become empty.
Agreed. Updated.
nit: describe what the `acked_rfh_id` argument represents?
Done
nit: mention that this might be nullopt when the timer expires?
Updated the code comment.
// are tests and android WebView using NavigationThrottles to navigate fromMinoru Chikamuneminor nit: capitalize
Done
Thank you for your deep insights and the detailed review. I also feel that keeping the state on `NavigationRequest` is safer and less prone to bugs. Regarding the timers, I agree that keeping them separate feels more secure as it avoids logic changes to the existing synchronous path.
The idea of passing `beforeunload_pending_replies_` to CheckOrDispatchBeforeUnloadForFrame makes a lot of sense. I’d like to explore that as a separate cleanup CL.
As for `beforeunload_pending_replies_.insert(this);`, I'd prefer to keep it as is for now. This placeholder ensures that the subsequent call to `ContinueNavigationAfterBeforeUnloadCheck` correctly triggers `ProcessBeforeUnloadCompleted{FromFrame}` and its following cleanup logic, including the critical update of the `navigationStart` timestamp. Without this, we risk missing that timestamp replacement, which could lead to artificial performance regressions in our metrics.
// to `SendBeforeUnload` immediately stops `beforeunload_timeout_` inMinoru ChikamuneContinueNavigationAfterBeforeUnloadCheck?
Done
// handler. See description of SendBeforeUnload() for details on simulatingMinoru ChikamuneUpdate to ContinueNavigationAfterBeforeUnloadCheck?
Done
nit: I'm remove this part and instead append "in ShouldRunBeforeUnloadAsynchronously()" to the end of this sentence.
Thanks! Updated.
/*renderer_before_unload_end_time=*/base::TimeTicks::Now(), for_legacy);This isn't correct for the async beforeunload case, right? Is this something you plan to follow up on separately?
Oh, you are right. But...
While not strictly accurate because the handlers are still running in the background, using `base::TimeTicks::Now()` here is intentional to avoid artificial performance regressions.
As you know, in the current synchronous path, `navigationStart` is determined by the time when the `beforeunload` handlers finish executing (which is passed as `renderer_before_unload_end_time`). In the asynchronous path, this code location—where we decide to unblock the navigation and proceed—is the closest logical equivalent to the end of that blocking phase.
If we were to use a different timestamp, it would shift the reported `navigationStart` time, which could make it appear as though the navigation is taking longer in our performance metrics, even though user-perceived performance has actually improved. I'd like to keep this as is for now to maintain consistency with existing timing calculations.
I added a code comment about it.
is_waiting_for_beforeunload_completion_ &&Where/when does this get reset for the async beforeunload flow? Would it happen as part of ProcessBeforeUnloadCompleted, similarly to the legacy beforeunload case?
Yes, it is reset as part of `ProcessBeforeUnloadCompleted`, similarly to the legacy beforeunload case.
When we decide to run handlers asynchronously in `CheckOrDispatchBeforeUnloadForSubtree`, we insert `this` RFH into `beforeunload_pending_replies_` as a placeholder and then call `ContinueNavigationAfterBeforeUnloadCheck(false)`. This method calls `ProcessBeforeUnloadCompleted`, then it calls `ProcessBeforeUnloadCompletedFromFrame`, which removes the placeholder from the pending set and clears `is_waiting_for_beforeunload_completion_`.
For AsyncBeforeUnload case, is_waiting_for_beforeunload_completion_ == false means that the synchronous 'blocking' phase has ended and can proceed navigation.
// haven't interact with the frame. (See: https://crbug.com/475716933)Minoru Chikamunenit: hasn't interacted
Thanks! Fixed.
// for example, when beforeunload handlers are being run asynchronously.Maybe leave a link to crbug for more information here as well?
- Alex Moshchuk
- Charlie Reis
- Rakina Zata Amni
Minoru Chikamune (Gerrit)
Minoru Chikamune added 2 comments![]( "Open in Gerrit")
node = FrameTreeNode::From(node->parent())) {It's generally more correct to walk the parent chain via RFH::GetParent() (since the FTN's current RFH might've changed - though this might not matter here), and also we've been [discouraging](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.h;l=4519-4521;drc=a1527fccf3804c7fc0f4fc4f474a260d4b248298) new uses of frame_tree_node_. So maybe it's worth changing the loop to use that and only converting to FTN when retrieving the NavigationRequest below?
On that note, one question here about another race condition: Suppose A navigates to B, and A is running unload handlers and is pending deletion. B has a beforeunload handler and starts to navigate to C, and we kick off async beforeunload for it. Before B's beforeunload ack arrives, A's unload handler finishes running and we get here to destroy A's RFH. We will look at the FTN (shared for A/B/C) and it seems that we will find the NavigationRequest for navigating B to C, and resume it potentially incorrectly. Do we need to guard against this happening? Or is this ok because A's ID won't be in async_before_unload_pending_replies_ and nothing really goes wrong in that case? Would it be possible to add a test for this?
So maybe it's worth changing the loop to use that and only converting to FTN when retrieving the NavigationRequest below?
Thanks! Done.
about another race condition
I added a guard condition.
Would it be possible to add a test for this?
Sorry, not yet.
The idea of passing beforeunload_pending_replies_ to CheckOrDispatchBeforeUnloadForFrame
I updated this CL.
Related details
- Alex Moshchuk
- Charlie Reis
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minoru Chikamune (Gerrit)
Minoru Chikamune added 3 comments![]( "Open in Gerrit")
IN_PROC_BROWSER_TEST_P(RenderFrameHostImplAsyncBeforeUnloadBrowserTest,Consider adding quick summaries of what each test covers, to easily determine what to expect for each one.
Done
// Disable beforeunload timer to prevent flakiness.Should this helper turn off the new timer as well (for tests that don't expect it to run), to similarly prevent flakiness on slower bots?
Thanks! I modified the following code in `RenderFrameHostImpl::CheckOrDispatchBeforeUnloadForSubtree`.
// `beforeunload_timeout_` may be null in tests (e.g., when
// DisableBeforeUnloadHangMonitorForTesting() is used). In such cases, we
// also skip starting the asynchronous beforeunload timer to maintain
// consistent behavior and avoid flakiness on slow bots.
if (beforeunload_timeout_) {
beforeunload_timeout_->Stop();
frame_tree_node_->navigation_request()->StartAsyncBeforeUnloadTimer();
}
After this line, could we verify that the NavigationRequest is in the proper state, i.e. that it hasn't moved past the sync beforeunload phase?
Done
Related details
- Alex Moshchuk
- Charlie Reis
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minoru Chikamune (Gerrit)
Minoru Chikamune added 2 comments![]( "Open in Gerrit")
node = FrameTreeNode::From(node->parent())) {Minoru ChikamuneIt's generally more correct to walk the parent chain via RFH::GetParent() (since the FTN's current RFH might've changed - though this might not matter here), and also we've been [discouraging](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.h;l=4519-4521;drc=a1527fccf3804c7fc0f4fc4f474a260d4b248298) new uses of frame_tree_node_. So maybe it's worth changing the loop to use that and only converting to FTN when retrieving the NavigationRequest below?
On that note, one question here about another race condition: Suppose A navigates to B, and A is running unload handlers and is pending deletion. B has a beforeunload handler and starts to navigate to C, and we kick off async beforeunload for it. Before B's beforeunload ack arrives, A's unload handler finishes running and we get here to destroy A's RFH. We will look at the FTN (shared for A/B/C) and it seems that we will find the NavigationRequest for navigating B to C, and resume it potentially incorrectly. Do we need to guard against this happening? Or is this ok because A's ID won't be in async_before_unload_pending_replies_ and nothing really goes wrong in that case? Would it be possible to add a test for this?
So maybe it's worth changing the loop to use that and only converting to FTN when retrieving the NavigationRequest below?
Thanks! Done.
about another race condition
I added a guard condition.
Would it be possible to add a test for this?
Sorry, not yet.
I initially added the following guard condition, but I removed it because `NavigationRequest::MaybeResumeAsyncBeforeUnloadCommit()` checks it. I added a test to confirm it. See `RaceConditionDuringOldFrameDestruction` in browsertest. But thank you for your review!
```
// If `NavigationRequest` is waiting for asynchronous beforeunload completion
// callback from this frame, try to proceed the all navigations that are
// unblocked by this frame.
for (RenderFrameHostImpl* rfh = this; rfh; rfh = rfh->GetParent()) {
if (NavigationRequest* navigation_request =
rfh->frame_tree_node()->navigation_request()) {
// A FrameTreeNode can only have one NavigationRequest at a time, but it
// might not be waiting for the RenderFrameHost being destroyed. For
// example, if frame A navigates to B, A is pending deletion. If B then
// starts a navigation to C which runs beforeunload handlers
// asynchronously, that navigation from B to C will be waiting for B's
// beforeunload ACK. When A's unload handler eventually finishes and A is
// destroyed, we must not incorrectly resume the navigation from B to C.
if (navigation_request->async_before_unload_pending_replies().contains(
GetGlobalId())) {
navigation_request->MaybeResumeAsyncBeforeUnloadCommit(GetGlobalId());
}
}
}
```
// BEFORE the browser is notified. Therefore, the async beforeunload
// optimization should not be triggered.What if we started on A(B) and did a renderer-initiated navigation to C? We would run A's beforeunload in the renderer, but B's beforeunload would be kicked off by the browser. Would that latter part be allowed to use async beforeunload or not? Could we cover that in a separate test?
Would that latter part be allowed to use async beforeunload or not?
Yes, it's allowed to use async beforeunload. I added a test for it. Thank you for pointing it out.
Related details
- Alex Moshchuk
- Charlie Reis
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minoru Chikamune (Gerrit)
Minoru Chikamune added 1 comment![]( "Open in Gerrit")
I believe I've addressed all the major review comments in this patch set. Looking forward to your next review.
Related details
- Alex Moshchuk
- Charlie Reis
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk added 13 comments![]( "Open in Gerrit")
Thanks, I think this is getting close! Just a few more minor comments below.
// When `for_legacy` is true callers should supplynit: update this comment
Ack - I think I'm ok with the how things are in the latest PS of the CL. @rak...@chromium.org, does it look acceptable to you?
// consistent behavior and avoid flakiness on slow bots.Let's explain that beforeunload_timeout_ *will* be non-null for all other cases, since it's already set by the caller of this function (DispatchBeforeUnload).
// proceed while we track the real replies asynchronously.I'm still torn about needing to insert the placeholder ID here. I don't think it's needed for correctness or for resuming the navigation, because when the posted task runs ProcessBeforeUnloadCompletedFromFrame, it would execute:
```
beforeunload_pending_replies_.erase(frame->GetGlobalId()); // No-op, already empty
if (!beforeunload_pending_replies_.empty()) { // Returns false, proceeds
return;
}
```
But I guess I'm ok with the ID being there, because (1) it matches the kForLegacy path, (2) it helps us keep an invariant that there's something in beforeunload_pending_replies_ while a navigation is blocked in the beforeunload phase (especially during the time after ContinueNavigationAfterBeforeUnloadCheck posts a task to proceed but before it runs). So given (2), I'm ok with it. But let's maybe modify the comment to explain this - while this isn't strictly necessary for resuming the navigation in ProcessBeforeUnloadCompletedFromFrame (which will let it proceed either way), we don't want to keep this set empty while the navigation is still blocked in the beforeunload phase (which violates an invariant that while is_waiting_for_beforeunload_completion_ is true, this set should have at least one frame we're waiting for).
/*renderer_before_unload_end_time=*/base::TimeTicks::Now(), for_legacy);Minoru ChikamuneThis isn't correct for the async beforeunload case, right? Is this something you plan to follow up on separately?
Oh, you are right. But...
While not strictly accurate because the handlers are still running in the background, using `base::TimeTicks::Now()` here is intentional to avoid artificial performance regressions.
As you know, in the current synchronous path, `navigationStart` is determined by the time when the `beforeunload` handlers finish executing (which is passed as `renderer_before_unload_end_time`). In the asynchronous path, this code location—where we decide to unblock the navigation and proceed—is the closest logical equivalent to the end of that blocking phase.
If we were to use a different timestamp, it would shift the reported `navigationStart` time, which could make it appear as though the navigation is taking longer in our performance metrics, even though user-perceived performance has actually improved. I'd like to keep this as is for now to maintain consistency with existing timing calculations.
I added a code comment about it.
Thanks - acknowledged.
bool proceed, base::TimeTicks renderer_before_unload_start_time,
base::TimeTicks renderer_before_unload_end_time,Minoru ChikamuneAre we going to need to process these for metrics, similarly how they're handled on the old path?
On that note, how are the NavigationRequest::MaybeRecordTraceEventsAndHistograms() trace events and metrics affected by AsyncBeforeUnload? Will they need to be updated, and if so, will that happen here or in a separate CL?
Charlie ReisAre we going to need to process these for metrics, similarly how they're handled on the old path?
I thought these TimeTicks are no longer important because they are no longer on the critical path.
On that note, how are the NavigationRequest::MaybeRecordTraceEventsAndHistograms() trace events and metrics affected by AsyncBeforeUnload? Will they need to be updated, and if so, will that happen here or in a separate CL?
I thought MaybeRecordTraceEventsAndHistograms does not need to be updated because running beforeunload does not block navigation anymore.
Minoru ChikamuneHmm, we may want to think about this a bit more.
(1) Beforeunload can still block navigation if the commit deferring condition runs, and it would probably be good to have metrics for how much of an effect that has in practice. Hopefully we don't hit it much.
(2) The beforeunload handlers are still running and could be shown in a separate (non-nested) trace event / metric if we wanted to. That's maybe less important, but might explain more about how the events overlap in a trace (which would be educational). Maybe a metric wouldn't be required for that, though, unless there's a reason we're curious about it. (It might help explain what portion of the time ended up in the commit deferring condition, for example.)
(1) Beforeunload can still block navigation if the commit deferring condition runs, and it would probably be good to have metrics for how much of an effect that has in practice. Hopefully we don't hit it much.
I see. Currently, the `commit deferring condition` duration is included in `ReceiveResponseToCommit`.
e.g. Tracelog of `RenderFrameHostImplAsyncBeforeUnloadBrowserTest.AsyncBeforeUnload`:
https://ui.perfetto.dev/#!/?s=97a4c641f9ab5ecd206ddccc3dcea9ce98c6c3b8(2) The beforeunload handlers are still running and could be shown in a separate (non-nested) trace event / metric if we wanted to. That's maybe less important, but might explain more about how the events overlap in a trace (which would be educational). Maybe a metric wouldn't be required for that, though, unless there's a reason we're curious about it. (It might help explain what portion of the time ended up in the commit deferring condition, for example.)
OK, I'll add a non-nested trace event in a follow-up CL.
Resolving, still it seems like we'll revisit this in a followup.
is_waiting_for_beforeunload_completion_ &&Minoru ChikamuneWhere/when does this get reset for the async beforeunload flow? Would it happen as part of ProcessBeforeUnloadCompleted, similarly to the legacy beforeunload case?
Yes, it is reset as part of `ProcessBeforeUnloadCompleted`, similarly to the legacy beforeunload case.
When we decide to run handlers asynchronously in `CheckOrDispatchBeforeUnloadForSubtree`, we insert `this` RFH into `beforeunload_pending_replies_` as a placeholder and then call `ContinueNavigationAfterBeforeUnloadCheck(false)`. This method calls `ProcessBeforeUnloadCompleted`, then it calls `ProcessBeforeUnloadCompletedFromFrame`, which removes the placeholder from the pending set and clears `is_waiting_for_beforeunload_completion_`.
For AsyncBeforeUnload case, is_waiting_for_beforeunload_completion_ == false means that the synchronous 'blocking' phase has ended and can proceed navigation.
Acknowledged
return;GTEST_SKIP? You could also force site isolation here via something like [this](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/back_forward_cache_basics_browsertest.cc;l=1469-1470;drc=af6927a3ff251d63093086932feb8fce181e9b91).
base::test::RunUntil([&]() { return rfh_a->IsPendingDeletion(); }));This might not be needed - I think `rfh_a->IsPendingDeletion()` should become true as part of A->B commit, so should already be true here. So maybe just `ASSERT_TRUE(rfh_a->IsPendingDeletion();` is sufficient?
// If the guard in ~RFHI was missing or broken, it would have been resumedThere is no guard in the latest version, right? We just rely on the fact that A's ID won't be in the set of pending replies? Update comment?
Related details
- Charlie Reis
- Minoru Chikamune
- Rakina Zata Amni
Minoru Chikamune (Gerrit)
Minoru Chikamune added 9 comments![]( "Open in Gerrit")
Thanks, I think this is getting close! Just a few more minor comments below.
Thanks!
// When `for_legacy` is true callers should supplyMinoru Chikamunenit: update this comment
Done
// consistent behavior and avoid flakiness on slow bots.Let's explain that beforeunload_timeout_ *will* be non-null for all other cases, since it's already set by the caller of this function (DispatchBeforeUnload).
Done
// proceed while we track the real replies asynchronously.I'm still torn about needing to insert the placeholder ID here. I don't think it's needed for correctness or for resuming the navigation, because when the posted task runs ProcessBeforeUnloadCompletedFromFrame, it would execute:
```
beforeunload_pending_replies_.erase(frame->GetGlobalId()); // No-op, already empty
if (!beforeunload_pending_replies_.empty()) { // Returns false, proceeds
return;
}
```But I guess I'm ok with the ID being there, because (1) it matches the kForLegacy path, (2) it helps us keep an invariant that there's something in beforeunload_pending_replies_ while a navigation is blocked in the beforeunload phase (especially during the time after ContinueNavigationAfterBeforeUnloadCheck posts a task to proceed but before it runs). So given (2), I'm ok with it. But let's maybe modify the comment to explain this - while this isn't strictly necessary for resuming the navigation in ProcessBeforeUnloadCompletedFromFrame (which will let it proceed either way), we don't want to keep this set empty while the navigation is still blocked in the beforeunload phase (which violates an invariant that while is_waiting_for_beforeunload_completion_ is true, this set should have at least one frame we're waiting for).
Thank you for the very thoughtful review. I agree with your reasoning. I've updated the comment.
GTEST_SKIP might be nicer here.
Done
GTEST_SKIP? You could also force site isolation here via something like [this](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/back_forward_cache_basics_browsertest.cc;l=1469-1470;drc=af6927a3ff251d63093086932feb8fce181e9b91).
Done
base::test::RunUntil([&]() { return rfh_a->IsPendingDeletion(); }));This might not be needed - I think `rfh_a->IsPendingDeletion()` should become true as part of A->B commit, so should already be true here. So maybe just `ASSERT_TRUE(rfh_a->IsPendingDeletion();` is sufficient?
Done
// If the guard in ~RFHI was missing or broken, it would have been resumedThere is no guard in the latest version, right? We just rely on the fact that A's ID won't be in the set of pending replies? Update comment?
- Alex Moshchuk
- Charlie Reis
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |