Frame::Navigate is responsible for immediately clearing frame observers

Context: LazyLoadFrameObserver::LoadImmediately only expects to ever be
called one time.

Before this CL, both LocalFrame::Navigate and RemoteFrame::Navigate
didn't immediately call HTMLFrameOwnerElement::CancelPendingLazyLoad().
Therefore if LazyLoadFrameObserver::LoadImmediately() was called, and
attempts to navigate a frame but the navigation fails before
CancelPendingLazyLoad() was called (which can happen due to the
navigation rate limiter etc.), LazyLoadFrameObserver::LoadImmedaitely
may be called a second time by the underlying intersection observer
which still observes the element.

R=sclittle@chromium.org

Bug: 1106290
Change-Id: I77f052c5d5a113d0c35f702322d6dc6ec831650b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2303352Reviewed-by: Scott Little <sclittle@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/master@{#790733}

third_party/blink/renderer/core/frame/local_frame.cc

@@ -453,6 +453,9 @@ bool LocalFrame::IsLocalRoot() const {
 
 void LocalFrame::Navigate(FrameLoadRequest& request,
                           WebFrameLoadType frame_load_type) {
+  if (HTMLFrameOwnerElement* element = DeprecatedLocalOwner())
+    element->CancelPendingLazyLoad();
+
   if (!navigation_rate_limiter().CanProceed())
     return;
   if (request.ClientRedirectReason() != ClientNavigationReason::kNone) {

third_party/blink/renderer/core/frame/remote_frame.cc

@@ -139,12 +139,12 @@ void RemoteFrame::Navigate(FrameLoadRequest& frame_request,
   // local frames.
   DCHECK_EQ(kNavigationPolicyCurrentTab, frame_request.GetNavigationPolicy());
 
-  if (!navigation_rate_limiter().CanProceed())
-    return;
-
   if (HTMLFrameOwnerElement* element = DeprecatedLocalOwner())
     element->CancelPendingLazyLoad();
 
+  if (!navigation_rate_limiter().CanProceed())
+    return;
+
   frame_request.SetFrameType(IsMainFrame()
                                  ? mojom::RequestContextFrameType::kTopLevel
                                  : mojom::RequestContextFrameType::kNested);

third_party/blink/renderer/core/html/lazy_load_frame_observer.cc

@@ -157,8 +157,6 @@ void LazyLoadFrameObserver::LoadIfHiddenOrNearViewport(
 }
 
 void LazyLoadFrameObserver::LoadImmediately() {
-  // TODO(domfarolino): Change the CHECKs to DCHECKs once we confirm there are
-  // no crashes.
   CHECK(IsLazyLoadPending());
   CHECK(lazy_load_request_info_);
 
@@ -181,8 +179,6 @@ void LazyLoadFrameObserver::LoadImmediately() {
   // DisconnectContentFrame() if the content frame changes.
   CHECK(element_->ContentFrame());
 
-  // Note that calling both FrameLoader::StartNavigation() and Frame::Navigate()
-  // causes the |lazy_load_intersection_observer_| to be disconnected.
   FrameLoadRequest request(element_->GetDocument().domWindow(),
                            scoped_request_info->resource_request);
 
@@ -195,6 +191,8 @@ void LazyLoadFrameObserver::LoadImmediately() {
                                        scoped_request_info->frame_load_type);
   }
 
+  // Note that whatever we delegate to for the navigation is responsible for
+  // clearing the frame's lazy load frame observer via |CancelPendingLayLoad()|.
   CHECK(!IsLazyLoadPending());
 }