Fix nullptr deref in maybeRenderFallbackContent()

third_party/WebKit/Source/web/WebLocalFrameImpl.cpp

 /*
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 2060 matching lines @@
 }
 
 bool WebLocalFrameImpl::maybeRenderFallbackContent(
     const WebURLError& error) const {
   DCHECK(frame());
 
   if (!frame()->owner() || !frame()->owner()->canRenderFallbackContent())
     return false;
 
   FrameLoader& frameloader = frame()->loader();
-  DCHECK(frameloader.provisionalDocumentLoader());
-  frameloader.loadFailed(frameloader.provisionalDocumentLoader(), error);
+  if (frameloader.provisionalDocumentLoader())

[arthursonzogni, 2017/03/31 09:16:33]

Can you add a comment that explain why the provisionalDocumentLoader have been
detached?

+    frameloader.loadFailed(frameloader.provisionalDocumentLoader(), error);
   return true;

[arthursonzogni, 2017/03/31 09:16:33]

Returning true here will not work. You can try to open the fallback.html page
you have made. The browser is still waiting the page to load the error page. You
can see the animated loading icon in the tab that is still running.

The FrameHostMsg_DidStopLoading should be sent to inform it that the load
stopped.

FYI: When a fallback content is displayed, this message will be sent because of
this list of call:
1) RenderFrameImpl::OnFailedNavigation()
2) WebLocalFrameImpl::maybeRenderFallbackContent()
3) Frameloader::loadFailed()
4) FrameLoader::checkCompleted()
5) ProgressTracker::progressCompleted()
6) LocalFrameClientImpl::didStopLoading()
7) RenderFrameImpl::didStopLoading()

If you don't call anything, the message will not be sent and the browser will
wait forever.

[Nate Chapin, 2017/03/31 23:59:59]

Is it safe to unconditionally call DidStopLoading here? It's hypothetically
possible (though surely very unlikely in practice) to end up in a situation
where the committed DocumentLoader is loading, but the provisional
DocumentLoader has been clobbered. In that case, we don't want to replace the
content with an error page, but at the same time the DCHECK is wrong. It's not
clear to me whether that's possible to do in a way that leaves the navigation
running in the browser process.

Assuming that path is possible: I've updated RenderFrameImpl to check
WebLocalFrame::isLoading(), and early-exiting with a FrameHostMsg_DidStopLoading
in that case, but no FrameHostMsg_DidStopLoading if maybeRenderFallbackContent()
is called and returns true. WDYT?

 }
 
 // Called when a navigation is blocked because a Content Security Policy (CSP)
 // is infringed.
 void WebLocalFrameImpl::reportContentSecurityPolicyViolation(
     const blink::WebContentSecurityPolicyViolation& violation) {
   addMessageToConsole(blink::WebConsoleMessage(
       WebConsoleMessage::LevelError, violation.consoleMessage,
       violation.sourceLocation.url, violation.sourceLocation.lineNumber,
       violation.sourceLocation.columnNumber));
@@ skipping 416 matching lines @@
         createMarkup(startPosition, endPosition, AnnotateForInterchange,
                      ConvertBlocksToInlines::NotConvert, ResolveNonLocalURLs);
   } else {
     clipHtml =
         createMarkup(endPosition, startPosition, AnnotateForInterchange,
                      ConvertBlocksToInlines::NotConvert, ResolveNonLocalURLs);
   }
 }
 
 }  // namespace blink