diff --git a/include/capi/cef_frame_handler_capi.h b/include/capi/cef_frame_handler_capi.h index 63cc590fa..4162d00be 100644 --- a/include/capi/cef_frame_handler_capi.h +++ b/include/capi/cef_frame_handler_capi.h @@ -33,7 +33,7 @@ // by hand. See the translator.README.txt file in the tools directory for // more information. // -// $hash=fc6fbee765ce2b649f5293c8c4b076d36014e4aa$ +// $hash=d8d06ee3d6e749d864e585fce8011d113b397220$ // #ifndef CEF_INCLUDE_CAPI_CEF_FRAME_HANDLER_CAPI_H_ @@ -76,6 +76,8 @@ extern "C" { /// objects are detached at the same time then notifications will be sent for /// any sub-frame objects before the main frame object. Commands can no longer /// be routed and will be discarded. +/// - CefFremeHadler::OnFrameDestroyed => An existing main frame or sub-frame +/// object has been destroyed. /// - cef_frame_handler_t::OnMainFrameChanged => A new main frame object has /// been assigned to the browser. This will only occur with cross-origin /// navigation or re-navigation after renderer process termination (due to @@ -85,42 +87,32 @@ extern "C" { /// - cef_frame_handler_t::OnFrameDetached => Any sub-frame objects have lost /// their connection to the renderer process. Commands can no longer be routed /// and will be discarded. +/// - CefFreameHandler::OnFrameDestroyed => Any sub-frame objects have been +/// destroyed. /// - cef_life_span_handler_t::OnBeforeClose => The browser has been destroyed. /// - cef_frame_handler_t::OnFrameDetached => The main frame object have lost /// its connection to the renderer process. Notifications will be sent for any /// sub-frame objects before the main frame object. Commands can no longer be /// routed and will be discarded. +/// - CefFreameHandler::OnFrameDestroyed => The main frame object has been +/// destroyed. /// - cef_frame_handler_t::OnMainFrameChanged => The final main frame object has /// been removed from the browser. /// -/// Cross-origin navigation and/or loading receives special handling. +/// Special handling applies for cross-origin loading on creation/navigation of +/// sub-frames, and cross-origin loading on creation of new popup browsers. A +/// temporary frame will first be created in the parent frame's renderer +/// process. This temporary frame will never attach and will be discarded after +/// the real cross-origin frame is created in the new/target renderer process. +/// The client will receive creation callbacks for the temporary frame, followed +/// by cross-origin navigation callbacks (2) for the transition from the +/// temporary frame to the real frame. The temporary frame will not receive or +/// execute commands during this transitional period (any sent commands will be +/// discarded). /// /// When the main frame navigates to a different origin the OnMainFrameChanged /// callback (2) will be executed with the old and new main frame objects. /// -/// When a new sub-frame is loaded in, or an existing sub-frame is navigated to, -/// a different origin from the parent frame, a temporary sub-frame object will -/// first be created in the parent's renderer process. That temporary sub-frame -/// will then be discarded after the real cross-origin sub-frame is created in -/// the new/target renderer process. The client will receive cross-origin -/// navigation callbacks (2) for the transition from the temporary sub-frame to -/// the real sub-frame. The temporary sub-frame will not receive or execute -/// commands during this transitional period (any sent commands will be -/// discarded). -/// -/// When a new popup browser is created in a different origin from the parent -/// browser, a temporary main frame object for the popup will first be created -/// in the parent's renderer process. That temporary main frame will then be -/// discarded after the real cross-origin main frame is created in the -/// new/target renderer process. The client will receive creation and initial -/// navigation callbacks (1) for the temporary main frame, followed by cross- -/// origin navigation callbacks (2) for the transition from the temporary main -/// frame to the real main frame. The temporary main frame may receive and -/// execute commands during this transitional period (any sent commands may be -/// executed, but the behavior is potentially undesirable since they execute in -/// the parent browser's renderer process and not the new/target renderer -/// process). -/// /// Callbacks will not be executed for placeholders that may be created during /// pre-commit navigation for sub-frames that do not yet exist in the renderer /// process. Placeholders will have cef_frame_t::get_identifier() == -4. @@ -138,17 +130,33 @@ typedef struct _cef_frame_handler_t { /// Called when a new frame is created. This will be the first notification /// that references |frame|. Any commands that require transport to the /// associated renderer process (LoadRequest, SendProcessMessage, GetSource, - /// etc.) will be queued until OnFrameAttached is called for |frame|. + /// etc.) will be queued. The queued commands will be sent before + /// OnFrameAttached or discarded before OnFrameDestroyed if the frame never + /// attaches. /// void(CEF_CALLBACK* on_frame_created)(struct _cef_frame_handler_t* self, struct _cef_browser_t* browser, struct _cef_frame_t* frame); + /// + /// Called when an existing frame is destroyed. This will be the last + /// notification that references |frame| and cef_frame_t::is_valid() will + /// return false (0) for |frame|. If called during browser destruction and + /// after cef_life_span_handler_t::on_before_close() then + /// cef_browser_t::is_valid() will return false (0) for |browser|. Any queued + /// commands that have not been sent will be discarded before this callback. + /// + void(CEF_CALLBACK* on_frame_destroyed)(struct _cef_frame_handler_t* self, + struct _cef_browser_t* browser, + struct _cef_frame_t* frame); + /// /// Called when a frame can begin routing commands to/from the associated /// renderer process. |reattached| will be true (1) if the frame was re- - /// attached after exiting the BackForwardCache. Any commands that were queued - /// have now been dispatched. + /// attached after exiting the BackForwardCache or after encountering a + /// recoverable connection error. Any queued commands will now have been + /// dispatched. This function will not be called for temporary frames created + /// during cross-origin navigation. /// void(CEF_CALLBACK* on_frame_attached)(struct _cef_frame_handler_t* self, struct _cef_browser_t* browser, @@ -156,12 +164,19 @@ typedef struct _cef_frame_handler_t { int reattached); /// - /// Called when a frame loses its connection to the renderer process and will - /// be destroyed. Any pending or future commands will be discarded and - /// cef_frame_t::is_valid() will now return false (0) for |frame|. If called - /// after cef_life_span_handler_t::on_before_close() during browser - /// destruction then cef_browser_t::is_valid() will return false (0) for - /// |browser|. + /// Called when a frame loses its connection to the renderer process. This may + /// occur when a frame is destroyed, enters the BackForwardCache, or + /// encounters a rare connection error. In the case of frame destruction this + /// call will be followed by a (potentially async) call to OnFrameDestroyed. + /// If frame destruction is occuring synchronously then + /// cef_frame_t::is_valid() will return false (0) for |frame|. If called + /// during browser destruction and after + /// cef_life_span_handler_t::on_before_close() then cef_browser_t::is_valid() + /// will return false (0) for |browser|. If, in the non-destruction case, the + /// same frame later exits the BackForwardCache or recovers from a connection + /// error then there will be a follow-up call to OnFrameAttached. This + /// function will not be called for temporary frames created during cross- + /// origin navigation. /// void(CEF_CALLBACK* on_frame_detached)(struct _cef_frame_handler_t* self, struct _cef_browser_t* browser, @@ -173,14 +188,14 @@ typedef struct _cef_frame_handler_t { /// navigation after renderer process termination (due to crashes, etc). /// |old_frame| will be NULL and |new_frame| will be non-NULL when a main /// frame is assigned to |browser| for the first time. |old_frame| will be - /// non-NULL and |new_frame| will be NULL and when a main frame is removed - /// from |browser| for the last time. Both |old_frame| and |new_frame| will be - /// non-NULL for cross-origin navigations or re-navigation after renderer - /// process termination. This function will be called after on_frame_created() - /// for |new_frame| and/or after on_frame_detached() for |old_frame|. If - /// called after cef_life_span_handler_t::on_before_close() during browser - /// destruction then cef_browser_t::is_valid() will return false (0) for - /// |browser|. + /// non-NULL and |new_frame| will be NULL when a main frame is removed from + /// |browser| for the last time. Both |old_frame| and |new_frame| will be non- + /// NULL for cross-origin navigations or re-navigation after renderer process + /// termination. This function will be called after on_frame_created() for + /// |new_frame| and/or after on_frame_destroyed() for |old_frame|. If called + /// during browser destruction and after + /// cef_life_span_handler_t::on_before_close() then cef_browser_t::is_valid() + /// will return false (0) for |browser|. /// void(CEF_CALLBACK* on_main_frame_changed)(struct _cef_frame_handler_t* self, struct _cef_browser_t* browser, diff --git a/include/cef_api_hash.h b/include/cef_api_hash.h index 48ea56b8b..b1c434f6a 100644 --- a/include/cef_api_hash.h +++ b/include/cef_api_hash.h @@ -42,13 +42,13 @@ // way that may cause binary incompatibility with other builds. The universal // hash value will change if any platform is affected whereas the platform hash // values will change only if that particular platform is affected. -#define CEF_API_HASH_UNIVERSAL "8d6d53c5732a12b4d663665e3745a93d1cfd742d" +#define CEF_API_HASH_UNIVERSAL "7ce563b3df3a227c06d331774d5fa51c8421a7f1" #if defined(OS_WIN) -#define CEF_API_HASH_PLATFORM "3593cc6344b391d992421dd985c4ebcc46d8314f" +#define CEF_API_HASH_PLATFORM "0b070263fe1c7304bd90bd7a8338fcc08410d299" #elif defined(OS_MAC) -#define CEF_API_HASH_PLATFORM "c045e75415a6abc2c29a3e1e05baea7528e2ec28" +#define CEF_API_HASH_PLATFORM "e99938b49f333ccda55ea34dfb36dd65a6b2cacd" #elif defined(OS_LINUX) -#define CEF_API_HASH_PLATFORM "b1058e8b167cfaa2f0feaccf4b4f23a813db515a" +#define CEF_API_HASH_PLATFORM "45669721b97760d8cda9ea29a5bd070d38267424" #endif #ifdef __cplusplus diff --git a/include/cef_frame_handler.h b/include/cef_frame_handler.h index ce4e0fc97..240546edf 100644 --- a/include/cef_frame_handler.h +++ b/include/cef_frame_handler.h @@ -68,6 +68,8 @@ /// objects are detached at the same time then notifications will be sent for /// any sub-frame objects before the main frame object. Commands can no longer /// be routed and will be discarded. +/// - CefFremeHadler::OnFrameDestroyed => An existing main frame or sub-frame +/// object has been destroyed. /// - CefFrameHandler::OnMainFrameChanged => A new main frame object has been /// assigned to the browser. This will only occur with cross-origin navigation /// or re-navigation after renderer process termination (due to crashes, etc). @@ -76,42 +78,32 @@ /// - CefFrameHandler::OnFrameDetached => Any sub-frame objects have lost their /// connection to the renderer process. Commands can no longer be routed and /// will be discarded. +/// - CefFreameHandler::OnFrameDestroyed => Any sub-frame objects have been +/// destroyed. /// - CefLifeSpanHandler::OnBeforeClose => The browser has been destroyed. /// - CefFrameHandler::OnFrameDetached => The main frame object have lost its /// connection to the renderer process. Notifications will be sent for any /// sub-frame objects before the main frame object. Commands can no longer be /// routed and will be discarded. +/// - CefFreameHandler::OnFrameDestroyed => The main frame object has been +/// destroyed. /// - CefFrameHandler::OnMainFrameChanged => The final main frame object has /// been removed from the browser. /// -/// Cross-origin navigation and/or loading receives special handling. +/// Special handling applies for cross-origin loading on creation/navigation of +/// sub-frames, and cross-origin loading on creation of new popup browsers. A +/// temporary frame will first be created in the parent frame's renderer +/// process. This temporary frame will never attach and will be discarded after +/// the real cross-origin frame is created in the new/target renderer process. +/// The client will receive creation callbacks for the temporary frame, followed +/// by cross-origin navigation callbacks (2) for the transition from the +/// temporary frame to the real frame. The temporary frame will not receive or +/// execute commands during this transitional period (any sent commands will be +/// discarded). /// /// When the main frame navigates to a different origin the OnMainFrameChanged /// callback (2) will be executed with the old and new main frame objects. /// -/// When a new sub-frame is loaded in, or an existing sub-frame is navigated to, -/// a different origin from the parent frame, a temporary sub-frame object will -/// first be created in the parent's renderer process. That temporary sub-frame -/// will then be discarded after the real cross-origin sub-frame is created in -/// the new/target renderer process. The client will receive cross-origin -/// navigation callbacks (2) for the transition from the temporary sub-frame to -/// the real sub-frame. The temporary sub-frame will not receive or execute -/// commands during this transitional period (any sent commands will be -/// discarded). -/// -/// When a new popup browser is created in a different origin from the parent -/// browser, a temporary main frame object for the popup will first be created -/// in the parent's renderer process. That temporary main frame will then be -/// discarded after the real cross-origin main frame is created in the -/// new/target renderer process. The client will receive creation and initial -/// navigation callbacks (1) for the temporary main frame, followed by -/// cross-origin navigation callbacks (2) for the transition from the temporary -/// main frame to the real main frame. The temporary main frame may receive and -/// execute commands during this transitional period (any sent commands may be -/// executed, but the behavior is potentially undesirable since they execute in -/// the parent browser's renderer process and not the new/target renderer -/// process). -/// /// Callbacks will not be executed for placeholders that may be created during /// pre-commit navigation for sub-frames that do not yet exist in the renderer /// process. Placeholders will have CefFrame::GetIdentifier() == -4. @@ -126,17 +118,33 @@ class CefFrameHandler : public virtual CefBaseRefCounted { /// Called when a new frame is created. This will be the first notification /// that references |frame|. Any commands that require transport to the /// associated renderer process (LoadRequest, SendProcessMessage, GetSource, - /// etc.) will be queued until OnFrameAttached is called for |frame|. + /// etc.) will be queued. The queued commands will be sent before + /// OnFrameAttached or discarded before OnFrameDestroyed if the frame never + /// attaches. /// /*--cef()--*/ virtual void OnFrameCreated(CefRefPtr browser, CefRefPtr frame) {} + /// + /// Called when an existing frame is destroyed. This will be the last + /// notification that references |frame| and CefFrame::IsValid() will return + /// false for |frame|. If called during browser destruction and after + /// CefLifeSpanHandler::OnBeforeClose() then CefBrowser::IsValid() will return + /// false for |browser|. Any queued commands that have not been sent will be + /// discarded before this callback. + /// + /*--cef()--*/ + virtual void OnFrameDestroyed(CefRefPtr browser, + CefRefPtr frame) {} + /// /// Called when a frame can begin routing commands to/from the associated /// renderer process. |reattached| will be true if the frame was re-attached - /// after exiting the BackForwardCache. Any commands that were queued have now - /// been dispatched. + /// after exiting the BackForwardCache or after encountering a recoverable + /// connection error. Any queued commands will now have been dispatched. This + /// method will not be called for temporary frames created during cross-origin + /// navigation. /// /*--cef()--*/ virtual void OnFrameAttached(CefRefPtr browser, @@ -144,11 +152,17 @@ class CefFrameHandler : public virtual CefBaseRefCounted { bool reattached) {} /// - /// Called when a frame loses its connection to the renderer process and will - /// be destroyed. Any pending or future commands will be discarded and - /// CefFrame::IsValid() will now return false for |frame|. If called after - /// CefLifeSpanHandler::OnBeforeClose() during browser destruction then - /// CefBrowser::IsValid() will return false for |browser|. + /// Called when a frame loses its connection to the renderer process. This may + /// occur when a frame is destroyed, enters the BackForwardCache, or + /// encounters a rare connection error. In the case of frame destruction this + /// call will be followed by a (potentially async) call to OnFrameDestroyed. + /// If frame destruction is occuring synchronously then CefFrame::IsValid() + /// will return false for |frame|. If called during browser destruction and + /// after CefLifeSpanHandler::OnBeforeClose() then CefBrowser::IsValid() will + /// return false for |browser|. If, in the non-destruction case, the same + /// frame later exits the BackForwardCache or recovers from a connection error + /// then there will be a follow-up call to OnFrameAttached. This method will + /// not be called for temporary frames created during cross-origin navigation. /// /*--cef()--*/ virtual void OnFrameDetached(CefRefPtr browser, @@ -160,13 +174,13 @@ class CefFrameHandler : public virtual CefBaseRefCounted { /// re-navigation after renderer process termination (due to crashes, etc). /// |old_frame| will be NULL and |new_frame| will be non-NULL when a main /// frame is assigned to |browser| for the first time. |old_frame| will be - /// non-NULL and |new_frame| will be NULL and when a main frame is removed - /// from |browser| for the last time. Both |old_frame| and |new_frame| will be + /// non-NULL and |new_frame| will be NULL when a main frame is removed from + /// |browser| for the last time. Both |old_frame| and |new_frame| will be /// non-NULL for cross-origin navigations or re-navigation after renderer /// process termination. This method will be called after OnFrameCreated() for - /// |new_frame| and/or after OnFrameDetached() for |old_frame|. If called - /// after CefLifeSpanHandler::OnBeforeClose() during browser destruction then - /// CefBrowser::IsValid() will return false for |browser|. + /// |new_frame| and/or after OnFrameDestroyed() for |old_frame|. If called + /// during browser destruction and after CefLifeSpanHandler::OnBeforeClose() + /// then CefBrowser::IsValid() will return false for |browser|. /// /*--cef(optional_param=old_frame,optional_param=new_frame)--*/ virtual void OnMainFrameChanged(CefRefPtr browser, diff --git a/libcef/browser/browser_info.cc b/libcef/browser/browser_info.cc index a1e7c551e..f19cc21aa 100644 --- a/libcef/browser/browser_info.cc +++ b/libcef/browser/browser_info.cc @@ -131,6 +131,9 @@ void CefBrowserInfo::MaybeCreateFrame(content::RenderFrameHost* host) { return; } + DVLOG(1) << __func__ << ": " + << frame_util::GetFrameDebugString(host->GetGlobalFrameToken()); + const auto global_id = host->GetGlobalId(); const bool is_main_frame = (host->GetParent() == nullptr); @@ -160,7 +163,7 @@ void CefBrowserInfo::MaybeCreateFrame(content::RenderFrameHost* host) { #endif // Update the associated RFH, which may have changed. - info->frame_->MaybeReAttach(this, host, /*require_detached=*/false); + info->frame_->MaybeAttach(this, host); if (info->is_speculative_ && !is_speculative) { // Upgrade the frame info from speculative to non-speculative. @@ -208,17 +211,20 @@ void CefBrowserInfo::FrameHostStateChanged( content::RenderFrameHost::LifecycleState new_state) { CEF_REQUIRE_UIT(); + DVLOG(1) << __func__ << ": " + << frame_util::GetFrameDebugString(host->GetGlobalFrameToken()); + if ((old_state == content::RenderFrameHost::LifecycleState::kPrerendering || old_state == content::RenderFrameHost::LifecycleState::kInBackForwardCache) && new_state == content::RenderFrameHost::LifecycleState::kActive) { if (auto frame = GetFrameForHost(host)) { // Update the associated RFH, which may have changed. - frame->MaybeReAttach(this, host, /*require_detached=*/true); + frame->MaybeAttach(this, host); if (frame->IsMain()) { - // Update the main frame object. NotificationStateLock lock_scope(this); + // Update the main frame object. SetMainFrame(browser_, frame); } @@ -251,6 +257,9 @@ void CefBrowserInfo::FrameHostStateChanged( void CefBrowserInfo::RemoveFrame(content::RenderFrameHost* host) { CEF_REQUIRE_UIT(); + DVLOG(1) << __func__ << ": " + << frame_util::GetFrameDebugString(host->GetGlobalFrameToken()); + NotificationStateLock lock_scope(this); const auto global_id = host->GetGlobalId(); @@ -281,12 +290,17 @@ void CefBrowserInfo::RemoveFrame(content::RenderFrameHost* host) { const auto& other_frame_info = *it2; if (other_frame_info->frame_) { const bool is_current_main_frame = other_frame_info->IsCurrentMainFrame(); - if (other_frame_info->frame_->Detach( + const auto [frame_detached, frame_destroyed] = + other_frame_info->frame_->Detach( CefFrameHostImpl::DetachReason::RENDER_FRAME_DELETED, - is_current_main_frame)) { - DCHECK(!is_current_main_frame); + is_current_main_frame); + if (frame_detached) { MaybeNotifyFrameDetached(browser_, other_frame_info->frame_); } + if (frame_destroyed) { + DCHECK(!is_current_main_frame); + MaybeNotifyFrameDestroyed(browser_, other_frame_info->frame_); + } } frame_info_set_.erase(it2); @@ -477,13 +491,21 @@ void CefBrowserInfo::SetMainFrame(CefRefPtr browser, return; } + DVLOG(1) << __func__ << ": " + << (frame ? frame->GetIdentifier().ToString() : "null"); + CefRefPtr old_frame; if (main_frame_) { old_frame = main_frame_; - if (old_frame->Detach(CefFrameHostImpl::DetachReason::NEW_MAIN_FRAME, - /*is_current_main_frame=*/false)) { + const auto [frame_detached, frame_destroyed] = + old_frame->Detach(CefFrameHostImpl::DetachReason::NEW_MAIN_FRAME, + /*is_current_main_frame=*/false); + if (frame_detached) { MaybeNotifyFrameDetached(browser, old_frame); } + if (frame_destroyed) { + MaybeNotifyFrameDestroyed(browser, old_frame); + } } main_frame_ = frame; @@ -526,6 +548,24 @@ void CefBrowserInfo::MaybeNotifyFrameDetached( browser, frame)); } +// Passing in |browser| here because |browser_| may already be cleared. +void CefBrowserInfo::MaybeNotifyFrameDestroyed( + CefRefPtr browser, + CefRefPtr frame) { + CEF_REQUIRE_UIT(); + + // Never notify for temporary objects. + DCHECK(!frame->is_temporary()); + + MaybeExecuteFrameNotification(base::BindOnce( + [](CefRefPtr browser, + CefRefPtr frame, + CefRefPtr handler) { + handler->OnFrameDestroyed(browser, frame); + }, + browser, frame)); +} + // Passing in |browser| here because |browser_| may already be cleared. void CefBrowserInfo::MaybeNotifyMainFrameChanged( CefRefPtr browser, @@ -563,13 +603,13 @@ void CefBrowserInfo::RemoveAllFrames( // Explicitly Detach everything. for (auto& info : frame_info_set_) { if (info->frame_) { - const bool is_current_main_frame = info->IsCurrentMainFrame(); - if (info->frame_->Detach( + [[maybe_unused]] const auto [frame_detached, frame_destroyed] = + info->frame_->Detach( CefFrameHostImpl::DetachReason::BROWSER_DESTROYED, - is_current_main_frame)) { - DCHECK(!is_current_main_frame); - MaybeNotifyFrameDetached(old_browser, info->frame_); - } + info->IsCurrentMainFrame()); + // Shouldn't need to trigger any notifications at this point. + DCHECK(!frame_detached); + DCHECK(!frame_destroyed); } } diff --git a/libcef/browser/browser_info.h b/libcef/browser/browser_info.h index c42c4cb2b..50f658e39 100644 --- a/libcef/browser/browser_info.h +++ b/libcef/browser/browser_info.h @@ -166,6 +166,8 @@ class CefBrowserInfo : public base::RefCountedThreadSafe { CefRefPtr browser, CefRefPtr frame, std::vector draggable_regions); + void MaybeNotifyFrameDetached(CefRefPtr browser, + CefRefPtr frame); private: friend class base::RefCountedThreadSafe; @@ -190,8 +192,8 @@ class CefBrowserInfo : public base::RefCountedThreadSafe { CefRefPtr frame); void MaybeNotifyFrameCreated(CefRefPtr frame); - void MaybeNotifyFrameDetached(CefRefPtr browser, - CefRefPtr frame); + void MaybeNotifyFrameDestroyed(CefRefPtr browser, + CefRefPtr frame); void MaybeNotifyMainFrameChanged(CefRefPtr browser, CefRefPtr old_frame, CefRefPtr new_frame); diff --git a/libcef/browser/frame_host_impl.cc b/libcef/browser/frame_host_impl.cc index a755aaf6e..e68afacdc 100644 --- a/libcef/browser/frame_host_impl.cc +++ b/libcef/browser/frame_host_impl.cc @@ -500,35 +500,18 @@ bool CefFrameHostImpl::IsDetached() const { return !GetRenderFrameHost(); } -bool CefFrameHostImpl::Detach(DetachReason reason, bool is_current_main_frame) { +std::pair CefFrameHostImpl::Detach(DetachReason reason, + bool is_current_main_frame) { CEF_REQUIRE_UIT(); - // This method may be called multiple times (e.g. from CefBrowserInfo - // SetMainFrame and RemoveFrame). - bool is_first_complete_detach = false; - // Should not be called for temporary frames. CHECK(!is_temporary()); // Must be a main frame if |is_current_main_frame| is true. CHECK(!is_current_main_frame || is_main_frame_); - if (!is_current_main_frame) { - { - base::AutoLock lock_scope(state_lock_); - if (browser_info_) { - is_first_complete_detach = true; - browser_info_ = nullptr; - } - } - - // In case we never attached, clean up. - while (!queued_renderer_actions_.empty()) { - queued_renderer_actions_.pop(); - } - } - - if (render_frame_.is_bound()) { + const bool is_bound = render_frame_.is_bound(); + if (is_bound) { if (VLOG_IS_ON(1)) { std::string reason_str; switch (reason) { @@ -554,7 +537,27 @@ bool CefFrameHostImpl::Detach(DetachReason reason, bool is_current_main_frame) { render_frame_host_ = nullptr; } - return is_first_complete_detach; + // This method may be called multiple times (e.g. from CefBrowserInfo + // SetMainFrame and RemoveFrame). + bool is_first_complete_detach = false; + + if (!is_current_main_frame) { + { + base::AutoLock lock_scope(state_lock_); + if (browser_info_) { + DVLOG(1) << __func__ << ": " << GetDebugString() << " invalidated"; + is_first_complete_detach = true; + browser_info_ = nullptr; + } + } + + // In case we never attached, clean up. + while (!queued_renderer_actions_.empty()) { + queued_renderer_actions_.pop(); + } + } + + return std::make_pair(is_bound, is_first_complete_detach); } void CefFrameHostImpl::DetachRenderFrame() { @@ -564,12 +567,11 @@ void CefFrameHostImpl::DetachRenderFrame() { static_cast(frame_util::ResetReason::kDetached), "Detached"); } -void CefFrameHostImpl::MaybeReAttach( +void CefFrameHostImpl::MaybeAttach( scoped_refptr browser_info, - content::RenderFrameHost* render_frame_host, - bool require_detached) { + content::RenderFrameHost* render_frame_host) { CEF_REQUIRE_UIT(); - if (render_frame_.is_bound() && render_frame_host_ == render_frame_host) { + if (render_frame_host_ == render_frame_host) { // Nothing to do here. return; } @@ -577,16 +579,13 @@ void CefFrameHostImpl::MaybeReAttach( // Should not be called for temporary frames. CHECK(!is_temporary()); - // If |require_detached| then we expect that Detach() was called previously. - CHECK(!require_detached || !render_frame_.is_bound()); + // We expect that either this frame has never attached (e.g. when swapping + // from speculative to non-speculative) or Detach() was called previously + // (e.g. when exiting the bfcache). + CHECK(!render_frame_.is_bound()); - if (render_frame_.is_bound()) { - // Intentionally not clearing |queued_renderer_actions_|, as we may be - // changing RFH during initial browser navigation. - DVLOG(1) << __func__ << ": " << GetDebugString() - << " detached (reason=RENDER_FRAME_CHANGED)"; - DetachRenderFrame(); - } + // Intentionally not clearing |queued_renderer_actions_|, as we may be + // changing RFH during initial browser navigation. // The RFH may change but the frame token should remain the same. CHECK(*frame_token_ == render_frame_host->GetGlobalFrameToken()); @@ -653,6 +652,14 @@ void CefFrameHostImpl::SendToRenderFrame(const std::string& function_name, void CefFrameHostImpl::OnRenderFrameDisconnect() { CEF_REQUIRE_UIT(); + DVLOG(1) << __func__ << ": " << GetDebugString(); + + if (auto browser_info = GetBrowserInfo()) { + if (auto browser = browser_info->browser()) { + browser_info->MaybeNotifyFrameDetached(browser, this); + } + } + // Reconnect, if any, will be triggered via FrameAttached(). render_frame_.reset(); } diff --git a/libcef/browser/frame_host_impl.h b/libcef/browser/frame_host_impl.h index 86993f1cd..8f10ec46b 100644 --- a/libcef/browser/frame_host_impl.h +++ b/libcef/browser/frame_host_impl.h @@ -10,6 +10,7 @@ #include #include #include +#include #include "base/memory/raw_ptr.h" #include "base/synchronization/lock.h" @@ -140,15 +141,15 @@ class CefFrameHostImpl : public CefFrame, public cef::mojom::BrowserFrame { // implicitly via CefBrowserInfo::browser() returning nullptr. If // |is_current_main_frame| is true then only the RenderFrameHost references // will be released as we want the frame object itself to remain valid. - // Returns true if the frame is completely detached for the first time. - bool Detach(DetachReason reason, bool is_current_main_frame); + // Returns (bool, bool) to indicate if frame detached and/or frame destroyed + // notifications should be triggered respectively. + std::pair Detach(DetachReason reason, bool is_current_main_frame); - // A frame has swapped to active status from prerendering or the back-forward - // cache. We may need to re-attach if the RFH has changed. See - // https://crbug.com/1179502#c8 for additional background. - void MaybeReAttach(scoped_refptr browser_info, - content::RenderFrameHost* render_frame_host, - bool require_detached); + // A new frame was created or a frame has swapped to active status from + // prerendering or the back-forward cache. Update internal state if the RFH + // has changed. See https://crbug.com/1179502#c8 for additional background. + void MaybeAttach(scoped_refptr browser_info, + content::RenderFrameHost* render_frame_host); // cef::mojom::BrowserFrame methods forwarded from CefBrowserFrame. void SendMessage(const std::string& name, diff --git a/libcef/browser/frame_service_base.h b/libcef/browser/frame_service_base.h index b1cc5b611..28ae04a84 100644 --- a/libcef/browser/frame_service_base.h +++ b/libcef/browser/frame_service_base.h @@ -69,7 +69,10 @@ class CefFrameServiceBase : public Interface, DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); if (render_frame_host == render_frame_host_) { - DVLOG(1) << __func__ << ": RenderFrameHost destroyed."; + DVLOG(1) << __func__ << ": " + << frame_util::GetFrameDebugString( + render_frame_host->GetGlobalFrameToken()) + << " destroyed"; if (receiver_.is_bound()) { receiver_.ResetWithReason( static_cast(frame_util::ResetReason::kDeleted), diff --git a/libcef/renderer/frame_impl.cc b/libcef/renderer/frame_impl.cc index fb3eb1b2c..319205023 100644 --- a/libcef/renderer/frame_impl.cc +++ b/libcef/renderer/frame_impl.cc @@ -316,20 +316,23 @@ void CefFrameImpl::SendProcessMessage(CefProcessId target_process, } } -void CefFrameImpl::OnAttached() { - // Called indirectly from RenderFrameCreated. - ConnectBrowserFrame(ConnectReason::RENDER_FRAME_CREATED); -} - void CefFrameImpl::OnWasShown() { - if (browser_connection_state_ == ConnectionState::DISCONNECTED) { - // Reconnect a frame that has exited the bfcache. + if (browser_connection_state_ == ConnectionState::DISCONNECTED && + did_commit_provisional_load_) { + // Reconnect a frame that has exited the bfcache. We ignore temporary + // frames that have never called DidCommitProvisionalLoad. ConnectBrowserFrame(ConnectReason::WAS_SHOWN); } } void CefFrameImpl::OnDidCommitProvisionalLoad() { did_commit_provisional_load_ = true; + if (browser_connection_state_ == ConnectionState::DISCONNECTED) { + // Connect after RenderFrameImpl::DidCommitNavigation has potentially + // reset the BrowserInterfaceBroker in the browser process. See related + // comments in OnDisconnect. + ConnectBrowserFrame(ConnectReason::DID_COMMIT); + } MaybeInitializeScriptContext(); } @@ -473,8 +476,8 @@ void CefFrameImpl::ConnectBrowserFrame(ConnectReason reason) { if (VLOG_IS_ON(1)) { std::string reason_str; switch (reason) { - case ConnectReason::RENDER_FRAME_CREATED: - reason_str = "RENDER_FRAME_CREATED"; + case ConnectReason::DID_COMMIT: + reason_str = "DID_COMMIT"; break; case ConnectReason::WAS_SHOWN: reason_str = "WAS_SHOWN"; diff --git a/libcef/renderer/frame_impl.h b/libcef/renderer/frame_impl.h index 41b888c47..bc2b8b463 100644 --- a/libcef/renderer/frame_impl.h +++ b/libcef/renderer/frame_impl.h @@ -81,7 +81,6 @@ class CefFrameImpl CefRefPtr message) override; // Forwarded from CefRenderFrameObserver. - void OnAttached(); void OnWasShown(); void OnDidCommitProvisionalLoad(); void OnDidFinishLoad(); @@ -105,7 +104,7 @@ class CefFrameImpl LocalFrameAction action); enum class ConnectReason { - RENDER_FRAME_CREATED, + DID_COMMIT, WAS_SHOWN, RETRY, }; diff --git a/libcef/renderer/render_frame_observer.cc b/libcef/renderer/render_frame_observer.cc index 1a88699f9..edbc48888 100644 --- a/libcef/renderer/render_frame_observer.cc +++ b/libcef/renderer/render_frame_observer.cc @@ -199,7 +199,6 @@ void CefRenderFrameObserver::AttachFrame(CefFrameImpl* frame) { DCHECK(frame); DCHECK(!frame_); frame_ = frame; - frame_->OnAttached(); } void CefRenderFrameObserver::OnLoadStart() { diff --git a/libcef_dll/cpptoc/frame_handler_cpptoc.cc b/libcef_dll/cpptoc/frame_handler_cpptoc.cc index db2e936d7..8f61790b1 100644 --- a/libcef_dll/cpptoc/frame_handler_cpptoc.cc +++ b/libcef_dll/cpptoc/frame_handler_cpptoc.cc @@ -9,7 +9,7 @@ // implementations. See the translator.README.txt file in the tools directory // for more information. // -// $hash=4a3d33abbaa00a373ea515338ed67d96708dbb9c$ +// $hash=51da21d569dd41e38cb2dc6e0f2dea0bd88dbdce$ // #include "libcef_dll/cpptoc/frame_handler_cpptoc.h" @@ -50,6 +50,34 @@ frame_handler_on_frame_created(struct _cef_frame_handler_t* self, CefBrowserCToCpp::Wrap(browser), CefFrameCToCpp::Wrap(frame)); } +void CEF_CALLBACK +frame_handler_on_frame_destroyed(struct _cef_frame_handler_t* self, + cef_browser_t* browser, + cef_frame_t* frame) { + shutdown_checker::AssertNotShutdown(); + + // AUTO-GENERATED CONTENT - DELETE THIS COMMENT BEFORE MODIFYING + + DCHECK(self); + if (!self) { + return; + } + // Verify param: browser; type: refptr_diff + DCHECK(browser); + if (!browser) { + return; + } + // Verify param: frame; type: refptr_diff + DCHECK(frame); + if (!frame) { + return; + } + + // Execute + CefFrameHandlerCppToC::Get(self)->OnFrameDestroyed( + CefBrowserCToCpp::Wrap(browser), CefFrameCToCpp::Wrap(frame)); +} + void CEF_CALLBACK frame_handler_on_frame_attached(struct _cef_frame_handler_t* self, cef_browser_t* browser, @@ -140,6 +168,7 @@ frame_handler_on_main_frame_changed(struct _cef_frame_handler_t* self, CefFrameHandlerCppToC::CefFrameHandlerCppToC() { GetStruct()->on_frame_created = frame_handler_on_frame_created; + GetStruct()->on_frame_destroyed = frame_handler_on_frame_destroyed; GetStruct()->on_frame_attached = frame_handler_on_frame_attached; GetStruct()->on_frame_detached = frame_handler_on_frame_detached; GetStruct()->on_main_frame_changed = frame_handler_on_main_frame_changed; diff --git a/libcef_dll/ctocpp/frame_handler_ctocpp.cc b/libcef_dll/ctocpp/frame_handler_ctocpp.cc index 020475719..3e43dcbf0 100644 --- a/libcef_dll/ctocpp/frame_handler_ctocpp.cc +++ b/libcef_dll/ctocpp/frame_handler_ctocpp.cc @@ -9,7 +9,7 @@ // implementations. See the translator.README.txt file in the tools directory // for more information. // -// $hash=0074492ed580ccc06962a05b6c72bdabae182a51$ +// $hash=14e4a39489488582d7965ae71ed1ef174a4f3b08$ // #include "libcef_dll/ctocpp/frame_handler_ctocpp.h" @@ -48,6 +48,34 @@ void CefFrameHandlerCToCpp::OnFrameCreated(CefRefPtr browser, CefFrameCppToC::Wrap(frame)); } +NO_SANITIZE("cfi-icall") +void CefFrameHandlerCToCpp::OnFrameDestroyed(CefRefPtr browser, + CefRefPtr frame) { + shutdown_checker::AssertNotShutdown(); + + cef_frame_handler_t* _struct = GetStruct(); + if (CEF_MEMBER_MISSING(_struct, on_frame_destroyed)) { + return; + } + + // AUTO-GENERATED CONTENT - DELETE THIS COMMENT BEFORE MODIFYING + + // Verify param: browser; type: refptr_diff + DCHECK(browser.get()); + if (!browser.get()) { + return; + } + // Verify param: frame; type: refptr_diff + DCHECK(frame.get()); + if (!frame.get()) { + return; + } + + // Execute + _struct->on_frame_destroyed(_struct, CefBrowserCppToC::Wrap(browser), + CefFrameCppToC::Wrap(frame)); +} + NO_SANITIZE("cfi-icall") void CefFrameHandlerCToCpp::OnFrameAttached(CefRefPtr browser, CefRefPtr frame, diff --git a/libcef_dll/ctocpp/frame_handler_ctocpp.h b/libcef_dll/ctocpp/frame_handler_ctocpp.h index 3a0459cc1..ebeb27cb8 100644 --- a/libcef_dll/ctocpp/frame_handler_ctocpp.h +++ b/libcef_dll/ctocpp/frame_handler_ctocpp.h @@ -9,7 +9,7 @@ // implementations. See the translator.README.txt file in the tools directory // for more information. // -// $hash=a571fa8b3c173d78cfb67eb3e44c8f2c3fb2e089$ +// $hash=f33fedc6d7e0d692b03fe7f35319e93c5f31b9b1$ // #ifndef CEF_LIBCEF_DLL_CTOCPP_FRAME_HANDLER_CTOCPP_H_ @@ -36,6 +36,8 @@ class CefFrameHandlerCToCpp : public CefCToCppRefCounted browser, CefRefPtr frame) override; + void OnFrameDestroyed(CefRefPtr browser, + CefRefPtr frame) override; void OnFrameAttached(CefRefPtr browser, CefRefPtr frame, bool reattached) override; diff --git a/tests/ceftests/frame_handler_unittest.cc b/tests/ceftests/frame_handler_unittest.cc index f4b4dbc50..f977916c8 100644 --- a/tests/ceftests/frame_handler_unittest.cc +++ b/tests/ceftests/frame_handler_unittest.cc @@ -6,6 +6,7 @@ #include #include #include +#include #include #include @@ -16,7 +17,8 @@ #include "tests/ceftests/test_util.h" #include "tests/gtest/include/gtest/gtest.h" -// Set to 1 to enable verbose debugging info logging. +// Set to 1 and add `--enable-logging --vmodule=*frame*=1 --log-file=` to +// the command-line to enable verbose debugging info logging. #define VERBOSE_DEBUGGING 0 namespace { @@ -33,8 +35,9 @@ struct FrameStatus { MAIN_FRAME_CHANGED_ASSIGNED, LOAD_START, LOAD_END, - BEFORE_CLOSE, FRAME_DETACHED, + BEFORE_CLOSE, + FRAME_DESTROYED, MAIN_FRAME_CHANGED_REMOVED, MAIN_FRAME_FINAL_REMOVED, @@ -57,10 +60,12 @@ struct FrameStatus { return "OnLoadStart"; case LOAD_END: return "OnLoadEnd"; - case BEFORE_CLOSE: - return "OnBeforeClose"; case FRAME_DETACHED: return "OnFrameDetached"; + case BEFORE_CLOSE: + return "OnBeforeClose"; + case FRAME_DESTROYED: + return "OnFrameDestroyed"; case MAIN_FRAME_CHANGED_REMOVED: return "OnMainFrameChanged(changed_removed)"; case MAIN_FRAME_FINAL_REMOVED: @@ -133,7 +138,9 @@ struct FrameStatus { #endif return got_callback_[LOAD_END]; } - bool IsDetached() const { return got_callback_[FRAME_DETACHED]; } + bool IsDestroyed() const { return got_callback_[FRAME_DESTROYED]; } + + bool IsMain() const { return is_main_; } void SetIsFirstMain(bool val) { EXPECT_TRUE(is_main_); @@ -148,17 +155,24 @@ struct FrameStatus { is_last_main_ = val; } - void SetIsTemporary(bool val) { - EXPECT_FALSE(is_main_); - is_temporary_ = val; - } + void SetIsTemporary(bool val) { is_temporary_ = val; } bool IsTemporary() const { return is_temporary_; } void SetAdditionalDebugInfo(const std::string& debug_info) { debug_info_ = debug_info; } - std::string GetDebugString() const { return debug_info_ + ident_str_; } + std::string GetDebugString(bool dump_state = false) const { + std::string result = debug_info_ + ident_str_; + if (dump_state) { + std::stringstream ss; + ss << "\nis_main=" << is_main_ << "\nis_first_main=" << is_first_main_ + << "\nis_last_main=" << is_last_main_ + << "\nis_temporary=" << is_temporary_; + result += ss.str(); + } + return result; + } // The main frame will be reused for same-origin navigations. void ResetMainLoadStatus() { @@ -200,12 +214,34 @@ struct FrameStatus { CefRefPtr frame) { EXPECT_UI_THREAD(); VerifyBrowser(__FUNCTION__, browser); - // A frame is never valid after it's detached. - VerifyFrame(__FUNCTION__, frame, /*expect_valid=*/false); + + // Don't check IsValid() here for sub-frames. Invalidation will occur during + // destruction and we may become detached either before or during + // destruction, so the value is not guaranteed. + std::optional expect_valid; + + if (frame->IsMain()) { + // Check IsValid() here for main frames. It will only return true for the + // current main frame. + expect_valid = + frame->GetIdentifier() == browser->GetMainFrame()->GetIdentifier(); + } + + VerifyFrame(__FUNCTION__, frame, expect_valid); GotCallback(__FUNCTION__, FRAME_DETACHED); } + void OnFrameDestroyed(CefRefPtr browser, + CefRefPtr frame) { + EXPECT_UI_THREAD(); + VerifyBrowser(__FUNCTION__, browser); + // A frame is never valid after it's destroyed. + VerifyFrame(__FUNCTION__, frame, /*expect_valid=*/false); + + GotCallback(__FUNCTION__, FRAME_DESTROYED); + } + void OnMainFrameChanged(CefRefPtr browser, CefRefPtr old_frame, CefRefPtr new_frame) { @@ -257,7 +293,7 @@ struct FrameStatus { // Called for all existing frames, not just the target frame. // We need to track this status to know if the browser should be valid in - // following calls to OnFrameDetached. + // following calls to OnFrameDetached/OnFrameDestroyed. void OnBeforeClose(CefRefPtr browser) { EXPECT_UI_THREAD(); VerifyBrowser(__FUNCTION__, browser); @@ -325,7 +361,6 @@ struct FrameStatus { if (is_temporary_) { // Should not receive any queries. - EXPECT_FALSE(is_main_); EXPECT_EQ(0, delivered_query_ct_); } else { // Verify that all expected messages have been sent and received. @@ -357,6 +392,14 @@ struct FrameStatus { return false; } + if (is_temporary_) { + // Temporary frames should not connect or load. + if (callback == FRAME_ATTACHED || callback == FRAME_DETACHED || + callback == LOAD_START || callback == LOAD_END) { + return false; + } + } + if (is_main_) { if ((callback == MAIN_FRAME_INITIAL_ASSIGNED || callback == AFTER_CREATED) && @@ -373,44 +416,43 @@ struct FrameStatus { if (callback == MAIN_FRAME_CHANGED_REMOVED && is_last_main_) { return false; } - } else if (is_temporary_) { - // For cross-process sub-frame navigation a sub-frame is first created in - // the parent's renderer process. That sub-frame is then discarded after - // the real cross-origin sub-frame is created in a different renderer - // process. These discarded sub-frames will get OnFrameCreated/ - // OnFrameAttached immediately followed by OnFrameDetached. - return callback == FRAME_CREATED || callback == FRAME_ATTACHED || - callback == FRAME_DETACHED; } return true; } + bool IsFlakyCallbackOrder(int callback1, int callback2) const { + if (callback1 == FRAME_ATTACHED && + (callback2 == MAIN_FRAME_CHANGED_ASSIGNED || callback2 == LOAD_START || + callback2 == LOAD_END)) { + // Timing of OnFrameAttached is flaky. See issue #3817. + return true; + } + + return false; + } + void VerifyCallbackStatus(const std::string& func, int current_callback) const { EXPECT_UI_THREAD(); for (int i = 0; i <= CALLBACK_LAST; ++i) { if (i < current_callback && IsExpectedCallback(i)) { - if (i == FRAME_ATTACHED && - (current_callback == MAIN_FRAME_CHANGED_ASSIGNED || - current_callback == LOAD_START || current_callback == LOAD_END)) { - // Timing of OnFrameAttached is flaky. See issue #3817. + if (IsFlakyCallbackOrder(i, current_callback)) { continue; } EXPECT_TRUE(got_callback_[i]) - << "inside " << func << " should already have gotten " - << GetCallbackName(i); + << "inside " << func << "\nfor " + << GetDebugString(/*dump_state=*/true) + << "\nshould already have gotten " << GetCallbackName(i); } else { - if (current_callback == FRAME_ATTACHED && - (i == MAIN_FRAME_CHANGED_ASSIGNED || i == LOAD_START || - i == LOAD_END)) { - // Timing of OnFrameAttached is flaky. See issue #3817. + if (IsFlakyCallbackOrder(current_callback, i)) { continue; } EXPECT_FALSE(got_callback_[i]) - << "inside " << func << " should not already have gotten " - << GetCallbackName(i); + << "inside " << func << "\nfor " + << GetDebugString(/*dump_state=*/true) + << "\nshould NOT already have gotten " << GetCallbackName(i); } } } @@ -436,7 +478,8 @@ struct FrameStatus { // nicely with the concept that "GetMainFrame() always returns a frame that // can be used", which wouldn't be the case if we returned the old frame // when calling GetMainFrame() from inside OnFrameCreated (for the new - // frame), OnFrameDetached (for the old frame) or OnMainFrameChanged. + // frame), OnFrameDetached/OnFrameDestoyed (for the old frame) or + // OnMainFrameChanged. auto main_frame = browser->GetMainFrame(); if (expect_valid) { EXPECT_TRUE(main_frame) << func; @@ -452,11 +495,13 @@ struct FrameStatus { void VerifyFrame(const std::string& func, CefRefPtr frame, - bool expect_valid = true) const { - if (expect_valid) { - EXPECT_TRUE(frame->IsValid()) << func; - } else { - EXPECT_FALSE(frame->IsValid()) << func; + std::optional expect_valid = true) const { + if (expect_valid.has_value()) { + if (*expect_valid) { + EXPECT_TRUE(frame->IsValid()) << func; + } else { + EXPECT_FALSE(frame->IsValid()) << func; + } } // |frame| should be us. This checks the frame type and ID. @@ -648,6 +693,13 @@ class OrderMainTestHandler : public RoutingTestHandler, public CefFrameHandler { current_main_frame_->OnFrameDetached(browser, frame); } + void OnFrameDestroyed(CefRefPtr browser, + CefRefPtr frame) override { + EXPECT_UI_THREAD(); + EXPECT_TRUE(current_main_frame_); + current_main_frame_->OnFrameDestroyed(browser, frame); + } + void OnMainFrameChanged(CefRefPtr browser, CefRefPtr old_frame, CefRefPtr new_frame) override { @@ -1003,14 +1055,14 @@ class FrameStatusMap { return true; } - bool AllFramesDetached() const { + bool AllFramesDestroyed() const { if (size() != expected_frame_ct_) { return false; } Map::const_iterator it = frame_map_.begin(); for (; it != frame_map_.end(); ++it) { - if (!it->second->IsDetached()) { + if (!it->second->IsDestroyed()) { return false; } } @@ -1125,21 +1177,33 @@ class OrderSubTestHandler : public NavigateOrderMainTestHandler { void OnFrameDetached(CefRefPtr browser, CefRefPtr frame) override { + if (!frame->IsMain()) { + auto map = GetFrameMap(frame); + auto status = map->GetFrameStatus(frame); + status->OnFrameDetached(browser, frame); + return; + } + + NavigateOrderMainTestHandler::OnFrameDetached(browser, frame); + } + + void OnFrameDestroyed(CefRefPtr browser, + CefRefPtr frame) override { if (!frame->IsMain()) { // Potentially the last notification for an old sub-frame after // navigation. auto map = GetFrameMap(frame); auto status = map->GetFrameStatus(frame); - status->OnFrameDetached(browser, frame); + status->OnFrameDestroyed(browser, frame); - if (map->AllFramesDetached()) { + if (map->AllFramesDestroyed()) { // Verify results from the previous navigation. VerifyAndClearSubFrameTestResults(map); } return; } - NavigateOrderMainTestHandler::OnFrameDetached(browser, frame); + NavigateOrderMainTestHandler::OnFrameDestroyed(browser, frame); } void OnLoadStart(CefRefPtr browser, @@ -1390,23 +1454,45 @@ class CrossOriginOrderSubTestHandler : public OrderSubTestHandler { mode, /*expected_frame_ct=*/4U) {} - void OnFrameDetached(CefRefPtr browser, - CefRefPtr frame) override { + void OnFrameCreated(CefRefPtr browser, + CefRefPtr frame) override { + OrderSubTestHandler::OnFrameCreated(browser, frame); + + if (!frame->IsMain() && + loaded_frame_child_ids_.find(ExtractChildId(frame->GetIdentifier())) != + loaded_frame_child_ids_.end()) { + // Mark sub-frames in the same process as a loaded frame as temporary. + // See below comments in OnFrameDestroyed. + auto map = GetFrameMap(frame); + auto status = map->GetFrameStatus(frame); + status->SetIsTemporary(true); + } + } + + void OnLoadStart(CefRefPtr browser, + CefRefPtr frame, + TransitionType transition_type) override { + OrderSubTestHandler::OnLoadStart(browser, frame, transition_type); + + loaded_frame_child_ids_.insert(ExtractChildId(frame->GetIdentifier())); + } + + void OnFrameDestroyed(CefRefPtr browser, + CefRefPtr frame) override { // A sub-frame is first created in the parent's renderer process. That // sub-frame is then discarded after the real cross-origin sub-frame is // created in a different renderer process. These discarded sub-frames will - // get OnFrameCreated/OnFrameAttached immediately followed by - // OnFrameDetached. + // get OnFrameCreated/OnFrameDestroyed. if (!frame->IsMain()) { auto map = GetFrameMap(frame); auto status = map->GetFrameStatus(frame); if (status && !status->DidGetCallback(FrameStatus::LOAD_START)) { - status->SetIsTemporary(true); + EXPECT_TRUE(status->IsTemporary()); temp_frame_detached_ct_++; } } - OrderSubTestHandler::OnFrameDetached(browser, frame); + OrderSubTestHandler::OnFrameDestroyed(browser, frame); } protected: @@ -1431,7 +1517,16 @@ class CrossOriginOrderSubTestHandler : public OrderSubTestHandler { } private: + // Parse the format from frame_util::MakeFrameIdentifier to return |child_id|. + static std::string ExtractChildId(const std::string& frame_id) { + const int pos = frame_id.find('-'); + CHECK_GT(pos, 0) << frame_id; + return frame_id.substr(0, pos); + } + size_t temp_frame_detached_ct_ = 0U; + + std::set loaded_frame_child_ids_; }; } // namespace @@ -1523,6 +1618,7 @@ class PopupOrderMainTestHandler : public OrderMainTestHandler { temp_main_frame_->SetAdditionalDebugInfo(GetAdditionalDebugInfo() + "temp "); temp_main_frame_->SetIsFirstMain(true); + temp_main_frame_->SetIsTemporary(true); temp_main_frame_->OnFrameCreated(browser, frame); return; } @@ -1558,6 +1654,17 @@ class PopupOrderMainTestHandler : public OrderMainTestHandler { OrderMainTestHandler::OnFrameAttached(browser, frame, reattached); } + void OnFrameDetached(CefRefPtr browser, + CefRefPtr frame) override { + if (temp_main_frame_ && temp_main_frame_->IsSame(frame)) { + EXPECT_TRUE(cross_origin_); + temp_main_frame_->OnFrameDetached(browser, frame); + return; + } + + OrderMainTestHandler::OnFrameDetached(browser, frame); + } + void OnMainFrameChanged(CefRefPtr browser, CefRefPtr old_frame, CefRefPtr new_frame) override { @@ -1570,8 +1677,8 @@ class PopupOrderMainTestHandler : public OrderMainTestHandler { OrderMainTestHandler::OnMainFrameChanged(browser, old_frame, new_frame); } - void OnFrameDetached(CefRefPtr browser, - CefRefPtr frame) override { + void OnFrameDestroyed(CefRefPtr browser, + CefRefPtr frame) override { if (temp_main_frame_ && temp_main_frame_->IsSame(frame)) { EXPECT_TRUE(cross_origin_); EXPECT_FALSE(got_temp_destroyed_); @@ -1579,28 +1686,30 @@ class PopupOrderMainTestHandler : public OrderMainTestHandler { #if VERBOSE_DEBUGGING LOG(INFO) << temp_main_frame_->GetDebugString() - << " callback OnFrameDetached(discarded)"; + << " callback OnFrameDestroyed(discarded)"; #endif // All of the initial main frame callbacks go to the proxy. EXPECT_TRUE(temp_main_frame_->DidGetCallback(FrameStatus::AFTER_CREATED)); EXPECT_TRUE(temp_main_frame_->DidGetCallback( FrameStatus::MAIN_FRAME_INITIAL_ASSIGNED)); - EXPECT_TRUE(!temp_main_frame_->DidGetCallback(FrameStatus::LOAD_START)); + EXPECT_FALSE(temp_main_frame_->DidGetCallback(FrameStatus::LOAD_START)); + EXPECT_FALSE(temp_main_frame_->DidGetCallback(FrameStatus::LOAD_END)); EXPECT_TRUE(temp_main_frame_->DidGetCallback(FrameStatus::FRAME_CREATED)); - EXPECT_TRUE( + EXPECT_FALSE( temp_main_frame_->DidGetCallback(FrameStatus::FRAME_ATTACHED)); + EXPECT_FALSE( + temp_main_frame_->DidGetCallback(FrameStatus::FRAME_DETACHED)); - // Should receive queries for OnFrameCreated, OnAfterCreated, - // OnFrameAttached. - EXPECT_EQ(temp_main_frame_->QueriesDeliveredCount(), 3); + // Temporary frames never attach. + EXPECT_EQ(temp_main_frame_->QueriesDeliveredCount(), 0); delete temp_main_frame_; temp_main_frame_ = nullptr; return; } - OrderMainTestHandler::OnFrameDetached(browser, frame); + OrderMainTestHandler::OnFrameDestroyed(browser, frame); } bool OnQuery(CefRefPtr browser, diff --git a/tests/ceftests/navigation_unittest.cc b/tests/ceftests/navigation_unittest.cc index 1aff16e3d..6076b3c4d 100644 --- a/tests/ceftests/navigation_unittest.cc +++ b/tests/ceftests/navigation_unittest.cc @@ -3593,7 +3593,9 @@ class ExtraInfoNavTestHandler : public TestHandler { CefRefPtr frame, int httpStatusCode) override { if (popup_opened_) { - DestroyTest(); + EXPECT_FALSE(got_load_end_popup_); + got_load_end_popup_.yes(); + MaybeDestroyTest(); } else { GrantPopupPermission(browser->GetHost()->GetRequestContext(), browser->GetMainFrame()->GetURL()); @@ -3641,9 +3643,12 @@ class ExtraInfoNavTestHandler : public TestHandler { EXPECT_TRUE(args->GetBool(0)); if (popup_opened_) { EXPECT_TRUE(args->GetBool(1)); + EXPECT_FALSE(got_process_message_popup_); got_process_message_popup_.yes(); + MaybeDestroyTest(); } else { EXPECT_FALSE(args->GetBool(1)); + EXPECT_FALSE(got_process_message_main_); got_process_message_main_.yes(); } return true; @@ -3653,15 +3658,23 @@ class ExtraInfoNavTestHandler : public TestHandler { return false; } - protected: + private: bool popup_opened_ = false; TrackCallback got_process_message_main_; TrackCallback got_process_message_popup_; + TrackCallback got_load_end_popup_; + + void MaybeDestroyTest() { + if (got_process_message_popup_ && got_load_end_popup_) { + DestroyTest(); + } + } void DestroyTest() override { // Verify test expectations. EXPECT_TRUE(got_process_message_main_); EXPECT_TRUE(got_process_message_popup_); + EXPECT_TRUE(got_load_end_popup_); TestHandler::DestroyTest(); }