Fix how captive portals track which page is loading.

Fix how captive portals track which page is loading.

The old code incorrectly assumed RenderView frame IDs
were globally unique, and used them to track main frame
reloads.

The corrected version tracks the provisional RenderViewHost
instead.

The new code also will now never send two LoadStart events
to the CaptivePortalReloader without an intervening load
committed/aborted event.

R=cbentzel@chromium.org
BUG=87100, 115487
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=151941

[mmenke, 2012-08-10 15:41:43]

I actually could never get the old code to fail in a browser or manual test - I
did get events in an order that would have failed once, but could never
reproduce it.  That having been said, at least how I tracked redirects was
incorrect.

I think it still might be possible for the original page to be in the middle of
committing an SSL timeout when a new navigation starts, and the timeout to be
committed before the old navigation is cancelled, which this code won't actually
work for, but the odds of that are so small I don't think it's worth worrying
about.  Any fix would require tracking the old and new navigations
independently, which I suppose is doable, but just seems to add too much
complexity.  If the original load times out, we should already have had more
than enough time to issue the captive portal check, and for the captive portal
to respond, anyways.

There may also be races in terms of the IO process informing the UI process of
redirects - if you interrupt one pending load with another that uses the same
process (And thus same RenderView/RenderViewHost), it may be possible to be
informed of a redirect for the original load after the new navigation has
started.  I don't think there's actually any way to distinguish one main frame
load from another, so I'm not even sure there's anything the browser process can
actually do about this.  Also doesn't seem too likely.

[mmenke, 2012-08-10 16:48:12]

Oh, should add that comparing set 3 to set 4 may make reviewing the browser
tests changes a little easier - I took one test, and turned it into 3.  The
tests all pass even without the rest of the CL applied, but figured I may as
well check a few more cases, though the tests are not exhaustive (Don't check
just 1 and 3 being the same site, or just 2 and 3 being the same site, though
both cases do pass if I test them).

[cbentzel, 2012-08-15 17:00:38]

Going through the tests some more, but the core logic looks correct.

http://codereview.chromium.org/10837146/diff/5014/chrome/browser/captive_port...
File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right):

http://codereview.chromium.org/10837146/diff/5014/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:70: // the previous
load.  Link Doctor pages act like error page loads in a
"Link Doctor pages act like error page loads in a row" is unclear to me - is
this supposed to be "act like two error page loads in a row"?

http://codereview.chromium.org/10837146/diff/5014/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:71: // row.  The
second time, provisional_render_view_host_ will be NULL.
Why will provisional_render_view_host_ be NULL? Will it commit the first error
page?

Is the sequence:

  - provisional load on original page
  - provisional load of first error page
  - commit of first error page
  - provisional load of link doctor page
  - commit of link doctor page

http://codereview.chromium.org/10837146/diff/5014/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:104: // Send
information about the new load.
Why do you need to do this? Just for stats?

http://codereview.chromium.org/10837146/diff/5014/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:182:
provisional_render_view_host_ = NULL;
Should you share this logic with the error_code == net::ERR_ABORTED case in
DidFailProvisionalLoad()?

http://codereview.chromium.org/10837146/diff/5014/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:197: if
(resource_type != ResourceType::MAIN_FRAME ||
Would child_id ever be -1 here?

http://codereview.chromium.org/10837146/diff/5014/chrome/browser/captive_port...
File chrome/browser/captive_portal/captive_portal_tab_helper.h (right):

http://codereview.chromium.org/10837146/diff/5014/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.h:41: // this may not be
perfectly accurate in all cases, it should be good enough.
Can you be clearer in this comment about a case where there would be more than
one RVH with a provisional load?

http://codereview.chromium.org/10837146/diff/5014/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.h:89: protected:
Why are these protected instead of private?

http://codereview.chromium.org/10837146/diff/5014/chrome/browser/captive_port...
File chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc
(right):

http://codereview.chromium.org/10837146/diff/5014/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc:65: virtual
int GetProvisionalChildID() const OVERRIDE {
This is rather gross. I assume that it is difficult to actually create a
RenderViewHost in a unit test?

[mmenke, 2012-08-16 14:30:03]

https://chromiumcodereview.appspot.com/10837146/diff/5014/chrome/browser/capt...
File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right):

https://chromiumcodereview.appspot.com/10837146/diff/5014/chrome/browser/capt...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:70: // the previous
load.  Link Doctor pages act like error page loads in a
On 2012/08/15 17:00:38, cbentzel wrote:
> "Link Doctor pages act like error page loads in a row" is unclear to me - is
> this supposed to be "act like two error page loads in a row"?

Correct.  "two" added

https://chromiumcodereview.appspot.com/10837146/diff/5014/chrome/browser/capt...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:71: // row.  The
second time, provisional_render_view_host_ will be NULL.
On 2012/08/15 17:00:38, cbentzel wrote:
> Why will provisional_render_view_host_ be NULL? Will it commit the first error
> page?
> 
> Is the sequence:
> 
>   - provisional load on original page
>   - provisional load of first error page
>   - commit of first error page
>   - provisional load of link doctor page
>   - commit of link doctor page

It's:

- provisional load
- provisional load fails  // I get the error code here
- provisional load of first error page (is_error_page == true)
- commit of first error page  // I use the error code here, and set
provisional_render_view_host_ to NULL (And clear the error code).
- provisional load of link doctor page (is_error_page == true)
- commit of link doctor page

https://chromiumcodereview.appspot.com/10837146/diff/5014/chrome/browser/capt...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:104: // Send
information about the new load.
On 2012/08/15 17:00:38, cbentzel wrote:
> Why do you need to do this? Just for stats?

To make TabReloader's interface a bit better defined:
TabReloader::OnLoadCommitted corresponds to real commits.  It has no other
purpose.  If you prefer, I can document the omission and remove this code.

https://chromiumcodereview.appspot.com/10837146/diff/5014/chrome/browser/capt...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:182:
provisional_render_view_host_ = NULL;
On 2012/08/15 17:00:38, cbentzel wrote:
> Should you share this logic with the error_code == net::ERR_ABORTED case in
> DidFailProvisionalLoad()?

Done.  Went ahead and used the same function for the other two abort cases as
well, though they don't actually need the extra two lines, they don't hurt.

https://chromiumcodereview.appspot.com/10837146/diff/5014/chrome/browser/capt...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:197: if
(resource_type != ResourceType::MAIN_FRAME ||
On 2012/08/15 17:00:38, cbentzel wrote:
> Would child_id ever be -1 here?

Since the id is actually used to lookup the RenderViewHost and send us the
message, I don't see how it could be.  That having been said, I've inlined
GetProvisionalChildID, since it's no longer needed.

https://chromiumcodereview.appspot.com/10837146/diff/5014/chrome/browser/capt...
File chrome/browser/captive_portal/captive_portal_tab_helper.h (right):

https://chromiumcodereview.appspot.com/10837146/diff/5014/chrome/browser/capt...
chrome/browser/captive_portal/captive_portal_tab_helper.h:41: // this may not be
perfectly accurate in all cases, it should be good enough.
On 2012/08/15 17:00:38, cbentzel wrote:
> Can you be clearer in this comment about a case where there would be more than
> one RVH with a provisional load?

Done.

https://chromiumcodereview.appspot.com/10837146/diff/5014/chrome/browser/capt...
chrome/browser/captive_portal/captive_portal_tab_helper.h:89: protected:
On 2012/08/15 17:00:38, cbentzel wrote:
> Why are these protected instead of private?

Since I had to override GetProvisionalChildID for unit tests anyways, I had the
subclass friend the test fixture, rather than the CaptivePortalTabHelper itself.
 I was thinking this would make it slightly clearer what functions were used by
the tests.

Since this apparently wasn't true, and the tests no longer need to use a
subclass, I've gone ahead and removed the protected section.

https://chromiumcodereview.appspot.com/10837146/diff/5014/chrome/browser/capt...
File chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10837146/diff/5014/chrome/browser/capt...
chrome/browser/captive_portal/captive_portal_tab_helper_unittest.cc:65: virtual
int GetProvisionalChildID() const OVERRIDE {
On 2012/08/15 17:00:38, cbentzel wrote:
> This is rather gross. I assume that it is difficult to actually create a
> RenderViewHost in a unit test?

Oh, that works.  For some reason, I was under the impression that the test
RenderViewHosts didn't come with their own RenderProcessHosts.

[cbentzel, 2012-08-16 19:15:04]

LGTM

http://codereview.chromium.org/10837146/diff/5014/chrome/browser/captive_port...
File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right):

http://codereview.chromium.org/10837146/diff/5014/chrome/browser/captive_port...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:104: // Send
information about the new load.
On 2012/08/16 14:30:03, Matt Menke wrote:
> On 2012/08/15 17:00:38, cbentzel wrote:
> > Why do you need to do this? Just for stats?
> 
> To make TabReloader's interface a bit better defined:
> TabReloader::OnLoadCommitted corresponds to real commits.  It has no other
> purpose.  If you prefer, I can document the omission and remove this code.

I think this is fine.

http://codereview.chromium.org/10837146/diff/13018/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right):

http://codereview.chromium.org/10837146/diff/13018/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:213: bool
CaptivePortalTabHelper::IsLoginTab() const {
Nit: This doesn't match ordering in header.

[mmenke, 2012-08-16 19:18:34]

Thanks for the feedback.

http://codereview.chromium.org/10837146/diff/13018/chrome/browser/captive_por...
File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right):

http://codereview.chromium.org/10837146/diff/13018/chrome/browser/captive_por...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:213: bool
CaptivePortalTabHelper::IsLoginTab() const {
On 2012/08/16 19:15:05, cbentzel wrote:
> Nit: This doesn't match ordering in header.

Done.

[mmenke, 2012-08-16 19:28:00]

Gah!  Didn't use the CQ.  Dunno what I was thinking.

It's already compiled on Linux on the tree, and since I only moved a function
since the last tryjob, I'll just keep an eye on the tree rather than reverting.