Better distinguish didFinishLoad and didStopLoading

third_party/WebKit/Source/core/loader/FrameLoader.cpp

 /*
  * Copyright (C) 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
  * Copyright (C) 2008 Alp Toker <alp@atoker.com>
  * Copyright (C) Research In Motion Limited 2009. All rights reserved.
  * Copyright (C) 2011 Kris Jordan <krisjordan@gmail.com>
  * Copyright (C) 2011 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ skipping 511 matching lines @@
         return false;
     if (!document->haveImportsLoaded())
         return false;
     if (document->fetcher()->requestCount())
         return false;
     if (document->isDelayingLoadEvent())
         return false;
     return allDescendantsAreComplete(document->frame());
 }
 
-static bool shouldSendCompleteNotifications(LocalFrame* frame)
+static bool shouldSendFinishNotification(LocalFrame* frame)
 {
     // Don't send stop notifications for inital empty documents, since they don't generate start notifications.
     if (!frame->loader().stateMachine()->committedFirstRealDocumentLoad())
         return false;
 
-    // FIXME: We might have already sent stop notifications and be re-completing.
-    if (!frame->isLoading())
-        return false;
-
-    // The readystatechanged or load event may have disconnected this frame.

[dcheng, 2015/10/08 05:34:35]

Move this comment to 578 in the new file.

[Nate Chapin, 2015/10/08 21:03:59]

Done.

-    if (!frame->client())
-        return false;
-
-    // An event might have restarted a child frame.
-    if (!allDescendantsAreComplete(frame))
-        return false;
-
-    // An event might have restarted this frame by scheduling a new navigation.
-    if (frame->loader().provisionalDocumentLoader())
-        return false;
-
-    // A navigation is still scheduled in the embedder, so don't complete yet.
-    if (frame->loader().client()->hasPendingNavigation())
+    // Don't send didFinishLoad more than once per DocumentLoader.
+    if (frame->loader().documentLoader()->sentDidFinishLoad())
         return false;
 
     // We might have declined to run the load event due to an imminent content-initiated navigation.

[dcheng, 2015/10/08 05:37:40]

This is probably a dumb question... content-initiated: is this referring to the
embedder, e.g. //content?

[Nate Chapin, 2015/10/08 21:03:59]

In this case, it means blink. I think it's the same terminology as in
RenderFrameImpl: browser-initiated is from the browser process,
content-initiated is from blink. That naming is unfortunate.

     if (!frame->document()->loadEventFinished())
         return false;
 
+    // An event might have restarted a child frame.
+    if (!allDescendantsAreComplete(frame))
+        return false;
     return true;
 }
 
+static bool shouldSendCompleteNotification(LocalFrame* frame)
+{
+    // FIXME: We might have already sent stop notifications and be re-completing.
+    if (!frame->isLoading())
+        return false;
+    // Only send didStopLoading() if there are no navigations in progress at all,
+    // whether committed, provisional, or pending.
+    return frame->loader().documentLoader()->sentDidFinishLoad() && !frame->loader().provisionalDocumentLoader() && !frame->loader().client()->hasPendingNavigation();
+}
+
 void FrameLoader::checkCompleted()
 {
     RefPtrWillBeRawPtr<LocalFrame> protect(m_frame.get());
     if (!shouldComplete(m_frame->document()))
         return;
 
     // OK, completed.
     m_frame->document()->setReadyState(Document::Complete);
     if (m_frame->document()->loadEventStillNeeded())
         m_frame->document()->implicitClose();
 
     m_frame->navigationScheduler().startTimer();
 
     if (m_frame->view())
         m_frame->view()->handleLoadCompleted();
 
-    if (shouldSendCompleteNotifications(m_frame)) {
+    if (!m_frame->client())
+        return;
+
+    if (shouldSendFinishNotification(m_frame)) {
+        // Report mobile vs. desktop page statistics. This will only report on Android.
+        if (m_frame->isMainFrame())
+            m_frame->document()->viewportDescription().reportMobilePageStats(m_frame);
+        m_documentLoader->setSentDidFinishLoad();
+        client()->dispatchDidFinishLoad();
+        // Finishing the load can detach the frame when running layout tests.

[dcheng, 2015/10/08 05:34:35]

Can we fix this, maybe in a followup patch? This is the layout test runner doing
something crazy in didFinishLoad(), right?

[Nate Chapin, 2015/10/08 21:03:59]

I think the craziness is tearing down the test. I guess that could/should be
done async?

[dcheng, 2015/10/09 18:34:01]

Yeah, I think it'd make sense to do it async... when possible, I think we should
move in the direction of allowing less insanity in the WebFrameClient callbacks.

[Nate Chapin, 2015/10/09 18:39:44]

Agreed.

+        if (!m_frame->client())
+            return;
+    }
+
+    if (shouldSendCompleteNotification(m_frame)) {
         m_progressTracker->progressCompleted();
         // Retry restoring scroll offset since finishing loading disables content
         // size clamping.
         restoreScrollPositionAndViewState();
 
         m_loadType = FrameLoadTypeStandard;
         m_frame->localDOMWindow()->finishedLoading();
-
-        // Report mobile vs. desktop page statistics. This will only report on Android.
-        if (m_frame->isMainFrame())
-            m_frame->document()->viewportDescription().reportMobilePageStats(m_frame);
-        client()->dispatchDidFinishLoad();
     }
 
     Frame* parent = m_frame->tree().parent();
     if (parent && parent->isLocalFrame())
         toLocalFrame(parent)->loader().checkCompleted();
 }
 
 void FrameLoader::checkTimerFired(Timer<FrameLoader>*)
 {
     if (Page* page = m_frame->page()) {
@@ skipping 612 matching lines @@
     if (loader == m_provisionalDocumentLoader) {
         client()->dispatchDidFailProvisionalLoad(error, historyCommitType);
         if (loader != m_provisionalDocumentLoader)
             return;
         detachDocumentLoader(m_provisionalDocumentLoader);
         m_progressTracker->progressCompleted();
     } else {
         ASSERT(loader == m_documentLoader);
         if (m_frame->document()->parser())
             m_frame->document()->parser()->stopParsing();
+        m_documentLoader->setSentDidFinishLoad();
         if (!m_provisionalDocumentLoader && m_frame->isLoading()) {
             client()->dispatchDidFailLoad(error, historyCommitType);
             m_progressTracker->progressCompleted();
         }
     }
     checkCompleted();
 }
 
 bool FrameLoader::shouldPerformFragmentNavigation(bool isFormSubmission, const String& httpMethod, FrameLoadType loadType, const KURL& url)
 {
@@ skipping 286 matching lines @@
     // FIXME: We need a way to propagate insecure requests policy flags to
     // out-of-process frames. For now, we'll always use default behavior.
     if (!parentFrame->isLocalFrame())
         return nullptr;
 
     ASSERT(toLocalFrame(parentFrame)->document());
     return toLocalFrame(parentFrame)->document()->insecureNavigationsToUpgrade();
 }
 
 } // namespace blink