Fixes a reuse WebUI bug because of the incorrect shadowing of a boolean value.

Fixes a reuse WebUI bug because of the incorrect shadowing of a boolean value.

This only affects PlzNavigate. When a same-site navigation should reuse the current WebUI, in RFHM::GetFrameHostForNavigation the (shadowing) method-local |should_reuse_web_ui_| would be set to true and the speculative WebUI would (correctly) not be created. But later on as the instance-level |should_reuse_web_ui_| defaults to false, RFHM::CommitPending would try to get the |speculative_web_ui_| which would not be set and *could* cause a crash. As of now the crash doesn't happen because CommitPending is not called when the current RFH is kept for the navigation.

BUG=376094
Committed: https://crrev.com/6454936159376e371fd27c2a14e27b18494850d7
Cr-Commit-Position: refs/heads/master@{#322485}

[carlosk, 2015-03-25 12:24:29]

carlosk@chromium.org changed reviewers:
+ clamy@chromium.org, creis@chromium.org, fdegans@chromium.org

[carlosk, 2015-03-25 12:24:29]

carlosk@chromium.org changed required reviewers:
+ creis@chromium.org

[carlosk, 2015-03-25 12:24:30]

creis: PTAL

clamy, fdegans: FYI

This fixes that variable shadowing bug I found in http://crrev.com/967383002
affecting PlzNavigate only.

I couldn't find a content_unittest that actually fails due to this bug. The
closest test I *think* could be adapted to trigger it is
RenderFrameHostManagerTest::WebUIInNewTab. Should I try to adapt it or is this
too little of a change for that?

[Charlie Reis, 2015-03-26 03:21:55]

LGTM.

A test would be nice (here or in a followup) if it's easy to add, but I won't
block this for it.  From a quick glance at the code, I'm guessing it means there
are some PlzNavigate cases where the WebUI is not reused when it should be?  (In
other words, if you click a link from one chrome:// URL to another, the new one
might not work.)

Let's at least put a TEST line in the CL description saying a manual way to
confirm the fix, if there is one.

[clamy, 2015-03-26 10:52:54]

Lgtm! Thanks for catching this.

[carlosk, 2015-03-26 15:57:36]

Thanks.

On 2015/03/26 03:21:55, Charlie Reis wrote:
> LGTM.
> 
> A test would be nice (here or in a followup) if it's easy to add, but I won't
> block this for it.  From a quick glance at the code, I'm guessing it means
there
> are some PlzNavigate cases where the WebUI is not reused when it should be? 
(In
> other words, if you click a link from one chrome:// URL to another, the new
one
> might not work.)
> 
> Let's at least put a TEST line in the CL description saying a manual way to
> confirm the fix, if there is one.

I forgot to further explain the consequences of this bug here and only added to
a still not submitted comment in the original CR. Sorry about that and it
follows below.

This only affects PlzNavigate when a same-site navigation should reuse the
current WebUI. In RFHM::GetFrameHostForNavigation the (shadowing) method-local
|should_reuse_web_ui_| would be set to true and the speculative WebUI would
(correctly) not be created. But later on as the instance-level
|should_reuse_web_ui_| defaults to false, RFHM::CommitPending would try to get
the |speculative_web_ui_| which would not be set and cause a crash.

I couldn't cause a manual repro of this because PlzNavigate can't still properly
load chrome:// pages so I'm trying to create an automated test for it now.

[carlosk, 2015-03-26 18:06:01]

On 2015/03/26 15:57:36, carlosk wrote:
> Thanks.
> 
> On 2015/03/26 03:21:55, Charlie Reis wrote:
> > LGTM.
> > 
> > A test would be nice (here or in a followup) if it's easy to add, but I
won't
> > block this for it.  From a quick glance at the code, I'm guessing it means
> there
> > are some PlzNavigate cases where the WebUI is not reused when it should be? 
> (In
> > other words, if you click a link from one chrome:// URL to another, the new
> one
> > might not work.)
> > 
> > Let's at least put a TEST line in the CL description saying a manual way to
> > confirm the fix, if there is one.
> 
> I forgot to further explain the consequences of this bug here and only added
to
> a still not submitted comment in the original CR. Sorry about that and it
> follows below.
> 
> This only affects PlzNavigate when a same-site navigation should reuse the
> current WebUI. In RFHM::GetFrameHostForNavigation the (shadowing) method-local
> |should_reuse_web_ui_| would be set to true and the speculative WebUI would
> (correctly) not be created. But later on as the instance-level
> |should_reuse_web_ui_| defaults to false, RFHM::CommitPending would try to get
> the |speculative_web_ui_| which would not be set and cause a crash.
> 
> I couldn't cause a manual repro of this because PlzNavigate can't still
properly
> load chrome:// pages so I'm trying to create an automated test for it now.

After investigating how and why WebUI are created in tests and talking with
clamy@ I was able to add a test to RenderFrameHostManagerTest that reproduces
this. To my surprise the crash didn't happen, so more investigation ensued.

I finally discovered what I think is another bug. In
RFHM::CommitPendingIfNecessary we run CommitPending iff the RFH that committed
was the speculative one without checking anything regarding the speculative
WebUI. So in this case as we're reusing the current RFH, there's no speculative
one and so CommitPending doesn't happen, avoiding the crash.

Now this would be a problem in the case where we are reusing the current RFH but
the current WebUI should not be reused. In that case there will be one set as
speculative and at this point it will simply be ignored. Looking at the code it
seems like this is a valid navigation case, am I right?

[Charlie Reis, 2015-03-26 19:00:27]

I can't think of a case when we would stay in the same RFH and not use the same
WebUI.  If the WebUI object is no longer valid, we should have done a RFH swap.

I would suggest committing this CL as is (no test) with a little more info in
the CL description about how it would have impacted things.

If you do come across a case where the second issue matters, we can fix that in
a separate CL.

[carlosk, 2015-03-26 19:48:53]

On 2015/03/26 19:00:27, Charlie Reis wrote:
> I can't think of a case when we would stay in the same RFH and not use the
same
> WebUI.  If the WebUI object is no longer valid, we should have done a RFH
swap.
> 
> I would suggest committing this CL as is (no test) with a little more info in
> the CL description about how it would have impacted things.
> 
> If you do come across a case where the second issue matters, we can fix that
in
> a separate CL.

OK, I updated the description and will commit it as as.

I'll create another change to fix other issues I found which will make the
proper testing of this problem possible.

[carlosk, 2015-03-26 19:49:07]

The CQ bit was checked by carlosk@chromium.org

[commit-bot: I haz the power, 2015-03-26 19:49:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1012863004/1

[commit-bot: I haz the power, 2015-03-26 22:42:00]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-03-26 22:42:43]

Patchset 1 (id:??) landed as
https://crrev.com/6454936159376e371fd27c2a14e27b18494850d7
Cr-Commit-Position: refs/heads/master@{#322485}