Move most of FrameLoader::CheckCompleted() to Document

third_party/WebKit/Source/core/dom/Document.cpp

Index: third_party/WebKit/Source/core/dom/Document.cpp
diff --git a/third_party/WebKit/Source/core/dom/Document.cpp b/third_party/WebKit/Source/core/dom/Document.cpp
index 900666f02f56d32fe7f1f1e991e73235bb9842d9..b153d54178e3b233836606f57327b756970f7b9c 100644
--- a/third_party/WebKit/Source/core/dom/Document.cpp
+++ b/third_party/WebKit/Source/core/dom/Document.cpp
@@ -2787,12 +2787,11 @@ void Document::CancelParsing() {
   DetachParser();
   SetParsingState(kFinishedParsing);
   SetReadyState(kComplete);
+  SuppressLoadEvent();

[Nate Chapin, 2017/04/11 23:25:49]

Instead of the early-exit for a nullptr parser_ in Document::ImplicitClose().

 }
 
 DocumentParser* Document::ImplicitOpen(
     ParserSynchronizationPolicy parser_sync_policy) {
-  DetachParser();
-
   RemoveChildren();

[Nate Chapin, 2017/04/11 23:25:50]

This can trigger events, which can in turn call document.open()/document.close()
and cause reentrant parser state confusion. Just DetachParser() after.

   DCHECK(!focused_element_);
 
@@ -2806,6 +2805,7 @@ DocumentParser* Document::ImplicitOpen(
     parser_sync_policy = kForceSynchronousParsing;
   }
 
+  DetachParser();
   parser_sync_policy_ = parser_sync_policy;
   parser_ = CreateParser();
   DocumentParserTiming::From(*this).MarkParserStart();
@@ -2952,30 +2952,15 @@ void Document::close() {
       !GetScriptableDocumentParser()->IsParsing())
     return;
 
-  if (DocumentParser* parser = parser_)
-    parser->Finish();
-
-  if (!frame_) {
-    // Because we have no frame, we don't know if all loading has completed,
-    // so we just call implicitClose() immediately. FIXME: This might fire
-    // the load event prematurely
-    // <http://bugs.webkit.org/show_bug.cgi?id=14568>.
-    ImplicitClose();
-    return;
-  }
-
-  frame_->Loader().CheckCompleted();
+  parser_->Finish();
+  if (!parser_ || !parser_->IsParsing())
+    SetReadyState(kComplete);

[Nate Chapin, 2017/04/11 23:25:50]

There are cases where we want to SetReadyState(kComplete) while
ProcessingLoadEvent() (wtf?)

Instead of making parts of CheckCompleted() re-entrant so that we can enable
this insanity, just call SetReadyState(kComplete) here, unless of course,
parser_->Finish() triggered an event that re-open()ed the Document.

+  CheckCompleted();
 }
 
 void Document::ImplicitClose() {
   DCHECK(!InStyleRecalc());
-  if (ProcessingLoadEvent() || !parser_)
-    return;
-  if (GetFrame() &&
-      GetFrame()->GetNavigationScheduler().LocationChangePending()) {
-    SuppressLoadEvent();
-    return;
-  }
+  DCHECK(parser_);
 
   load_event_progress_ = kLoadEventInProgress;
 
@@ -3055,6 +3040,70 @@ void Document::ImplicitClose() {
 
   if (SvgExtensions())
     AccessSVGExtensions().StartAnimations();
+
+  View()->HandleLoadCompleted();

[Nate Chapin, 2017/04/11 23:25:50]

From FrameLoader::CheckCompleted(). It just needs to happen sometime around the
load event, not very timing-sensitive.

[yhirano, 2017/04/14 10:33:35]

Hmm, there are cases where this line is not executed.
 - LoadEventStillNeeded() may return false in CheckCompleted()
 - There are two early returns.

Could you give me some explanation why it's OK?

[Nate Chapin, 2017/04/18 21:56:38]

I'm not absolutely sure it's OK (though all the tests did pass), and it harms
nothing to return it to CheckCompleted(), so I'll do that.

+}
+
+static bool AllDescendantsAreComplete(Frame* frame) {
+  if (!frame)

[Nate Chapin, 2017/04/11 23:25:50]

Vacuously true! You can't have any descendants if you're already nullptr!

This cases needs to be handled in ShouldComplete() so that detached Documents
can fire onload, and doing it here means ShouldComplete() is all && clauses, no
paren-nesting required.

+    return true;
+  for (Frame* child = frame->Tree().FirstChild(); child;
+       child = child->Tree().TraverseNext(frame)) {
+    if (child->IsLoading())
+      return false;
+  }
+  return true;
+}
+
+bool Document::ShouldComplete() {
+  return parsing_state_ == kFinishedParsing && HaveImportsLoaded() &&
+         !fetcher_->BlockingRequestCount() && !IsDelayingLoadEvent() &&
+         load_event_progress_ != kLoadEventInProgress &&
+         AllDescendantsAreComplete(frame_);
+}
+
+void Document::CheckCompleted() {
+  if (!ShouldComplete())
+    return;
+
+  if (frame_) {
+    frame_->Client()->RunScriptsAtDocumentIdle();
+
+    // Injected scripts may have disconnected this frame.
+    if (!frame_)
+      return;
+
+    // Check again, because runScriptsAtDocumentIdle() may have delayed the load
+    // event.
+    if (!ShouldComplete())
+      return;
+  }
+
+  // OK, completed. Fire load completion events as needed.
+  SetReadyState(kComplete);
+  if (LoadEventStillNeeded())
+    ImplicitClose();
+
+  // The readystatechanged or load event may have disconnected this frame.
+  if (!frame_ || !frame_->IsAttached())
+    return;
+  frame_->GetNavigationScheduler().StartTimer();
+  // The document itself is complete, but if a child frame was restarted due to
+  // an event, this document is still considered to be in progress.
+  if (!AllDescendantsAreComplete(frame_))
+    return;
+
+  // No need to repeat if we've already notified this load as finished.
+  if (!Loader()->SentDidFinishLoad()) {
+    if (frame_->IsMainFrame())
+      ViewportDescription().ReportMobilePageStats(frame_);
+    Loader()->SetSentDidFinishLoad();
+    frame_->Client()->DispatchDidFinishLoad();
+    if (!frame_)
+      return;
+  }
+
+  frame_->Loader().DidFinishNavigation();
 }
 
 bool Document::DispatchBeforeUnloadEvent(ChromeClient& chrome_client,
@@ -5972,7 +6021,7 @@ void Document::DecrementLoadEventDelayCountAndCheckLoadEvent() {
   --load_event_delay_count_;
 
   if (!load_event_delay_count_ && GetFrame())

[yhirano, 2017/04/14 10:33:35]

Is checking GetFrame() needed here?

[Nate Chapin, 2017/04/18 21:56:38]

This is needed to ensure the DCHECK(parser_) isn't triggered in CheckCompleted()
in the case where a Document *never* has a parser (cloneNode seems to be the
main way this can happen).

Instead of checking GetFrame() here, this patch now sets load_event_progress_ to
kLoadEventCompleted in the constructor, and moves resetting the progress to
kLoadEventNotRun to implicitOpen() instead of open(), so that it runs for
document.open() as well as parser creation during DocumentWriter initialization.

-    GetFrame()->Loader().CheckCompleted();
+    CheckCompleted();
 }
 
 void Document::CheckLoadEventSoon() {
@@ -5994,8 +6043,7 @@ bool Document::IsDelayingLoadEvent() {
 }
 
 void Document::LoadEventDelayTimerFired(TimerBase*) {
-  if (GetFrame())
-    GetFrame()->Loader().CheckCompleted();
+  CheckCompleted();
 }
 
 void Document::LoadPluginsSoon() {