Fix nullptr deref in maybeRenderFallbackContent()

Fix nullptr deref in maybeRenderFallbackContent()

There are no strict guarantees the provisional DocumentLoader won't have been
detached by the time maybeRenderFallbackContent() is called.

BUG=704523
Review-Url: https://codereview.chromium.org/2783743002
Cr-Commit-Position: refs/heads/master@{#464526}
Committed: https://chromium.googlesource.com/chromium/src/+/ecbbca63b50474ff80cf3410e491f2bc52813b81

[Nate Chapin, 2017-03-28 18:22:13]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[Nate Chapin, 2017-03-28 18:22:29]

The CQ bit was unchecked by japhet@chromium.org

[Nate Chapin, 2017-03-28 18:22:40]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 18:23:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nate Chapin, 2017-03-28 18:25:01]

japhet@chromium.org changed reviewers:
+ ananta@chromium.org, arthursonzogni@chromium.org, clamy@chromium.org

[Nate Chapin, 2017-03-28 18:25:01]

Greetings PlzNavigaters,

I made a crash in
https://chromium.googlesource.com/chromium/src/+/476af767372c72fd5e71f365e784....
The fix is simple and obvious, but I haven't been able to actually reproduce,
since it appears to be a very specific timing issue. Thoughts on how to
reproduce, or alternately, whether it's ok to land without test given its
trivial nature?

[commit-bot: I haz the power, 2017-03-28 20:40:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 20:40:36]

Dry run: This issue passed the CQ dry run.

[arthursonzogni, 2017-03-29 08:24:39]

On 2017/03/28 18:25:01, Nate Chapin wrote:
> Greetings PlzNavigaters,
> 
> I made a crash in
>
https://chromium.googlesource.com/chromium/src/+/476af767372c72fd5e71f365e784....
> The fix is simple and obvious, but I haven't been able to actually reproduce,
> since it appears to be a very specific timing issue. Thoughts on how to
> reproduce, or alternately, whether it's ok to land without test given its
> trivial nature?

Hi Nate,
Thank you for what you have done in https://crrev.com/2694013005 !

I have a question for this CL, returning false here means that no fallback
content will be displayed/executed, even if there is one. I think it is a
problem, because the web page will not be fully displayed. What do you think?

What does it means for the frame when there is no more
provisionalDocumentLoader?

I was thinking, maybe you can do this at least:
```
if (!frameloader.provisionalDocumentLoader()) {
  NOTREACHED(); // <----
  return false;
}
```
In order to always be able to detect this in debug mode and maybe to find a way
to reproduce it.

[Nate Chapin, 2017-03-29 19:16:13]

On 2017/03/29 08:24:39, arthursonzogni wrote:
> On 2017/03/28 18:25:01, Nate Chapin wrote:
> > Greetings PlzNavigaters,
> > 
> > I made a crash in
> >
>
https://chromium.googlesource.com/chromium/src/+/476af767372c72fd5e71f365e784....
> > The fix is simple and obvious, but I haven't been able to actually
reproduce,
> > since it appears to be a very specific timing issue. Thoughts on how to
> > reproduce, or alternately, whether it's ok to land without test given its
> > trivial nature?
> 
> Hi Nate,
> Thank you for what you have done in https://crrev.com/2694013005 !
> 
> I have a question for this CL, returning false here means that no fallback
> content will be displayed/executed, even if there is one. I think it is a
> problem, because the web page will not be fully displayed. What do you think?
> 
> What does it means for the frame when there is no more
> provisionalDocumentLoader?
> 
> I was thinking, maybe you can do this at least:
> ```
> if (!frameloader.provisionalDocumentLoader()) {
>   NOTREACHED(); // <----
>   return false;
> }
> ```
> In order to always be able to detect this in debug mode and maybe to find a
way
> to reproduce it.

If the frame is still alive but there is no more provisionalDocumentLoader(), it
means either: (1) a different navigation started and ended in the meantime
(about:blank might do this?), or (2) the frame was stopped (user abort,
window.stop() in JS, etc.). For (1), the navigation will have either
successfully loaded or failed and triggered fallback. For (2),
FrameLoader::loadFailed() will definitely be called, so fallback should be
triggered. So either way, I think we will be showing either valid content or the
fallback.

[arthursonzogni, 2017-03-30 08:42:01]

On 2017/03/29 19:16:13, Nate Chapin wrote:
> On 2017/03/29 08:24:39, arthursonzogni wrote:
> > On 2017/03/28 18:25:01, Nate Chapin wrote:
> > > Greetings PlzNavigaters,
> > > 
> > > I made a crash in
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/476af767372c72fd5e71f365e784....
> > > The fix is simple and obvious, but I haven't been able to actually
> reproduce,
> > > since it appears to be a very specific timing issue. Thoughts on how to
> > > reproduce, or alternately, whether it's ok to land without test given its
> > > trivial nature?
> > 
> > Hi Nate,
> > Thank you for what you have done in https://crrev.com/2694013005 !
> > 
> > I have a question for this CL, returning false here means that no fallback
> > content will be displayed/executed, even if there is one. I think it is a
> > problem, because the web page will not be fully displayed. What do you
think?
> > 
> > What does it means for the frame when there is no more
> > provisionalDocumentLoader?
> > 
> > I was thinking, maybe you can do this at least:
> > ```
> > if (!frameloader.provisionalDocumentLoader()) {
> >   NOTREACHED(); // <----
> >   return false;
> > }
> > ```
> > In order to always be able to detect this in debug mode and maybe to find a
> way
> > to reproduce it.
> 
> If the frame is still alive but there is no more provisionalDocumentLoader(),
it
> means either: (1) a different navigation started and ended in the meantime
> (about:blank might do this?), or (2) the frame was stopped (user abort,
> window.stop() in JS, etc.). For (1), the navigation will have either
> successfully loaded or failed and triggered fallback. For (2),
> FrameLoader::loadFailed() will definitely be called, so fallback should be
> triggered. So either way, I think we will be showing either valid content or
the
> fallback.

I see, thanks!. For (1), I understand that no fallback content should be
displayed because there is already something loaded. This case occurs mainly for
about:blank. To be exact, for every url such that 
ShouldMakeNetworkRequestForURL(url) == false.
So I think what you have done is the right thing to do.

I wasn't able to reproduce (2), but I was able to reproduce (1) following what
you have said. It triggers the old DCHECK:
```
<object id="obj" data="http://page-that-will-display-an-error.com">Fallback
content</object>
<script> document.getElementById("obj").data = "about:blank"; </script>
```
I think it would be nice to have a test for this kind of navigation. Would you
mind making one?
I tried your patch with this test. The result is that the about:blank page is
replaced by the error page. I think we probably want the previous loaded page
not to be replaced, but it is probably acceptable. Let me know what do you
think. If we want not to display the error page, the render_frame_impl should do
nothing, except sending a FrameHostMsg_DidStopLoading.

I think you should add a comment explaining that when
frameloader.provisionalDocumentLoader is null, it means that the frameloader has
already loaded some content while the previous navigation was taking place.

LGTM % test && comment. For what it worth since I am not an OWNER.

[Nate Chapin, 2017-03-30 22:50:35]

On 2017/03/30 08:42:01, arthursonzogni wrote:
> On 2017/03/29 19:16:13, Nate Chapin wrote:
> > On 2017/03/29 08:24:39, arthursonzogni wrote:
> > > On 2017/03/28 18:25:01, Nate Chapin wrote:
> > > > Greetings PlzNavigaters,
> > > > 
> > > > I made a crash in
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/476af767372c72fd5e71f365e784....
> > > > The fix is simple and obvious, but I haven't been able to actually
> > reproduce,
> > > > since it appears to be a very specific timing issue. Thoughts on how to
> > > > reproduce, or alternately, whether it's ok to land without test given
its
> > > > trivial nature?
> > > 
> > > Hi Nate,
> > > Thank you for what you have done in https://crrev.com/2694013005 !
> > > 
> > > I have a question for this CL, returning false here means that no fallback
> > > content will be displayed/executed, even if there is one. I think it is a
> > > problem, because the web page will not be fully displayed. What do you
> think?
> > > 
> > > What does it means for the frame when there is no more
> > > provisionalDocumentLoader?
> > > 
> > > I was thinking, maybe you can do this at least:
> > > ```
> > > if (!frameloader.provisionalDocumentLoader()) {
> > >   NOTREACHED(); // <----
> > >   return false;
> > > }
> > > ```
> > > In order to always be able to detect this in debug mode and maybe to find
a
> > way
> > > to reproduce it.
> > 
> > If the frame is still alive but there is no more
provisionalDocumentLoader(),
> it
> > means either: (1) a different navigation started and ended in the meantime
> > (about:blank might do this?), or (2) the frame was stopped (user abort,
> > window.stop() in JS, etc.). For (1), the navigation will have either
> > successfully loaded or failed and triggered fallback. For (2),
> > FrameLoader::loadFailed() will definitely be called, so fallback should be
> > triggered. So either way, I think we will be showing either valid content or
> the
> > fallback.
> 
> I see, thanks!. For (1), I understand that no fallback content should be
> displayed because there is already something loaded. This case occurs mainly
for
> about:blank. To be exact, for every url such that 
> ShouldMakeNetworkRequestForURL(url) == false.
> So I think what you have done is the right thing to do.
> 
> I wasn't able to reproduce (2), but I was able to reproduce (1) following what
> you have said. It triggers the old DCHECK:
> ```
> <object id="obj" data="http://page-that-will-display-an-error.com">Fallback
> content</object>
> <script> document.getElementById("obj").data = "about:blank"; </script>
> ```
> I think it would be nice to have a test for this kind of navigation. Would you
> mind making one?

Made it a unit test, because the layout test was too timing-dependent.

> I tried your patch with this test. The result is that the about:blank page is
> replaced by the error page. I think we probably want the previous loaded page
> not to be replaced, but it is probably acceptable. Let me know what do you
> think. If we want not to display the error page, the render_frame_impl should
do
> nothing, except sending a FrameHostMsg_DidStopLoading.

I changed maybeRenderFallbackContent() to return trun if the
provisionalDocumentLoader() is null, so that we won't show an error page. I
think that's sufficient, since the about:blank navigation should have taken care
of sending the FrameHostMsg_DidStopLoading.

> 
> I think you should add a comment explaining that when
> frameloader.provisionalDocumentLoader is null, it means that the frameloader
has
> already loaded some content while the previous navigation was taking place.
> 
> LGTM % test && comment. For what it worth since I am not an OWNER.

That's all I need, I'm an OWNER for all these files, and you're the domain
expert on this particular function anyway :)

[Nate Chapin, 2017-03-30 22:50:50]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 22:51:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 00:02:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 00:02:56]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[arthursonzogni, 2017-03-31 09:15:55]

On 2017/03/30 22:50:35, Nate Chapin wrote:
> On 2017/03/30 08:42:01, arthursonzogni wrote:
> > On 2017/03/29 19:16:13, Nate Chapin wrote:
> > > On 2017/03/29 08:24:39, arthursonzogni wrote:
> > > > On 2017/03/28 18:25:01, Nate Chapin wrote:
> > > > > Greetings PlzNavigaters,
> > > > > 
> > > > > I made a crash in
> > > > >
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/476af767372c72fd5e71f365e784....
> > > > > The fix is simple and obvious, but I haven't been able to actually
> > > reproduce,
> > > > > since it appears to be a very specific timing issue. Thoughts on how
to
> > > > > reproduce, or alternately, whether it's ok to land without test given
> its
> > > > > trivial nature?
> > > > 
> > > > Hi Nate,
> > > > Thank you for what you have done in https://crrev.com/2694013005 !
> > > > 
> > > > I have a question for this CL, returning false here means that no
fallback
> > > > content will be displayed/executed, even if there is one. I think it is
a
> > > > problem, because the web page will not be fully displayed. What do you
> > think?
> > > > 
> > > > What does it means for the frame when there is no more
> > > > provisionalDocumentLoader?
> > > > 
> > > > I was thinking, maybe you can do this at least:
> > > > ```
> > > > if (!frameloader.provisionalDocumentLoader()) {
> > > >   NOTREACHED(); // <----
> > > >   return false;
> > > > }
> > > > ```
> > > > In order to always be able to detect this in debug mode and maybe to
find
> a
> > > way
> > > > to reproduce it.
> > > 
> > > If the frame is still alive but there is no more
> provisionalDocumentLoader(),
> > it
> > > means either: (1) a different navigation started and ended in the meantime
> > > (about:blank might do this?), or (2) the frame was stopped (user abort,
> > > window.stop() in JS, etc.). For (1), the navigation will have either
> > > successfully loaded or failed and triggered fallback. For (2),
> > > FrameLoader::loadFailed() will definitely be called, so fallback should be
> > > triggered. So either way, I think we will be showing either valid content
or
> > the
> > > fallback.
> > 
> > I see, thanks!. For (1), I understand that no fallback content should be
> > displayed because there is already something loaded. This case occurs mainly
> for
> > about:blank. To be exact, for every url such that 
> > ShouldMakeNetworkRequestForURL(url) == false.
> > So I think what you have done is the right thing to do.
> > 
> > I wasn't able to reproduce (2), but I was able to reproduce (1) following
what
> > you have said. It triggers the old DCHECK:
> > ```
> > <object id="obj" data="http://page-that-will-display-an-error.com">Fallback
> > content</object>
> > <script> document.getElementById("obj").data = "about:blank"; </script>
> > ```
> > I think it would be nice to have a test for this kind of navigation. Would
you
> > mind making one?
> 
> Made it a unit test, because the layout test was too timing-dependent.
> 
> > I tried your patch with this test. The result is that the about:blank page
is
> > replaced by the error page. I think we probably want the previous loaded
page
> > not to be replaced, but it is probably acceptable. Let me know what do you
> > think. If we want not to display the error page, the render_frame_impl
should
> do
> > nothing, except sending a FrameHostMsg_DidStopLoading.
> 
> I changed maybeRenderFallbackContent() to return trun if the
> provisionalDocumentLoader() is null, so that we won't show an error page. I
> think that's sufficient, since the about:blank navigation should have taken
care
> of sending the FrameHostMsg_DidStopLoading.
> 
> > 
> > I think you should add a comment explaining that when
> > frameloader.provisionalDocumentLoader is null, it means that the frameloader
> has
> > already loaded some content while the previous navigation was taking place.
> > 
> > LGTM % test && comment. For what it worth since I am not an OWNER.
> 
> That's all I need, I'm an OWNER for all these files, and you're the domain
> expert on this particular function anyway :)

Thanks for the test! It is a pity that we can't make a Layout test.
If you choose to return true and so not loading the error page. You will have to
take care of sending the DidStopLoading message to the browser.

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

https://codereview.chromium.org/2783743002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2783743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2081: if
(frameloader.provisionalDocumentLoader())
Can you add a comment that explain why the provisionalDocumentLoader have been
detached?

https://codereview.chromium.org/2783743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2083: return true;
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.

https://codereview.chromium.org/2783743002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2783743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11619: // Overwrite the
client-handled child frame navigation with about:blank
Nit: maybe a dot at the end of the line.

[Nate Chapin, 2017-03-31 23:59:49]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

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

https://codereview.chromium.org/2783743002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2783743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2083: return true;
On 2017/03/31 09:16:33, arthursonzogni wrote:
> 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.
> 
> 

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?

https://codereview.chromium.org/2783743002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2783743002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11619: // Overwrite the
client-handled child frame navigation with about:blank
On 2017/03/31 09:16:33, arthursonzogni wrote:
> Nit: maybe a dot at the end of the line.

Done.

[commit-bot: I haz the power, 2017-04-01 00:00:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-01 00:31:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-01 00:31:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[arthursonzogni, 2017-04-03 11:02:02]

On 2017/03/31 23:59:59, Nate Chapin wrote:
> Is it safe to unconditionally call DidStopLoading here? 
It looks like it makes
PlzNavigateNavigationHandleImplBrowserTest.ErrorPageBlockedNavigation
failing.

I don't really like the following idea, but you can probably return an enum in
maybeRenderFallbackContent to inform the caller of what happened.
Alternatively, you can split the method into CanRenderFallbackContent() and
MaybeRenderFallbackContent()
Depending on the result:
(0,_) -> There is no fallback content => load an error page instead.
(1,1) -> The fallback content has been loaded => return;
(1,0) -> The fallback content has not been loaded => send DidStopLoading and
return;

[Nate Chapin, 2017-04-04 18:39:29]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 18:40:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nate Chapin, 2017-04-04 18:41:00]

On 2017/04/03 11:02:02, arthursonzogni wrote:
> On 2017/03/31 23:59:59, Nate Chapin wrote:
> > Is it safe to unconditionally call DidStopLoading here? 
> It looks like it makes
> PlzNavigateNavigationHandleImplBrowserTest.ErrorPageBlockedNavigation
> failing.
> 
> I don't really like the following idea, but you can probably return an enum in
> maybeRenderFallbackContent to inform the caller of what happened.
> Alternatively, you can split the method into CanRenderFallbackContent() and
> MaybeRenderFallbackContent()
> Depending on the result:
> (0,_) -> There is no fallback content => load an error page instead.
> (1,1) -> The fallback content has been loaded => return;
> (1,0) -> The fallback content has not been loaded => send DidStopLoading and
> return;

I did a variant of this: leave maybeRenderFallbackContent() with a bool return
value, returning true if we canRenderFallbackContent(), regardless of whether we
did or not. Let RenderFrameImpl check isLoading() before calling
maybeRenderFallbackContent(), and if it returns true and isLoading() was false,
then do the special FrameHostMsg_DidStopLoading. PTAL

[commit-bot: I haz the power, 2017-04-04 20:15:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 20:15:17]

Dry run: This issue passed the CQ dry run.

[arthursonzogni, 2017-04-05 07:53:34]

It looks good to me. Thanks!
LGTM % one error to fix.

https://codereview.chromium.org/2783743002/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2783743002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:5271: bool was_loading =
!frame_->isLoading();
This is wrong here.
You meant: bool was_loading = frame_->isLoading();
Without the "!"

[Nate Chapin, 2017-04-11 18:39:00]

https://codereview.chromium.org/2783743002/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2783743002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:5271: bool was_loading =
!frame_->isLoading();
On 2017/04/05 07:53:34, arthursonzogni wrote:
> This is wrong here.
> You meant: bool was_loading = frame_->isLoading();
> Without the "!"

Done.

[Nate Chapin, 2017-04-11 18:39:05]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2017-04-11 18:39:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
arthursonzogni@chromium.org
Link to the patchset: https://codereview.chromium.org/2783743002/#ps80001
(title: "Rebase, flip was_loading")

[commit-bot: I haz the power, 2017-04-11 18:39:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 18:53:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 18:53:43]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Nate Chapin, 2017-04-11 20:28:22]

japhet@chromium.org changed reviewers:
+ creis@chromium.org

[Nate Chapin, 2017-04-11 20:28:22]

Hey creis, would you mind doing a quick content/ review here?

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

https://codereview.chromium.org/2783743002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2783743002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:5263: // If the frame wasn't loading but
was fallback-elligible, the fallback
nit: eligible

https://codereview.chromium.org/2783743002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:5267: // browser to unwind its state, and
leave the frame as-is.
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 19:43:14]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 19:44:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 21:17:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-12 21:17:28]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2783743002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2783743002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:5267: // browser to unwind its state, and
leave the frame as-is.
On 2017/04/11 21:08:40, Charlie Reis (busy until Fri) wrote:
> 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?

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.

[Charlie Reis, 2017-04-12 22:05:22]

Yep, that works for me-- thanks!  LGTM.

https://codereview.chromium.org/2783743002/diff/100001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2783743002/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5265: // If the frame wasn't loading but
was fallback-elligible, the fallback
nit: eligible

https://codereview.chromium.org/2783743002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/2783743002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebLocalFrame.h:158: NoFallbackContent,
Comment: An error page should be shown instead of fallback content.

https://codereview.chromium.org/2783743002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebLocalFrame.h:159: NoLoadInProgress,
Comment: Something else committed instead, so no error page or fallback content
is needed.

https://codereview.chromium.org/2783743002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebLocalFrame.h:160: FallbackRendered
Comment: Fallback content was shown so no error page should be displayed.

[Nate Chapin, 2017-04-13 17:32:22]

https://codereview.chromium.org/2783743002/diff/100001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2783743002/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5265: // If the frame wasn't loading but
was fallback-elligible, the fallback
On 2017/04/12 22:05:22, Charlie Reis (busy until Fri) wrote:
> nit: eligible

Done.

https://codereview.chromium.org/2783743002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/2783743002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebLocalFrame.h:158: NoFallbackContent,
On 2017/04/12 22:05:22, Charlie Reis (busy until Fri) wrote:
> Comment: An error page should be shown instead of fallback content.

Done.

https://codereview.chromium.org/2783743002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebLocalFrame.h:159: NoLoadInProgress,
On 2017/04/12 22:05:22, Charlie Reis (busy until Fri) wrote:
> Comment: Something else committed instead, so no error page or fallback
content
> is needed.

Done.

https://codereview.chromium.org/2783743002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebLocalFrame.h:160: FallbackRendered
On 2017/04/12 22:05:22, Charlie Reis (busy until Fri) wrote:
> Comment: Fallback content was shown so no error page should be displayed.

Done.

[Nate Chapin, 2017-04-13 17:32:38]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2017-04-13 17:32:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
arthursonzogni@chromium.org, creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2783743002/#ps120001
(title: "Fix comments")

[commit-bot: I haz the power, 2017-04-13 17:33:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 20:25:46]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1492104758804760,
"parent_rev": "b5f4c9ef3c4ee3f316de7dacc5bee73bef98c4bc", "commit_rev":
"ecbbca63b50474ff80cf3410e491f2bc52813b81"}

[commit-bot: I haz the power, 2017-04-13 20:26:43]

Description was changed from

==========
Fix nullptr deref in maybeRenderFallbackContent()

There are no strict guarantees the provisional DocumentLoader won't have been
detached by the time maybeRenderFallbackContent() is called.

BUG=704523
==========

to

==========
Fix nullptr deref in maybeRenderFallbackContent()

There are no strict guarantees the provisional DocumentLoader won't have been
detached by the time maybeRenderFallbackContent() is called.

BUG=704523

Review-Url: https://codereview.chromium.org/2783743002
Cr-Commit-Position: refs/heads/master@{#464526}
Committed:
https://chromium.googlesource.com/chromium/src/+/ecbbca63b50474ff80cf3410e491...
==========

[commit-bot: I haz the power, 2017-04-13 20:26:46]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/ecbbca63b50474ff80cf3410e491...