Fix nullptr deref in maybeRenderFallbackContent()

content/renderer/render_frame_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/render_frame_impl.h"
 
 #include <map>
 #include <string>
 #include <utility>
 #include <vector>
@@ skipping 5240 matching lines @@
   if (!ShouldDisplayErrorPageForFailedLoad(error_code, common_params.url)) {
     // The browser expects this frame to be loading an error page. Inform it
     // that the load stopped.
     Send(new FrameHostMsg_DidStopLoading(routing_id_));
     browser_side_navigation_pending_ = false;
     return;
   }
 
   // On load failure, a frame can ask its owner to render fallback content.
   // When that happens, don't load an error page.
+  bool was_loading = frame_->IsLoading();
   if (frame_->MaybeRenderFallbackContent(error)) {
+    // If the frame wasn't loading but was fallback-elligible, the fallback

[Charlie Reis, 2017/04/11 21:08:40]

nit: eligible

+    // content won't be shown. However, showing an error page isn't right
+    // either, as the frame has already been populated with something unrelated
+    // to this navigation failure. In that case, just send a stop IPC to the
+    // browser to unwind its state, and leave the frame as-is.

[Charlie Reis, 2017/04/11 21:08:40]

Sorry, I'm getting confused by this comment and method. 
MaybeRenderFallbackContent sounds like the kind of method that would return true
if it actually rendered the fallback content, but now it apparently returns true
even when it doesn't render the fallback in some cases.  (I'm also having
trouble reading the comment on the declaration of MaybeRenderFallbackContent,
since it might have a typo in it?)

Can we rename or restructure this (e.g., into two separate calls?) to make it
clearer?

[Nate Chapin, 2017/04/12 21:58:01]

I don't really like having two separate public/ API calls for this, so I changed
this to return an enum, which should give sufficient infromation for
RenderFrameImpl to do the right thing. Is that OK with you?

I think MaybeRenderFallbackContent is still an ok name with the enum return
value, let me know if you feel otherwise.

+    if (!was_loading)
+      Send(new FrameHostMsg_DidStopLoading(routing_id_));
     browser_side_navigation_pending_ = false;
     return;
   }
 
   // Make sure errors are not shown in view source mode.
   frame_->EnableViewSourceMode(false);
 
   // Replace the current history entry in reloads, and loads of the same url.
   // This corresponds to Blink's notion of a standard commit.
   // Also replace the current history entry if the browser asked for it
@@ skipping 1689 matching lines @@
       policy(info.default_policy),
       replaces_current_history_item(info.replaces_current_history_item),
       history_navigation_in_new_child_frame(
           info.is_history_navigation_in_new_child_frame),
       client_redirect(info.is_client_redirect),
       cache_disabled(info.is_cache_disabled),
       form(info.form),
       source_location(info.source_location) {}
 
 }  // namespace content