Only commit cookie changes in prerenders after a prerender is shown

Only commit cookie changes in prerenders after a prerender is shown
Will create a PrerenderCookieStore for each prerender, retaining all cookie
operations of a prerender until the prerender is shown to the user.
Forces prerenders to be in a new render process by themselves for this to work.
BUG=371003
R=creis@chromium.org, davidben@chromium.org, erikwright@chromium.org, jam@chromium.org, jochen@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269798

[tburkard, 2014-04-10 15:03:13]

First draft. I will add testing to this change, but if you could already start
taking a look, that'd be great.

erikwright: net/cookies
jam: content
davidben, mmenke: chrome/browser/prerender and entire change
jochen: rubberstamp ok everything not mentioned before

Thanks.

[tburkard, 2014-04-16 13:05:07]

(ping)

Thanks.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[erikwright (departed), 2014-04-16 14:17:56]

Overall, looks reasonable.

There is a lot of code here, and it seems to handle at least three different
things:

(1) Enforcing limits on what goes to a specific render process.
(2) Implementing support for swapping out a cookie store for a given renderer.
(3) The actual PrerenderCookieStore and the other logic around deciding whether
to use it.

I would split this into at least 2 (1, and 2+3) or even 3 CLs. Hopefully each
will touch half or fewer files than this one. And it will be easier for the
different reviewers.

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_contents.cc:341: // Create a Prerender Cookie
Store for the prerender through the
I don't understand the relationship between this content and the code that
follows.

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_cookie_store.cc (right):

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.cc:24:
DCHECK(default_cookie_monster_->loaded());
This requirement needs to be documented in the header.

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.cc:64:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
Does the IPC system reponding to the renderer, in fact, operate on the IO thread
and invoke the cookie store on the IO thread?

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.cc:124:
cookie_ops_.push_back(op);
This line completely violated my expectations based on the name of this method.

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.cc:138: // If we haven't
returned yet, we the key has not been copied yet,
rm 'we'

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.cc:145: DCHECK(copy_success);
If it 'must' succeed, why return a result boolean at all?

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.cc:153:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
Why post a separate task here to the same thread?

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.cc:188: default:
I'm told that 'default:NOTREACHED();' is not required and is actually harmful
because some of our compilers will actually fail when there is an unhandled
ENUM, but the 'default' makes them handled and thus thwarts this check.

In other words, without default, when you add a new enum but don't add a case,
you would fail at compile time. With the 'default' you will only fail at
runtime.

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.cc:196: changes_cookie_monster_
= NULL;
Personally I'm in favor of reducing redundant state. The boolean could be done
away with and you could just check if the changes_cookie_monster_ is NULL.

The boolean is more readable, but it introduces the possibility of getting out
of sync with the other state (hence needing a DCHECK above, etc.).

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.cc:214: if (read_keys_.find(key)
== read_keys_.end() &&
in base/stl_utils.h there is ContainsKey(collection)

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.cc:220: // cancel the prerender
if necessary.
'if necessary' or always?

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_cookie_store.h (right):

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.h:25: // is called, the changes
will be applied to the  default cookie monster,
remove double space

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.h:27: // default cookie monster.
Does this cookie store remain in place for the lifetime of the renderer, or is
it expected to be swapped out at some point?

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.h:28: class PrerenderCookieStore
: public net::CookieStore {
IWYU cookie_store.h

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.h:34:
PrerenderCookieStore(scoped_refptr<net::CookieMonster> default_cookie_store_,
IWYU ref_counted.h

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.h:37: virtual void
SetCookieWithOptionsAsync(
// CookieStore implementation

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.h:41: const SetCookiesCallback&
callback) OVERRIDE;
IWYU compiler_specific.h

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.h:78: // into forwarding mode.
To be called on the UI thread.
The comment says UI. The code says IO.

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.h:93: };
blank after };

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.h:135: base::Closure
cookie_conflict_cb_;
IWYU callback.h

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.h:138: bool cookie_conflict_;
DISALLOW_COPY_AND_ASSIGN
IWYU macros.h

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:1298: // (used to only commit
cookie changes once a prerender is shown) would
I would restrict this to saying that the PrerenderCookieStore requires the
underlying CookieMonster to be fully loaded, and then put the explanation as to
why in the header for PrerenderCookieStore.

I would probably just remove the other justification about initial startup. If
that's really relevant, you might want to be looking for other signals to
disable prerendering during early browser lifetime.

[erikwright (departed), 2014-04-16 14:18:38]

On 2014/04/16 14:17:56, erikwright wrote:
> Overall, looks reasonable.
> 
> There is a lot of code here, and it seems to handle at least three different
> things:
> 
> (1) Enforcing limits on what goes to a specific render process.
> (2) Implementing support for swapping out a cookie store for a given renderer.
> (3) The actual PrerenderCookieStore and the other logic around deciding
whether
> to use it.
> 
> I would split this into at least 2 (1, and 2+3) or even 3 CLs. Hopefully each
> will touch half or fewer files than this one. And it will be easier for the
> different reviewers.
> 
>
https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
> File chrome/browser/prerender/prerender_contents.cc (right):
> 
>
https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
> chrome/browser/prerender/prerender_contents.cc:341: // Create a Prerender
Cookie
> Store for the prerender through the
> I don't understand the relationship between this content and the code that
> follows.
> 
>
https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
> File chrome/browser/prerender/prerender_cookie_store.cc (right):
> 
>
https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
> chrome/browser/prerender/prerender_cookie_store.cc:24:
> DCHECK(default_cookie_monster_->loaded());
> This requirement needs to be documented in the header.
> 
>
https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
> chrome/browser/prerender/prerender_cookie_store.cc:64:
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
> Does the IPC system reponding to the renderer, in fact, operate on the IO
thread
> and invoke the cookie store on the IO thread?
> 
>
https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
> chrome/browser/prerender/prerender_cookie_store.cc:124:
> cookie_ops_.push_back(op);
> This line completely violated my expectations based on the name of this
method.
> 
>
https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
> chrome/browser/prerender/prerender_cookie_store.cc:138: // If we haven't
> returned yet, we the key has not been copied yet,
> rm 'we'
> 
>
https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
> chrome/browser/prerender/prerender_cookie_store.cc:145: DCHECK(copy_success);
> If it 'must' succeed, why return a result boolean at all?
> 
>
https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
> chrome/browser/prerender/prerender_cookie_store.cc:153:
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
> Why post a separate task here to the same thread?
> 
>
https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
> chrome/browser/prerender/prerender_cookie_store.cc:188: default:
> I'm told that 'default:NOTREACHED();' is not required and is actually harmful
> because some of our compilers will actually fail when there is an unhandled
> ENUM, but the 'default' makes them handled and thus thwarts this check.
> 
> In other words, without default, when you add a new enum but don't add a case,
> you would fail at compile time. With the 'default' you will only fail at
> runtime.
> 
>
https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
> chrome/browser/prerender/prerender_cookie_store.cc:196:
changes_cookie_monster_
> = NULL;
> Personally I'm in favor of reducing redundant state. The boolean could be done
> away with and you could just check if the changes_cookie_monster_ is NULL.
> 
> The boolean is more readable, but it introduces the possibility of getting out
> of sync with the other state (hence needing a DCHECK above, etc.).
> 
>
https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
> chrome/browser/prerender/prerender_cookie_store.cc:214: if
(read_keys_.find(key)
> == read_keys_.end() &&
> in base/stl_utils.h there is ContainsKey(collection)
> 
>
https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
> chrome/browser/prerender/prerender_cookie_store.cc:220: // cancel the
prerender
> if necessary.
> 'if necessary' or always?
> 
>
https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
> File chrome/browser/prerender/prerender_cookie_store.h (right):
> 
>
https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
> chrome/browser/prerender/prerender_cookie_store.h:25: // is called, the
changes
> will be applied to the  default cookie monster,
> remove double space
> 
>
https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
> chrome/browser/prerender/prerender_cookie_store.h:27: // default cookie
monster.
> Does this cookie store remain in place for the lifetime of the renderer, or is
> it expected to be swapped out at some point?
> 
>
https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
> chrome/browser/prerender/prerender_cookie_store.h:28: class
PrerenderCookieStore
> : public net::CookieStore {
> IWYU cookie_store.h
> 
>
https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
> chrome/browser/prerender/prerender_cookie_store.h:34:
> PrerenderCookieStore(scoped_refptr<net::CookieMonster> default_cookie_store_,
> IWYU ref_counted.h
> 
>
https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
> chrome/browser/prerender/prerender_cookie_store.h:37: virtual void
> SetCookieWithOptionsAsync(
> // CookieStore implementation
> 
>
https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
> chrome/browser/prerender/prerender_cookie_store.h:41: const
SetCookiesCallback&
> callback) OVERRIDE;
> IWYU compiler_specific.h
> 
>
https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
> chrome/browser/prerender/prerender_cookie_store.h:78: // into forwarding mode.
> To be called on the UI thread.
> The comment says UI. The code says IO.
> 
>
https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
> chrome/browser/prerender/prerender_cookie_store.h:93: };
> blank after };
> 
>
https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
> chrome/browser/prerender/prerender_cookie_store.h:135: base::Closure
> cookie_conflict_cb_;
> IWYU callback.h
> 
>
https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
> chrome/browser/prerender/prerender_cookie_store.h:138: bool cookie_conflict_;
> DISALLOW_COPY_AND_ASSIGN
> IWYU macros.h
> 
>
https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
> File chrome/browser/prerender/prerender_manager.cc (right):
> 
>
https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
> chrome/browser/prerender/prerender_manager.cc:1298: // (used to only commit
> cookie changes once a prerender is shown) would
> I would restrict this to saying that the PrerenderCookieStore requires the
> underlying CookieMonster to be fully loaded, and then put the explanation as
to
> why in the header for PrerenderCookieStore.
> 
> I would probably just remove the other justification about initial startup. If
> that's really relevant, you might want to be looking for other signals to
> disable prerendering during early browser lifetime.

I should explicitly state that I stopped at prerender_manager for now.

[tburkard, 2014-04-22 12:20:59]

Addressed all of Erik's feedback.

Erik, I agree this is a big change, and splitting it up might make sense.
However, in this review, people might ask me to change stuff around which would
affect multiple ones of the separate CLs simultaneously. Therefore, I think it's
easier to just have one change for now, and work off that. Perhaps I can split
it up later before I commit it, but as long as people have not provided overall
feedback yet, keeping it as a single CL makes things easier, I think.

Thanks.

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_contents.cc:341: // Create a Prerender Cookie
Store for the prerender through the
On 2014/04/16 14:17:56, erikwright wrote:
> I don't understand the relationship between this content and the code that
> follows.

Clarified the comment. The method AddPrerender will add the prerender to be
tracked in the prerender tracker, and create the cookie store.
Is this sufficient?

Thanks.

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_cookie_store.cc (right):

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.cc:24:
DCHECK(default_cookie_monster_->loaded());
On 2014/04/16 14:17:56, erikwright wrote:
> This requirement needs to be documented in the header.

Done.

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.cc:64:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
Yes, that's my understanding, and based on the fact that the DCHECK does not
trigger :)

On 2014/04/16 14:17:56, erikwright wrote:
> Does the IPC system reponding to the renderer, in fact, operate on the IO
thread
> and invoke the cookie store on the IO thread?

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.cc:124:
cookie_ops_.push_back(op);
On 2014/04/16 14:17:56, erikwright wrote:
> This line completely violated my expectations based on the name of this
method.

Updated the name of this method.

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.cc:138: // If we haven't
returned yet, we the key has not been copied yet,
On 2014/04/16 14:17:56, erikwright wrote:
> rm 'we'

Done.

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.cc:145: DCHECK(copy_success);
On 2014/04/16 14:17:56, erikwright wrote:
> If it 'must' succeed, why return a result boolean at all?

To specifically test for it. Also, I think that a general purpose cookie monster
requiring the conditions with a DCHECK not appropriate. I thought it was better
if the cookie monster just returns false when the conditions are violated, and
then treat the case here using DCHECK.

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.cc:153:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2014/04/16 14:17:56, erikwright wrote:
> Why post a separate task here to the same thread?

Whoops, had changed the semantics back and forth a few times, fixed.

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.cc:188: default:
On 2014/04/16 14:17:56, erikwright wrote:
> I'm told that 'default:NOTREACHED();' is not required and is actually harmful
> because some of our compilers will actually fail when there is an unhandled
> ENUM, but the 'default' makes them handled and thus thwarts this check.
> 
> In other words, without default, when you add a new enum but don't add a case,
> you would fail at compile time. With the 'default' you will only fail at
> runtime.

Like this?

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.cc:196: changes_cookie_monster_
= NULL;
On 2014/04/16 14:17:56, erikwright wrote:
> Personally I'm in favor of reducing redundant state. The boolean could be done
> away with and you could just check if the changes_cookie_monster_ is NULL.
> 
> The boolean is more readable, but it introduces the possibility of getting out
> of sync with the other state (hence needing a DCHECK above, etc.).

I agree with you on redundant state.

However, I think this makes it much easier to read, and if it is only an extra
DCHECK, I think that's worth it.

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.cc:214: if (read_keys_.find(key)
== read_keys_.end() &&
On 2014/04/16 14:17:56, erikwright wrote:
> in base/stl_utils.h there is ContainsKey(collection)

Done.

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.cc:220: // cancel the prerender
if necessary.
On 2014/04/16 14:17:56, erikwright wrote:
> 'if necessary' or always?

Clarified.

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_cookie_store.h (right):

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.h:25: // is called, the changes
will be applied to the  default cookie monster,
On 2014/04/16 14:17:56, erikwright wrote:
> remove double space

Done.

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.h:27: // default cookie monster.
On 2014/04/16 14:17:56, erikwright wrote:
> Does this cookie store remain in place for the lifetime of the renderer, or is
> it expected to be swapped out at some point?

Clarified the comment.

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.h:28: class PrerenderCookieStore
: public net::CookieStore {
On 2014/04/16 14:17:56, erikwright wrote:
> IWYU cookie_store.h

Done.

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.h:34:
PrerenderCookieStore(scoped_refptr<net::CookieMonster> default_cookie_store_,
On 2014/04/16 14:17:56, erikwright wrote:
> IWYU ref_counted.h

Done.

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.h:37: virtual void
SetCookieWithOptionsAsync(
On 2014/04/16 14:17:56, erikwright wrote:
> // CookieStore implementation

Done.

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.h:41: const SetCookiesCallback&
callback) OVERRIDE;
On 2014/04/16 14:17:56, erikwright wrote:
> IWYU compiler_specific.h

Done.

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.h:78: // into forwarding mode.
To be called on the UI thread.
On 2014/04/16 14:17:56, erikwright wrote:
> The comment says UI. The code says IO.

Done.

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.h:93: };
On 2014/04/16 14:17:56, erikwright wrote:
> blank after };

Done.

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.h:135: base::Closure
cookie_conflict_cb_;
On 2014/04/16 14:17:56, erikwright wrote:
> IWYU callback.h

Done.

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.h:138: bool cookie_conflict_;
On 2014/04/16 14:17:56, erikwright wrote:
> DISALLOW_COPY_AND_ASSIGN
> IWYU macros.h

Done.

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/233353003/diff/120001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:1298: // (used to only commit
cookie changes once a prerender is shown) would
On 2014/04/16 14:17:56, erikwright wrote:
> I would restrict this to saying that the PrerenderCookieStore requires the
> underlying CookieMonster to be fully loaded, and then put the explanation as
to
> why in the header for PrerenderCookieStore.
> 
> I would probably just remove the other justification about initial startup. If
> that's really relevant, you might want to be looking for other signals to
> disable prerendering during early browser lifetime.

Done.

[mmenke, 2014-04-22 14:22:58]

I'm going to defer to David and Erik on this one - Erik knows cookies, and David
knows prerender.  And I'm becoming less and less familiar with the prerendering
code.

[davidben, 2014-04-22 21:20:42]

Looks reasonable so far. Some initial comments below. Also try bots are unhappy
about some test.

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/net/cook...
File chrome/browser/net/cookie_store_util.cc (right):

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/net/cook...
chrome/browser/net/cookie_store_util.cc:52: }
Nit: newline

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/prerende...
chrome/browser/prerender/prerender_contents.cc:351:
FINAL_STATUS_COOKIE_CONFLICT));
This is assuming a 1 prerender <-> 1 process already, right? What if you made
the callback something like

  base::Bind(&PrerenderContents::Destroy, AsWeakPtr(),
FINAL_STATUS_COOKIE_CONFLICT)

and make PrerenderContents weakptr-able. Might be simpler than having to go back
and iterate over processes and such.

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_cookie_store.cc (right):

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.cc:176: break;
Does replaying reads do anything? I wonder if having ops for them is even
necessary since you've got read_keys_ anyway.

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.cc:212: // cancelled for some
othe reason).
Nit: othe -> others

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.cc:215: BrowserThread::PostTask(
There's a race here. If a conflict happens right as a swap is happening, the UI
thread signal can happen after the swap occurs. (Canonical swap state lives on
the UI thread.)

I suspect it doesn't matter since you probably also get races if two requests
happen at the same time and both change cookies. But I wanted to point it out.

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:1335:
IsLocalPredictorPrerenderAlwaysControlEnabled()));
Hrm. Is this part of a different change?

[tburkard, 2014-04-23 14:52:54]

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/net/cook...
File chrome/browser/net/cookie_store_util.cc (right):

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/net/cook...
chrome/browser/net/cookie_store_util.cc:52: }
On 2014/04/22 21:20:43, David Benjamin wrote:
> Nit: newline

Done.

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/prerende...
chrome/browser/prerender/prerender_contents.cc:351:
FINAL_STATUS_COOKIE_CONFLICT));
On 2014/04/22 21:20:43, David Benjamin wrote:
> This is assuming a 1 prerender <-> 1 process already, right? What if you made
> the callback something like
> 
>   base::Bind(&PrerenderContents::Destroy, AsWeakPtr(),
> FINAL_STATUS_COOKIE_CONFLICT)
> 
> and make PrerenderContents weakptr-able. Might be simpler than having to go
back
> and iterate over processes and such.

I'd prefer to not make this weakptr-able. It's a pretty small loop iterating
over the prerender contents and destroying it, if that's ok?

Thanks.

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_cookie_store.cc (right):

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.cc:176: break;
On 2014/04/22 21:20:43, David Benjamin wrote:
> Does replaying reads do anything? I wonder if having ops for them is even
> necessary since you've got read_keys_ anyway.

Yes, because it updates the timestamps on when cookies where last accessed.

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.cc:212: // cancelled for some
othe reason).
On 2014/04/22 21:20:43, David Benjamin wrote:
> Nit: othe -> others

Done.

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.cc:215: BrowserThread::PostTask(
On 2014/04/22 21:20:43, David Benjamin wrote:
> There's a race here. If a conflict happens right as a swap is happening, the
UI
> thread signal can happen after the swap occurs. (Canonical swap state lives on
> the UI thread.)
> 
> I suspect it doesn't matter since you probably also get races if two requests
> happen at the same time and both change cookies. But I wanted to point it out.

Exactly. It's the same situation as if two tabs try to perform conflicting
operations on the cookie store at about the same time.

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:1335:
IsLocalPredictorPrerenderAlwaysControlEnabled()));
On 2014/04/22 21:20:43, David Benjamin wrote:
> Hrm. Is this part of a different change?

Yes, this is an error that should be fixed. Should I make a new change for this?
I noticed it as I was doing these changes, so I thought I'd just include it
here.

Thanks.

[davidben, 2014-04-23 23:58:24]

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/prerende...
chrome/browser/prerender/prerender_contents.cc:351:
FINAL_STATUS_COOKIE_CONFLICT));
On 2014/04/23 14:52:54, tburkard wrote:
> On 2014/04/22 21:20:43, David Benjamin wrote:
> > This is assuming a 1 prerender <-> 1 process already, right? What if you
made
> > the callback something like
> > 
> >   base::Bind(&PrerenderContents::Destroy, AsWeakPtr(),
> > FINAL_STATUS_COOKIE_CONFLICT)
> > 
> > and make PrerenderContents weakptr-able. Might be simpler than having to go
> back
> > and iterate over processes and such.
> 
> I'd prefer to not make this weakptr-able. It's a pretty small loop iterating
> over the prerender contents and destroying it, if that's ok?
> 
> Thanks.

I think I'd prefer the weakptr version, just because it seems simpler, but it's
not a strong preference so *shrug*. I'm not sure we can keep prerender
multi-process-unaware forever (What happens if a prerender has a cross-origin
iframe post site-isolation? Do we find some way to defer it or allow it?), and
the other version at least keeps everything after the callback agnostic to this,
so we'd just have to attach the cookie store to all the relevant processes.

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_cookie_store.cc (right):

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.cc:215: BrowserThread::PostTask(
On 2014/04/23 14:52:54, tburkard wrote:
> On 2014/04/22 21:20:43, David Benjamin wrote:
> > There's a race here. If a conflict happens right as a swap is happening, the
> UI
> > thread signal can happen after the swap occurs. (Canonical swap state lives
on
> > the UI thread.)
> > 
> > I suspect it doesn't matter since you probably also get races if two
requests
> > happen at the same time and both change cookies. But I wanted to point it
out.
> 
> Exactly. It's the same situation as if two tabs try to perform conflicting
> operations on the cookie store at about the same time.

Cool. Maybe put a comment to that effect here just so the reason the race is
okay is documented.

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:1335:
IsLocalPredictorPrerenderAlwaysControlEnabled()));
On 2014/04/23 14:52:54, tburkard wrote:
> On 2014/04/22 21:20:43, David Benjamin wrote:
> > Hrm. Is this part of a different change?
> 
> Yes, this is an error that should be fixed. Should I make a new change for
this?
> I noticed it as I was doing these changes, so I thought I'd just include it
> here.
> 
> Thanks.

Ah. Personally I tend to err towards splitting things, but I don't really care.
(I guess it'll get in sooner since this guy still wants a test and such.)

[jochen (gone - plz use gerrit), 2014-04-24 12:47:20]

https://codereview.chromium.org/233353003/diff/220001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/233353003/diff/220001/content/public/browser/...
content/public/browser/content_browser_client.h:245: // existing renderer in an
existing host.
that's kinda strange. I would have expected that IsSuitableHost is only used
when we want to launch a new view in the given host (i.e.
adding_new_render_view_to_existing_process should always be true)?

[jochen (gone - plz use gerrit), 2014-04-24 12:49:48]

what happens if you have e..g third-party cookies blocked, a page is
prerenderer, you navigate to that page, open the "Show cookies and other site
data" dialog via the blocked cookies icon in the omnibox, then select one of the
accepted cookies, and click remove

Does it correctly remove the cookie?

[tburkard, 2014-04-24 13:03:59]

David, I will address your concerns later.

Jochen, re: your third party cookie block issue, can you describe the scenario
more detailed?
you prerender a page, and then navigate to the prerendered page? Or to a
different URL with the same 3rd party cookie?
And then you remove an accepted cookie?

Regardless of the scenario, it should work, for the following reasons (but I'd
be happy to test once you specify how exactly):
- before a prerender is swapped, will cancel prerender on conflicts (eg another
tab/ui sets or clears a cookie)
- at time of swap, all cookie changes are applied to cookie store and cookie
store delegate is informed (notice it is NOT informed for cookies set/changed
during prerender, since the cookie monster for the deltas does not have a
delegate), just as it would have if the page loaded
- after the swap, it's just like operating on the existing cookie store, so
there is no difference any more.

Based on that, I think the scenario(s) you are describing should work.

Please clarify.

Thanks.

https://codereview.chromium.org/233353003/diff/220001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/233353003/diff/220001/content/public/browser/...
content/public/browser/content_browser_client.h:245: // existing renderer in an
existing host.
On 2014/04/24 12:47:21, jochen wrote:
> that's kinda strange. I would have expected that IsSuitableHost is only used
> when we want to launch a new view in the given host (i.e.
> adding_new_render_view_to_existing_process should always be true)?

see the change in content/browser/site_instance_impl.cc

There, it is used with the "false" semantics.

Do you agree? Does this make sense?

[jochen (gone - plz use gerrit), 2014-04-25 08:35:57]

On 2014/04/24 13:03:59, tburkard wrote:
> David, I will address your concerns later.
> 
> Jochen, re: your third party cookie block issue, can you describe the scenario
> more detailed?
> you prerender a page, and then navigate to the prerendered page? Or to a
> different URL with the same 3rd party cookie?
> And then you remove an accepted cookie?

The former.

Basically, I was wondering whether the tab specific content settings (that track
the accepted and blocked cookies per tab) work with your changes.

> 
> Regardless of the scenario, it should work, for the following reasons (but I'd
> be happy to test once you specify how exactly):
> - before a prerender is swapped, will cancel prerender on conflicts (eg
another
> tab/ui sets or clears a cookie)
> - at time of swap, all cookie changes are applied to cookie store and cookie
> store delegate is informed (notice it is NOT informed for cookies set/changed
> during prerender, since the cookie monster for the deltas does not have a
> delegate), just as it would have if the page loaded
> - after the swap, it's just like operating on the existing cookie store, so
> there is no difference any more.
> 
> Based on that, I think the scenario(s) you are describing should work.
> 
> Please clarify.
> 
> Thanks.
> 
>
https://codereview.chromium.org/233353003/diff/220001/content/public/browser/...
> File content/public/browser/content_browser_client.h (right):
> 
>
https://codereview.chromium.org/233353003/diff/220001/content/public/browser/...
> content/public/browser/content_browser_client.h:245: // existing renderer in
an
> existing host.
> On 2014/04/24 12:47:21, jochen wrote:
> > that's kinda strange. I would have expected that IsSuitableHost is only used
> > when we want to launch a new view in the given host (i.e.
> > adding_new_render_view_to_existing_process should always be true)?
> 
> see the change in content/browser/site_instance_impl.cc
> 
> There, it is used with the "false" semantics.


hum, I'm not sure but we'll create a new view there as well, no?

Maybe creis@ should have a look at this.

> 
> Do you agree? Does this make sense?

[tburkard, 2014-04-25 10:41:32]

+ creis. Charlie, can you read the conversation with Jochen here, and review the
change I made to content only, regarding differentiating whether IsSuitableHost 
is called for adding a new render view to an existing render process, vs.
checking whether the current render host is still suitable for the new
navigation.

As described in the most recent response to Jochen, I think these two cases need
to be differentiated now that I changed the policy to never let new render views
join a prerender process (by checking in IsSuitableHost).

Thanks.


Jochen, regarding your tab specific content settings questions: Yes, TSCS will
only be called for all changes to the cookie store once the page gets swapped,
and not before. Furthermore, if any other tab makes changes, resulting in TSCS
calls, which conflict with the prerender changes, the prerender will be
cancelled. Does this answer your question/concerns?

Thanks.

https://codereview.chromium.org/233353003/diff/220001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/233353003/diff/220001/content/public/browser/...
content/public/browser/content_browser_client.h:245: // existing renderer in an
existing host.
On 2014/04/24 13:04:00, tburkard wrote:
> On 2014/04/24 12:47:21, jochen wrote:
> > that's kinda strange. I would have expected that IsSuitableHost is only used
> > when we want to launch a new view in the given host (i.e.
> > adding_new_render_view_to_existing_process should always be true)?
> 
> see the change in content/browser/site_instance_impl.cc
> 
> There, it is used with the "false" semantics.
> 
> Do you agree? Does this make sense?

My understanding is that the code in site_instance_impl.cc which I modified is
called when a navigation in an existing tab happens.
Eg this was a problem in prerendering after the changes that I made, when
IsSuitableHost would return false for a prerender process:
A new render process was created and a new render view, for the prerender. Then,
the navigation for the prerender occurred, and the codepath which I changed to
"false" (as the parameter) would check whether the current renderer is suitable,
which would return false, of course, b/c it was a prerender process. That's the
reason why I had to make this change in content to begin with.

[Charlie Reis, 2014-04-25 23:40:28]

https://codereview.chromium.org/233353003/diff/220001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/233353003/diff/220001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:314: prerender::PrerenderTracker*
prerender_tracker_;
Seems strange to be adding members to this class rather than getting this from
the Profile.  Please have jam@ review this if you decide to keep it this way.

https://codereview.chromium.org/233353003/diff/220001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/233353003/diff/220001/content/public/browser/...
content/public/browser/content_browser_client.h:245: // existing renderer in an
existing host.
On 2014/04/25 10:41:33, tburkard wrote:
> On 2014/04/24 13:04:00, tburkard wrote:
> > On 2014/04/24 12:47:21, jochen wrote:
> > > that's kinda strange. I would have expected that IsSuitableHost is only
used
> > > when we want to launch a new view in the given host (i.e.
> > > adding_new_render_view_to_existing_process should always be true)?
> > 
> > see the change in content/browser/site_instance_impl.cc
> > 
> > There, it is used with the "false" semantics.
> > 
> > Do you agree? Does this make sense?
> 
> My understanding is that the code in site_instance_impl.cc which I modified is
> called when a navigation in an existing tab happens.

This doesn't sound correct.  HasWrongProcessForURL can be called for newly
created tabs as well, as in RFHM::GetSiteInstanceForEntry when it's deciding
whether to use the current (unused) SiteInstance.

> Eg this was a problem in prerendering after the changes that I made, when
> IsSuitableHost would return false for a prerender process:
> A new render process was created and a new render view, for the prerender.
Then,
> the navigation for the prerender occurred, and the codepath which I changed to
> "false" (as the parameter) would check whether the current renderer is
suitable,
> which would return false, of course, b/c it was a prerender process. That's
the
> reason why I had to make this change in content to begin with.

Hmm, I don't think this is a good change to IsSuitableHost.  It's too subtle and
specific to one use case.

It sounds like you're trying to add a different kind of constraint on process
allocation (i.e., how many views it can be used for during a certain stage of
its lifetime), so a different API is probably better for that.  Note that
RenderViews are also not the right unit to be discussing here; we're probably
talking about RenderFrameHosts.

Side note: this would be much easier to review in multiple smaller CLs (e.g.,
one that modifies the prerender process allocation policy).  There should be a
bug number for a change of this size as well.

[tburkard, 2014-04-29 12:01:05]

Thanks, Charlie, just responded to your comments.

It sounds like we are converging here, could you just confirm that my
understanding of your feedback and my proposed change is ok?

jam@, if you could also chime in re: the member for the prerender tracker. We
had discussed this, and, this seemed the easiest solution that would work given
the ordering constraints during init.

Thanks.

https://codereview.chromium.org/233353003/diff/220001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/233353003/diff/220001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:314: prerender::PrerenderTracker*
prerender_tracker_;
On 2014/04/25 23:40:29, Charlie Reis wrote:
> Seems strange to be adding members to this class rather than getting this from
> the Profile.  Please have jam@ review this if you decide to keep it this way.

The reason why we had to do it this way is due to initialization ordering. This
was the easiest way without major rework to achieve this, and jam@ was ok with
this design in my conversation with him.

Does this address your concerns, Charlie?

https://codereview.chromium.org/233353003/diff/220001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/233353003/diff/220001/content/public/browser/...
content/public/browser/content_browser_client.h:245: // existing renderer in an
existing host.
On 2014/04/25 23:40:29, Charlie Reis wrote:
> On 2014/04/25 10:41:33, tburkard wrote:
> > On 2014/04/24 13:04:00, tburkard wrote:
> > > On 2014/04/24 12:47:21, jochen wrote:
> > > > that's kinda strange. I would have expected that IsSuitableHost is only
> used
> > > > when we want to launch a new view in the given host (i.e.
> > > > adding_new_render_view_to_existing_process should always be true)?
> > > 
> > > see the change in content/browser/site_instance_impl.cc
> > > 
> > > There, it is used with the "false" semantics.
> > > 
> > > Do you agree? Does this make sense?
> > 
> > My understanding is that the code in site_instance_impl.cc which I modified
is
> > called when a navigation in an existing tab happens.
> 
> This doesn't sound correct.  HasWrongProcessForURL can be called for newly
> created tabs as well, as in RFHM::GetSiteInstanceForEntry when it's deciding
> whether to use the current (unused) SiteInstance.

Yes. And I think this very case you are describing is what caused prerenders to
not use the newly created RenderProcess when I wanted them to, which is why I
made this change. But both cases are similar, in that they indicate whether a
renderprocess currently assigned should continued to be used, vs. whether we
need to figure out whether for a new RenderView, we can re-use an existing
process. That's why the additional parameter is indicating. Are these semantics
correct?

In some sense, what you are describing is just a special case, where the current
unused SiteInstance can be used. So apart from that detail, is my assessment of
how this is used and the way I am changing it correct (whether or not there's a
more elegant way we can discuss later, but I'm mostly concerned with correctness
for now just so that I fully understand the details of this).
 
> > Eg this was a problem in prerendering after the changes that I made, when
> > IsSuitableHost would return false for a prerender process:
> > A new render process was created and a new render view, for the prerender.
> Then,
> > the navigation for the prerender occurred, and the codepath which I changed
to
> > "false" (as the parameter) would check whether the current renderer is
> suitable,
> > which would return false, of course, b/c it was a prerender process. That's
> the
> > reason why I had to make this change in content to begin with.
> 
> Hmm, I don't think this is a good change to IsSuitableHost.  It's too subtle
and
> specific to one use case.
> 
> It sounds like you're trying to add a different kind of constraint on process
> allocation (i.e., how many views it can be used for during a certain stage of
> its lifetime), so a different API is probably better for that.

I think that's a great suggestion. So to rephrase: leave IsSuitableHost
unchanged, but then have an additional method, such as
MayUseHostForAdditionalRenderView?

The two places where "true" is passed as the new parameter that I added, the new
method would be used performing this additional check, whereas in the other case
where "false" was passed, this additional check would not happen.

Sounds like a pretty simple change and I'd be fine with making it, I just wanted
to check with you that the way I just described it is what you want.

  Note that
> RenderViews are also not the right unit to be discussing here; we're probably
> talking about RenderFrameHosts.

This is only referring to naming of variables/comments, correct? I just used the
terminology to be analogous to what's already there. Eg. see
render_process_host_impl.h:
  // Returns true if |host| is suitable for launching a new view with |site_url|
  // in the given |browser_context|.
  static bool IsSuitableHost(RenderProcessHost* host,
                             BrowserContext* browser_context,
                             const GURL& site_url);

So this seems to be just a comment/variable name issue, but not really a change
in semantics of this method, correct?

Please let me know.

Thanks.

> Side note: this would be much easier to review in multiple smaller CLs (e.g.,
> one that modifies the prerender process allocation policy).  There should be a
> bug number for a change of this size as well.

I may split it into smaller CLs at the time I commit this.

[Charlie Reis, 2014-04-29 22:43:51]

Thanks.  Sounds like there's a path forward.

https://codereview.chromium.org/233353003/diff/220001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/233353003/diff/220001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:314: prerender::PrerenderTracker*
prerender_tracker_;
On 2014/04/29 12:01:06, tburkard wrote:
> On 2014/04/25 23:40:29, Charlie Reis wrote:
> > Seems strange to be adding members to this class rather than getting this
from
> > the Profile.  Please have jam@ review this if you decide to keep it this
way.
> 
> The reason why we had to do it this way is due to initialization ordering.
This
> was the easiest way without major rework to achieve this, and jam@ was ok with
> this design in my conversation with him.
> 
> Does this address your concerns, Charlie?

Yep, thanks.  Might be worth a short comment in the code to explain why it's
there.

https://codereview.chromium.org/233353003/diff/220001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/233353003/diff/220001/content/public/browser/...
content/public/browser/content_browser_client.h:245: // existing renderer in an
existing host.
On 2014/04/29 12:01:06, tburkard wrote:
> On 2014/04/25 23:40:29, Charlie Reis wrote:
> > On 2014/04/25 10:41:33, tburkard wrote:
> > > On 2014/04/24 13:04:00, tburkard wrote:
> > > > On 2014/04/24 12:47:21, jochen wrote:
> > > > > that's kinda strange. I would have expected that IsSuitableHost is
only
> > used
> > > > > when we want to launch a new view in the given host (i.e.
> > > > > adding_new_render_view_to_existing_process should always be true)?
> > > > 
> > > > see the change in content/browser/site_instance_impl.cc
> > > > 
> > > > There, it is used with the "false" semantics.
> > > > 
> > > > Do you agree? Does this make sense?
> > > 
> > > My understanding is that the code in site_instance_impl.cc which I
modified
> is
> > > called when a navigation in an existing tab happens.
> > 
> > This doesn't sound correct.  HasWrongProcessForURL can be called for newly
> > created tabs as well, as in RFHM::GetSiteInstanceForEntry when it's deciding
> > whether to use the current (unused) SiteInstance.
> 
> Yes. And I think this very case you are describing is what caused prerenders
to
> not use the newly created RenderProcess when I wanted them to, which is why I
> made this change. But both cases are similar, in that they indicate whether a
> renderprocess currently assigned should continued to be used, vs. whether we
> need to figure out whether for a new RenderView, we can re-use an existing
> process. That's why the additional parameter is indicating. Are these
semantics
> correct?
> 
> In some sense, what you are describing is just a special case, where the
current
> unused SiteInstance can be used. So apart from that detail, is my assessment
of
> how this is used and the way I am changing it correct (whether or not there's
a
> more elegant way we can discuss later, but I'm mostly concerned with
correctness
> for now just so that I fully understand the details of this).

Yes, I believe so.  (I was responding to your explanation more than the code.  I
don't immediately see a problem with the implementation.)

>  
> > > Eg this was a problem in prerendering after the changes that I made, when
> > > IsSuitableHost would return false for a prerender process:
> > > A new render process was created and a new render view, for the prerender.
> > Then,
> > > the navigation for the prerender occurred, and the codepath which I
changed
> to
> > > "false" (as the parameter) would check whether the current renderer is
> > suitable,
> > > which would return false, of course, b/c it was a prerender process.
That's
> > the
> > > reason why I had to make this change in content to begin with.
> > 
> > Hmm, I don't think this is a good change to IsSuitableHost.  It's too subtle
> and
> > specific to one use case.
> > 
> > It sounds like you're trying to add a different kind of constraint on
process
> > allocation (i.e., how many views it can be used for during a certain stage
of
> > its lifetime), so a different API is probably better for that.
> 
> I think that's a great suggestion. So to rephrase: leave IsSuitableHost
> unchanged, but then have an additional method, such as
> MayUseHostForAdditionalRenderView?

Right.  Perhaps MayReuseHost?

> The two places where "true" is passed as the new parameter that I added, the
new
> method would be used performing this additional check, whereas in the other
case
> where "false" was passed, this additional check would not happen.
> 
> Sounds like a pretty simple change and I'd be fine with making it, I just
wanted
> to check with you that the way I just described it is what you want.

Yes, that sounds like it would work well.

>   Note that
> > RenderViews are also not the right unit to be discussing here; we're
probably
> > talking about RenderFrameHosts.
> 
> This is only referring to naming of variables/comments, correct? I just used
the
> terminology to be analogous to what's already there. Eg. see
> render_process_host_impl.h:
>   // Returns true if |host| is suitable for launching a new view with
|site_url|
>   // in the given |browser_context|.
>   static bool IsSuitableHost(RenderProcessHost* host,
>                              BrowserContext* browser_context,
>                              const GURL& site_url);
> 
> So this seems to be just a comment/variable name issue, but not really a
change
> in semantics of this method, correct?

Correct.  I'm mainly hoping to avoid adding new code that makes the same
assumptions as the code we're trying to change or eliminate.  It's not a huge
problem if we have to revisit it if necessary.

> 
> Please let me know.
> 
> Thanks.
> 
> > Side note: this would be much easier to review in multiple smaller CLs
(e.g.,
> > one that modifies the prerender process allocation policy).  There should be
a
> > bug number for a change of this size as well.
> 
> I may split it into smaller CLs at the time I commit this.

Ah, great.  That would probably help.

[jam, 2014-04-29 22:55:18]

https://codereview.chromium.org/233353003/diff/220001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/233353003/diff/220001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:314: prerender::PrerenderTracker*
prerender_tracker_;
On 2014/04/29 22:43:52, Charlie Reis wrote:
> On 2014/04/29 12:01:06, tburkard wrote:
> > On 2014/04/25 23:40:29, Charlie Reis wrote:
> > > Seems strange to be adding members to this class rather than getting this
> from
> > > the Profile.  Please have jam@ review this if you decide to keep it this
> way.
> > 
> > The reason why we had to do it this way is due to initialization ordering.
> This
> > was the easiest way without major rework to achieve this, and jam@ was ok
with
> > this design in my conversation with him.
> > 
> > Does this address your concerns, Charlie?
> 
> Yep, thanks.  Might be worth a short comment in the code to explain why it's
> there.

+1 to comment, otherwise it seems like the most reasonable way to accomplish
this

[tburkard, 2014-05-06 14:54:44]

All remaining comments should be addressed now, and all tests should pass now.

The only thing left is to add a browser test for this.

While I do this, please take a last pass and let me know if there is anything
else.

Thanks.

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/prerende...
chrome/browser/prerender/prerender_contents.cc:351:
FINAL_STATUS_COOKIE_CONFLICT));
On 2014/04/23 23:58:24, David Benjamin wrote:
> On 2014/04/23 14:52:54, tburkard wrote:
> > On 2014/04/22 21:20:43, David Benjamin wrote:
> > > This is assuming a 1 prerender <-> 1 process already, right? What if you
> made
> > > the callback something like
> > > 
> > >   base::Bind(&PrerenderContents::Destroy, AsWeakPtr(),
> > > FINAL_STATUS_COOKIE_CONFLICT)
> > > 
> > > and make PrerenderContents weakptr-able. Might be simpler than having to
go
> > back
> > > and iterate over processes and such.
> > 
> > I'd prefer to not make this weakptr-able. It's a pretty small loop iterating
> > over the prerender contents and destroying it, if that's ok?
> > 
> > Thanks.
> 
> I think I'd prefer the weakptr version, just because it seems simpler, but
it's
> not a strong preference so *shrug*. I'm not sure we can keep prerender
> multi-process-unaware forever (What happens if a prerender has a cross-origin
> iframe post site-isolation? Do we find some way to defer it or allow it?), and
> the other version at least keeps everything after the callback agnostic to
this,
> so we'd just have to attach the cookie store to all the relevant processes.

Done.

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_cookie_store.cc (right):

https://codereview.chromium.org/233353003/diff/180001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.cc:215: BrowserThread::PostTask(
On 2014/04/23 23:58:24, David Benjamin wrote:
> On 2014/04/23 14:52:54, tburkard wrote:
> > On 2014/04/22 21:20:43, David Benjamin wrote:
> > > There's a race here. If a conflict happens right as a swap is happening,
the
> > UI
> > > thread signal can happen after the swap occurs. (Canonical swap state
lives
> on
> > > the UI thread.)
> > > 
> > > I suspect it doesn't matter since you probably also get races if two
> requests
> > > happen at the same time and both change cookies. But I wanted to point it
> out.
> > 
> > Exactly. It's the same situation as if two tabs try to perform conflicting
> > operations on the cookie store at about the same time.
> 
> Cool. Maybe put a comment to that effect here just so the reason the race is
> okay is documented.

Done.

https://codereview.chromium.org/233353003/diff/220001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/233353003/diff/220001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:314: prerender::PrerenderTracker*
prerender_tracker_;
On 2014/04/29 22:55:19, jam wrote:
> On 2014/04/29 22:43:52, Charlie Reis wrote:
> > On 2014/04/29 12:01:06, tburkard wrote:
> > > On 2014/04/25 23:40:29, Charlie Reis wrote:
> > > > Seems strange to be adding members to this class rather than getting
this
> > from
> > > > the Profile.  Please have jam@ review this if you decide to keep it this
> > way.
> > > 
> > > The reason why we had to do it this way is due to initialization ordering.
> > This
> > > was the easiest way without major rework to achieve this, and jam@ was ok
> with
> > > this design in my conversation with him.
> > > 
> > > Does this address your concerns, Charlie?
> > 
> > Yep, thanks.  Might be worth a short comment in the code to explain why it's
> > there.
> 
> +1 to comment, otherwise it seems like the most reasonable way to accomplish
> this

Done.

https://codereview.chromium.org/233353003/diff/220001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:314: prerender::PrerenderTracker*
prerender_tracker_;
On 2014/04/29 22:43:52, Charlie Reis wrote:
> On 2014/04/29 12:01:06, tburkard wrote:
> > On 2014/04/25 23:40:29, Charlie Reis wrote:
> > > Seems strange to be adding members to this class rather than getting this
> from
> > > the Profile.  Please have jam@ review this if you decide to keep it this
> way.
> > 
> > The reason why we had to do it this way is due to initialization ordering.
> This
> > was the easiest way without major rework to achieve this, and jam@ was ok
with
> > this design in my conversation with him.
> > 
> > Does this address your concerns, Charlie?
> 
> Yep, thanks.  Might be worth a short comment in the code to explain why it's
> there.

Done.

https://codereview.chromium.org/233353003/diff/220001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/233353003/diff/220001/content/public/browser/...
content/public/browser/content_browser_client.h:245: // existing renderer in an
existing host.
On 2014/04/29 22:43:52, Charlie Reis wrote:
> On 2014/04/29 12:01:06, tburkard wrote:
> > On 2014/04/25 23:40:29, Charlie Reis wrote:
> > > On 2014/04/25 10:41:33, tburkard wrote:
> > > > On 2014/04/24 13:04:00, tburkard wrote:
> > > > > On 2014/04/24 12:47:21, jochen wrote:
> > > > > > that's kinda strange. I would have expected that IsSuitableHost is
> only
> > > used
> > > > > > when we want to launch a new view in the given host (i.e.
> > > > > > adding_new_render_view_to_existing_process should always be true)?
> > > > > 
> > > > > see the change in content/browser/site_instance_impl.cc
> > > > > 
> > > > > There, it is used with the "false" semantics.
> > > > > 
> > > > > Do you agree? Does this make sense?
> > > > 
> > > > My understanding is that the code in site_instance_impl.cc which I
> modified
> > is
> > > > called when a navigation in an existing tab happens.
> > > 
> > > This doesn't sound correct.  HasWrongProcessForURL can be called for newly
> > > created tabs as well, as in RFHM::GetSiteInstanceForEntry when it's
deciding
> > > whether to use the current (unused) SiteInstance.
> > 
> > Yes. And I think this very case you are describing is what caused prerenders
> to
> > not use the newly created RenderProcess when I wanted them to, which is why
I
> > made this change. But both cases are similar, in that they indicate whether
a
> > renderprocess currently assigned should continued to be used, vs. whether we
> > need to figure out whether for a new RenderView, we can re-use an existing
> > process. That's why the additional parameter is indicating. Are these
> semantics
> > correct?
> > 
> > In some sense, what you are describing is just a special case, where the
> current
> > unused SiteInstance can be used. So apart from that detail, is my assessment
> of
> > how this is used and the way I am changing it correct (whether or not
there's
> a
> > more elegant way we can discuss later, but I'm mostly concerned with
> correctness
> > for now just so that I fully understand the details of this).
> 
> Yes, I believe so.  (I was responding to your explanation more than the code. 
I
> don't immediately see a problem with the implementation.)
> 
> >  
> > > > Eg this was a problem in prerendering after the changes that I made,
when
> > > > IsSuitableHost would return false for a prerender process:
> > > > A new render process was created and a new render view, for the
prerender.
> > > Then,
> > > > the navigation for the prerender occurred, and the codepath which I
> changed
> > to
> > > > "false" (as the parameter) would check whether the current renderer is
> > > suitable,
> > > > which would return false, of course, b/c it was a prerender process.
> That's
> > > the
> > > > reason why I had to make this change in content to begin with.
> > > 
> > > Hmm, I don't think this is a good change to IsSuitableHost.  It's too
subtle
> > and
> > > specific to one use case.
> > > 
> > > It sounds like you're trying to add a different kind of constraint on
> process
> > > allocation (i.e., how many views it can be used for during a certain stage
> of
> > > its lifetime), so a different API is probably better for that.
> > 
> > I think that's a great suggestion. So to rephrase: leave IsSuitableHost
> > unchanged, but then have an additional method, such as
> > MayUseHostForAdditionalRenderView?
> 
> Right.  Perhaps MayReuseHost?
> 
> > The two places where "true" is passed as the new parameter that I added, the
> new
> > method would be used performing this additional check, whereas in the other
> case
> > where "false" was passed, this additional check would not happen.
> > 
> > Sounds like a pretty simple change and I'd be fine with making it, I just
> wanted
> > to check with you that the way I just described it is what you want.
> 
> Yes, that sounds like it would work well.
> 
> >   Note that
> > > RenderViews are also not the right unit to be discussing here; we're
> probably
> > > talking about RenderFrameHosts.
> > 
> > This is only referring to naming of variables/comments, correct? I just used
> the
> > terminology to be analogous to what's already there. Eg. see
> > render_process_host_impl.h:
> >   // Returns true if |host| is suitable for launching a new view with
> |site_url|
> >   // in the given |browser_context|.
> >   static bool IsSuitableHost(RenderProcessHost* host,
> >                              BrowserContext* browser_context,
> >                              const GURL& site_url);
> > 
> > So this seems to be just a comment/variable name issue, but not really a
> change
> > in semantics of this method, correct?
> 
> Correct.  I'm mainly hoping to avoid adding new code that makes the same
> assumptions as the code we're trying to change or eliminate.  It's not a huge
> problem if we have to revisit it if necessary.
> 
> > 
> > Please let me know.
> > 
> > Thanks.
> > 
> > > Side note: this would be much easier to review in multiple smaller CLs
> (e.g.,
> > > one that modifies the prerender process allocation policy).  There should
be
> a
> > > bug number for a change of this size as well.
> > 
> > I may split it into smaller CLs at the time I commit this.
> 
> Ah, great.  That would probably help.

Done.

[tburkard, 2014-05-07 04:12:55]

I'd like to still get this into M36 (ie commit it before Friday). I will
finish the tests today. It'd be great if you could take another pass in
time and issue the lgtm before then, or send any remaining concerns far
enough in advance so that I can address them on wednesday or thursday.

Thanks.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jochen (gone - plz use gerrit), 2014-05-07 06:24:01]

On 2014/05/07 04:12:55, tburkard wrote:
> I'd like to still get this into M36 (ie commit it before Friday). I will

the deadline for new features was last friday...

> finish the tests today. It'd be great if you could take another pass in
> time and issue the lgtm before then, or send any remaining concerns far
> enough in advance so that I can address them on wednesday or thursday.
> 
> Thanks.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[tburkard, 2014-05-07 08:35:49]

On Wed, May 7, 2014 at 8:24 AM, <jochen@chromium.org> wrote:

> On 2014/05/07 04:12:55, tburkard wrote:
>
>> I'd like to still get this into M36 (ie commit it before Friday). I will
>>
>
> the deadline for new features was last friday...


Probably a good idea to rather let this bake in canary/dev for a bit rather
than rushing it out.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tburkard, 2014-05-07 14:48:57]

Added browser tests for the new functionality, and fixed a bug in the change
yesterday.

Everyone, please review this change and let me know if you have any other
concerns, or if this looks good to you.

Thanks.

[Charlie Reis, 2014-05-07 17:21:31]

Main remaining concerns: it looks like you're using prerender_tracker_ on both
the UI thread and the IO thread (unsafe), and this needs a bug number.

A few nits apart from that.

https://codereview.chromium.org/233353003/diff/220001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/233353003/diff/220001/content/public/browser/...
content/public/browser/content_browser_client.h:245: // existing renderer in an
existing host.
On 2014/05/06 14:54:46, tburkard wrote:
> > > > Side note: this would be much easier to review in multiple smaller CLs
> > (e.g.,
> > > > one that modifies the prerender process allocation policy).  There
should
> be
> > a
> > > > bug number for a change of this size as well.
> > > 
> > > I may split it into smaller CLs at the time I commit this.
> > 
> > Ah, great.  That would probably help.
> 
> Done.

This part is not done.  Please add a bug number to the CL, especially if you're
trying to get it into a particular release.

It's also still one big CL.  Waiting to break it up until after it's approved
means the reviewers don't benefit from having smaller chunks to review.  Perhaps
it's too late for that now, but please keep it in mind in the future.

https://codereview.chromium.org/233353003/diff/270001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/233353003/diff/270001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1218:
prerender_tracker_->IsProcessPrerendering(process_host)) {
Can we add a DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)) in this
function?

https://codereview.chromium.org/233353003/diff/270001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/233353003/diff/270001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:315: // (initialize on the UI
thread, use on the IO thread).
What makes this safe to use on a different thread than it's initialized? 
(Otherwise it could be partially initialized when we try to use it.)  

Can you clarify that in the comment?

https://codereview.chromium.org/233353003/diff/270001/chrome/browser/prerende...
File chrome/browser/prerender/external_prerender_handler_android.cc (right):

https://codereview.chromium.org/233353003/diff/270001/chrome/browser/prerende...
chrome/browser/prerender/external_prerender_handler_android.cc:97:
prerender::PrerenderManagerFactory::GetForProfile(profile);
nit: Looks like wrong indent.

https://codereview.chromium.org/233353003/diff/270001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.h (right):

https://codereview.chromium.org/233353003/diff/270001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.h:370: void
set_on_cookie_store_loaded_cb(base::Closure cb) {
I think we typically include "_for_testing" in the name to make it easier to
spot places that try to call such functions in normal code.

https://codereview.chromium.org/233353003/diff/270001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/233353003/diff/270001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1683:
ContentBrowserClient* client = GetContentClient()->browser();
This change doesn't look necessary.

https://codereview.chromium.org/233353003/diff/270001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1764: if
(GetContentClient()->browser()->MayReuseHost(iter.GetCurrentValue()) &&
Wait, this is on the UI thread, not the IO thread.  Is the comment in
ChromeContentBrowserClient incorrect?

https://codereview.chromium.org/233353003/diff/270001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1823:
(!GetContentClient()->browser()->MayReuseHost(host)) ||
This is also on the UI thread.

https://codereview.chromium.org/233353003/diff/270001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1824:
(!IsSuitableHost(host, browser_context, url)))) {
Seems like there's some unnecessary parentheses here, making it harder to read.

https://codereview.chromium.org/233353003/diff/270001/content/public/browser/...
File content/public/browser/content_browser_client.cc (right):

https://codereview.chromium.org/233353003/diff/270001/content/public/browser/...
content/public/browser/content_browser_client.cc:72: const GURL& site_url) {
This change looks unnecessary.

https://codereview.chromium.org/233353003/diff/270001/net/cookies/canonical_c...
File net/cookies/canonical_cookie.cc (right):

https://codereview.chromium.org/233353003/diff/270001/net/cookies/canonical_c...
net/cookies/canonical_cookie.cc:397: CanonicalCookie*
CanonicalCookie::Duplicate() {
This looks like it will quickly get stale if more members are added to
CanonicalCookie.  Maybe there should be a comment near the members in the .h
file saying to update this method?

[tburkard, 2014-05-07 18:34:26]

Addressed all your concerns, Charlie.

Added a bug number, and clarified that my use of prerender_tracker_ on both
threads is safe due to when init happens and when the IO thread is created.

Please let me know if you have any more concerns.

Thanks.

https://codereview.chromium.org/233353003/diff/220001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/233353003/diff/220001/content/public/browser/...
content/public/browser/content_browser_client.h:245: // existing renderer in an
existing host.
On 2014/05/07 17:21:32, Charlie Reis wrote:
> On 2014/05/06 14:54:46, tburkard wrote:
> > > > > Side note: this would be much easier to review in multiple smaller CLs
> > > (e.g.,
> > > > > one that modifies the prerender process allocation policy).  There
> should
> > be
> > > a
> > > > > bug number for a change of this size as well.
> > > > 
> > > > I may split it into smaller CLs at the time I commit this.
> > > 
> > > Ah, great.  That would probably help.
> > 
> > Done.
> 
> This part is not done.  Please add a bug number to the CL, especially if
you're
> trying to get it into a particular release.
> 
> It's also still one big CL.  Waiting to break it up until after it's approved
> means the reviewers don't benefit from having smaller chunks to review. 
Perhaps
> it's too late for that now, but please keep it in mind in the future.

Added a bug number.

As for breaking up, won't do that this change, but keep it in mind for the
future.

Thanks.

https://codereview.chromium.org/233353003/diff/270001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/233353003/diff/270001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1218:
prerender_tracker_->IsProcessPrerendering(process_host)) {
On 2014/05/07 17:21:32, Charlie Reis wrote:
> Can we add a DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)) in this
> function?

Done. It is actually on UI, not on IO.

https://codereview.chromium.org/233353003/diff/270001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/233353003/diff/270001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:315: // (initialize on the UI
thread, use on the IO thread).
On 2014/05/07 17:21:32, Charlie Reis wrote:
> What makes this safe to use on a different thread than it's initialized? 
> (Otherwise it could be partially initialized when we try to use it.)  
> 
> Can you clarify that in the comment?

Corrected & clarified the comment.

https://codereview.chromium.org/233353003/diff/270001/chrome/browser/prerende...
File chrome/browser/prerender/external_prerender_handler_android.cc (right):

https://codereview.chromium.org/233353003/diff/270001/chrome/browser/prerende...
chrome/browser/prerender/external_prerender_handler_android.cc:97:
prerender::PrerenderManagerFactory::GetForProfile(profile);
On 2014/05/07 17:21:32, Charlie Reis wrote:
> nit: Looks like wrong indent.

Done.

https://codereview.chromium.org/233353003/diff/270001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.h (right):

https://codereview.chromium.org/233353003/diff/270001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.h:370: void
set_on_cookie_store_loaded_cb(base::Closure cb) {
On 2014/05/07 17:21:32, Charlie Reis wrote:
> I think we typically include "_for_testing" in the name to make it easier to
> spot places that try to call such functions in normal code.

Done.

https://codereview.chromium.org/233353003/diff/270001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/233353003/diff/270001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1683:
ContentBrowserClient* client = GetContentClient()->browser();
On 2014/05/07 17:21:32, Charlie Reis wrote:
> This change doesn't look necessary.

Done.

https://codereview.chromium.org/233353003/diff/270001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1764: if
(GetContentClient()->browser()->MayReuseHost(iter.GetCurrentValue()) &&
On 2014/05/07 17:21:32, Charlie Reis wrote:
> Wait, this is on the UI thread, not the IO thread.  Is the comment in
> ChromeContentBrowserClient incorrect?

resolved per prior comments.

https://codereview.chromium.org/233353003/diff/270001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1823:
(!GetContentClient()->browser()->MayReuseHost(host)) ||
On 2014/05/07 17:21:32, Charlie Reis wrote:
> This is also on the UI thread.

Resolved per prior comments.

https://codereview.chromium.org/233353003/diff/270001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1824:
(!IsSuitableHost(host, browser_context, url)))) {
On 2014/05/07 17:21:32, Charlie Reis wrote:
> Seems like there's some unnecessary parentheses here, making it harder to
read.

Done.

https://codereview.chromium.org/233353003/diff/270001/content/public/browser/...
File content/public/browser/content_browser_client.cc (right):

https://codereview.chromium.org/233353003/diff/270001/content/public/browser/...
content/public/browser/content_browser_client.cc:72: const GURL& site_url) {
On 2014/05/07 17:21:32, Charlie Reis wrote:
> This change looks unnecessary.

Done.

https://codereview.chromium.org/233353003/diff/270001/net/cookies/canonical_c...
File net/cookies/canonical_cookie.cc (right):

https://codereview.chromium.org/233353003/diff/270001/net/cookies/canonical_c...
net/cookies/canonical_cookie.cc:397: CanonicalCookie*
CanonicalCookie::Duplicate() {
On 2014/05/07 17:21:32, Charlie Reis wrote:
> This looks like it will quickly get stale if more members are added to
> CanonicalCookie.  Maybe there should be a comment near the members in the .h
> file saying to update this method?

Done.

[tburkard, 2014-05-07 18:58:00]

On Wed, May 7, 2014 at 7:21 PM, <creis@chromium.org> wrote:

> Main remaining concerns: it looks like you're using prerender_tracker_ on
> both
> the UI thread and the IO thread (unsafe), and this needs a bug number.
>

Actually, I take back what I just said: It does seem like the IO thread is
started before the init completes.

And I guess there could (either now or at some point in the future) be
early network requests affecting cookies.

Therefore, it's probably safer to add a lock, since I can't see any other
way to initialize this variable earlier.

Charlie/John: If you have any other suggestions on how to do this without a
lock, please let me know. Or, if you are ok with the lock, please let me
know, too.

Adding a lock should not be bad in terms of contention, because the
operations on the UI thread would be very rare, whereas the ones on the IO
thread might be more frequent (eg if bulk cookie operations are performed).

Thoughts?

Thanks.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tburkard, 2014-05-07 19:03:09]

Thinking about this some more: So the member is initialized to NULL, and
that happens before the IO thread starts.

Then, the IO thread starts.

Next, on the UI thread, we assign the correct pointer to the member which
was formerly NULL.

Isn't assigning the pointer an atomic operation? Ie on the IO thread, you'd
always see a NULL or the new pointer.

In that case, no lock would be necessary?

Thoughts?

Thanks.

On Wed, May 7, 2014 at 8:57 PM, Timo Burkard <tburkard@chromium.org> wrote:

>
> On Wed, May 7, 2014 at 7:21 PM, <creis@chromium.org> wrote:
>
>> Main remaining concerns: it looks like you're using prerender_tracker_ on
>> both
>> the UI thread and the IO thread (unsafe), and this needs a bug number.
>>
>
> Actually, I take back what I just said: It does seem like the IO thread is
> started before the init completes.
>
> And I guess there could (either now or at some point in the future) be
> early network requests affecting cookies.
>
> Therefore, it's probably safer to add a lock, since I can't see any other
> way to initialize this variable earlier.
>
> Charlie/John: If you have any other suggestions on how to do this without
> a lock, please let me know. Or, if you are ok with the lock, please let me
> know, too.
>
> Adding a lock should not be bad in terms of contention, because the
> operations on the UI thread would be very rare, whereas the ones on the IO
> thread might be more frequent (eg if bulk cookie operations are performed).
>
> Thoughts?
>
> Thanks.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jam, 2014-05-07 19:45:07]

lgtm

[Charlie Reis, 2014-05-07 19:46:40]

This sounds very subtle and potentially fragile to me.  Looking closer at
PrerenderTracker, it appears that it has two disjoint roles:
prerender_process_hosts_ is accessed and modified by methods on the UI thread,
and the rest is for the IO thread.  Would it be possible to separate those into
separate objects to make it clearer which roles they're responsible for?

On 2014/05/07 19:03:09, tburkard wrote:
> Thinking about this some more: So the member is initialized to NULL, and
> that happens before the IO thread starts.
> 
> Then, the IO thread starts.
> 
> Next, on the UI thread, we assign the correct pointer to the member which
> was formerly NULL.
> 
> Isn't assigning the pointer an atomic operation? Ie on the IO thread, you'd
> always see a NULL or the new pointer.
> 
> In that case, no lock would be necessary?
> 
> Thoughts?
> 
> Thanks.
> 
> On Wed, May 7, 2014 at 8:57 PM, Timo Burkard <mailto:tburkard@chromium.org>
wrote:
> 
> >
> > On Wed, May 7, 2014 at 7:21 PM, <mailto:creis@chromium.org> wrote:
> >
> >> Main remaining concerns: it looks like you're using prerender_tracker_ on
> >> both
> >> the UI thread and the IO thread (unsafe), and this needs a bug number.
> >>
> >
> > Actually, I take back what I just said: It does seem like the IO thread is
> > started before the init completes.
> >
> > And I guess there could (either now or at some point in the future) be
> > early network requests affecting cookies.
> >
> > Therefore, it's probably safer to add a lock, since I can't see any other
> > way to initialize this variable earlier.
> >
> > Charlie/John: If you have any other suggestions on how to do this without
> > a lock, please let me know. Or, if you are ok with the lock, please let me
> > know, too.
> >
> > Adding a lock should not be bad in terms of contention, because the
> > operations on the UI thread would be very rare, whereas the ones on the IO
> > thread might be more frequent (eg if bulk cookie operations are performed).
> >
> > Thoughts?
> >
> > Thanks.
> >
>

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:314: // the profile due to
initialization ordering, as well as due to threading
nit: end with period.

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:315: // It is initialize on the
UI thread as early as possible once the
nit: initialized

(Also sounds like our understanding of this is still changing.)

[davidben, 2014-05-07 19:56:59]

Still need to finish looking over the new version, but I wanted to publish the
threading/initialization comment.

https://codereview.chromium.org/233353003/diff/270001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/233353003/diff/270001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2219: return
g_browser_process->ResourceDispatcherHostCreated();
I think we're fine with this threading behavior, though it's a little screwy.
This hook is called in RDH's ctor. The RDH is created on the UI thread, calls
this hook, and then posts to inform the IO thread.

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo...


If there were a race here, we'd have quite a lot of other problems.
BrowserProcessImpl::ResourceDispatcherHostCreated initializes other things for
the IO thread:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro...

The RDH's delegate gets set in there. That delegate also gets passed the
PrerenderTracker which does the same thing as here. (Initialize on UI, use on
IO.)

(Oh, for more fun, BrowserProcessImpl::prerender_tracker() isn't actually
thread-safe despite looking like an accessor.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro...
)

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/extensio...
File chrome/browser/extensions/activity_log/activity_log_browsertest.cc (right):

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/extensio...
chrome/browser/extensions/activity_log/activity_log_browsertest.cc:110:
base::MessageLoop::current()->Run();
Nit: I think base::RunLoop is the thing to be using these days?

  base::RunLoop loop;
  [...]->set_[...](loop.QuitClosure());
  loop.Run();

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/net_internals/net_internals_ui_browsertest.cc
(right):

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/ui/webui...
chrome/browser/ui/webui/net_internals/net_internals_ui_browsertest.cc:406:
base::MessageLoop::current()->Run();
Nit: Ditto to other comment; this can be a base::RunLoop.

[tburkard, 2014-05-07 20:02:28]

On Wed, May 7, 2014 at 9:46 PM, <creis@chromium.org> wrote:

> This sounds very subtle and potentially fragile to me.  Looking closer at
> PrerenderTracker, it appears that it has two disjoint roles:
> prerender_process_hosts_ is accessed and modified by methods on the UI
> thread,
> and the rest is for the IO thread.  Would it be possible to separate those
> into
> separate objects to make it clearer which roles they're responsible for?


Historically, the prerender tracker  has been used for operations done on
the UI thread and the IO thread.

In this case, the things done on the IO thread and on the UI thread are
closely related, which is why I think having it in one class makes sense.

Given John's and David's feedback, I think just keeping it the way it is
should be fine, adjusting the comments to clarify it.

Would you be ok with that?

Thanks.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Charlie Reis, 2014-05-07 20:30:35]

On 2014/05/07 20:02:28, tburkard wrote:
> On Wed, May 7, 2014 at 9:46 PM, <mailto:creis@chromium.org> wrote:
> 
> > This sounds very subtle and potentially fragile to me.  Looking closer at
> > PrerenderTracker, it appears that it has two disjoint roles:
> > prerender_process_hosts_ is accessed and modified by methods on the UI
> > thread,
> > and the rest is for the IO thread.  Would it be possible to separate those
> > into
> > separate objects to make it clearer which roles they're responsible for?
> 
> 
> Historically, the prerender tracker  has been used for operations done on
> the UI thread and the IO thread.

I suppose its UI tasks have since been removed?  At the moment it seems to be
about things on the IO thread, apart from construction.

> In this case, the things done on the IO thread and on the UI thread are
> closely related, which is why I think having it in one class makes sense.
> 
> Given John's and David's feedback, I think just keeping it the way it is
> should be fine, adjusting the comments to clarify it.
> 
> Would you be ok with that?
> 
> Thanks.

I'm not thrilled with this aspect of the design, but I'll defer to David as the
prerender reviewer here.

content/ LGTM.

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_tracker.cc (right):

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/prerende...
chrome/browser/prerender/prerender_tracker.cc:23:
PrerenderTracker::~PrerenderTracker() {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_tracker.h (right):

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/prerende...
chrome/browser/prerender/prerender_tracker.h:28: // Global object for
maintaining prerender state on the IO thread.
Let's update this comment if it's no longer true.

[davidben, 2014-05-07 23:17:42]

https://codereview.chromium.org/233353003/diff/290001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java
(right):

https://codereview.chromium.org/233353003/diff/290001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java:41:
launchChromeShellWithUrl(HOMEPAGE_URL);
This seems a little bizarre. Does this mean that we can't start prerenders until
we've attempted to load some non-blank page?

https://codereview.chromium.org/233353003/diff/290001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java:52:
Thread.sleep(CHECK_COOKIE_STORE_FREQUENCY_MS);
Eww. I suppose the rest of this test is also full of sleeps, and plumbing some
signal out is going to be messy, so I'm not asking for a change here, but eww.
:-)

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:315: // It is initialize on the
UI thread as early as possible once the
On 2014/05/07 19:46:41, Charlie Reis wrote:
> nit: initialized
> 
> (Also sounds like our understanding of this is still changing.)

I would maybe replace "as early as possible once the prerender tracker has been
created" to "when the ResourceDispatcherHost is created". The PrerenderTracker
gets created lazily in BrowserProcessImpl::prerender_tracker() anyway, so "once
the prerender tracker has been created" doesn't really mean much.

Actually, I'm slightly puzzled why you need the interesting initialization
anyway. Is g_browser_process initialized after ChromeContentBrowserClient (I
haven't checked)? If not, I'd think you could just set it in this guy's
constructor.

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/net/cook...
File chrome/browser/net/cookie_store_util.cc (right):

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/net/cook...
chrome/browser/net/cookie_store_util.cc:51: this, cookie, removed, cause));
Would it work to use this hook to call
PrerenderCookieStore::OnCookieChangedForURL? (Appropriately tweaked to check
CanonicalCookie instead of the URL.) That seems like it might be easier than
having to hook into two places for the different ways you can set a cookie.

It also means that the following would work. (Or does it already?)

1. Prerender a.com/1. It sets a cookie.
2. Prerender a.com/2. It sets a cookie.
3. Swap a.com/1 in. Changes are replayed.
4. a.com/2's prerender should abort.

Mind adding a test for that too? :-)

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:1115:
ui_test_utils::NavigateToURLWithDisposition(
Nit: This can probably just be

  ui_test_utils::NavigateToURL(current_browser(),
test_server()->GetURL(dest_html_file));

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:4212:
prerender->WaitForLoads(1);
Could you also make this test assert that the prerender sees the new cookie
value prior to swap?

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:4232: 
Could you add variants of these tests for cookies set by HTTP headers? You can
probably do it with some of these test server handlers.

https://code.google.com/p/chromium/codesearch#chromium/src/net/tools/testserv...

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_tracker.cc (right):

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/prerende...
chrome/browser/prerender/prerender_tracker.cc:146:
it->second->OnCookieChangedForURL(url);
This is actually a somewhat more aggressive check that we actually need; if a
cookie is changed in one profile, we don't need to kill prerenders in other
profiles.

This can be a follow-up since it involves more refactoring, but we could maybe
turn CookieMonster::Delegate into CookieMonster::Observer (all the methods are
observer methods anyway). And then PrerenderCookieStore could register itself as
an observer of the CookieMonster it wraps. (This is assuming my other comment of
moving the OnCookieChangedForURL call to Chrome's CookieMonster::Delegate
implementation works.)

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_tracker.h (right):

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/prerende...
chrome/browser/prerender/prerender_tracker.h:122: PrerenderProcessSet
prerender_process_hosts_;
I think I agree with creis that this should perhaps be better on the
PrerenderManager. PrerenderManager is getting monstrously large, but I'm not
sure putting new things in PrerenderTracker is the best way to fix that. (I'm
thinking we can maybe pull PendingSwap out into a separate file.) After the
various refactors from RenderView -> RenderFrame, PrerenderTracker has become
some auxiliary object for tracking IO-thread-level state. Seems worth keeping it
that way so it's not unclear which component should do what.

(In ChromeContentBrowserClient, you can go from RPH to PrerenderManager via
content::BrowserContext which you can exchange for a Profile with
Profile::FromBrowserContext.)

[jochen (gone - plz use gerrit), 2014-05-08 07:07:58]

I defer to Erik and Charlie for my parts, moving myself to cc

[tburkard, 2014-05-08 10:59:53]

This should address all outstanding comments, except for David's browser_test
related comments. I will address these later today in a separate upload.

Please let me know if things otherwise look good now.

Thanks.

https://codereview.chromium.org/233353003/diff/270001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/233353003/diff/270001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2219: return
g_browser_process->ResourceDispatcherHostCreated();
On 2014/05/07 19:57:00, David Benjamin wrote:
> I think we're fine with this threading behavior, though it's a little screwy.
> This hook is called in RDH's ctor. The RDH is created on the UI thread, calls
> this hook, and then posts to inform the IO thread.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo...
> 
> 
> If there were a race here, we'd have quite a lot of other problems.
> BrowserProcessImpl::ResourceDispatcherHostCreated initializes other things for
> the IO thread:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro...
> 
> The RDH's delegate gets set in there. That delegate also gets passed the
> PrerenderTracker which does the same thing as here. (Initialize on UI, use on
> IO.)
> 
> (Oh, for more fun, BrowserProcessImpl::prerender_tracker() isn't actually
> thread-safe despite looking like an accessor.
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro...
> )

Done.

https://codereview.chromium.org/233353003/diff/290001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java
(right):

https://codereview.chromium.org/233353003/diff/290001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java:41:
launchChromeShellWithUrl(HOMEPAGE_URL);
On 2014/05/07 23:17:42, David Benjamin wrote:
> This seems a little bizarre. Does this mean that we can't start prerenders
until
> we've attempted to load some non-blank page?

In the actual browser, there is a lot of network activity that happens so that
this is def. not the case. In this test cases here however, it happens to be the
case.

We will observe how often prerender is not taking place due to cookies not being
loaded, and if it's too high, we can address it then. But I suspect it will be
extremely low.

https://codereview.chromium.org/233353003/diff/290001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/prerender/ExternalPrerenderRequestTest.java:52:
Thread.sleep(CHECK_COOKIE_STORE_FREQUENCY_MS);
On 2014/05/07 23:17:42, David Benjamin wrote:
> Eww. I suppose the rest of this test is also full of sleeps, and plumbing some
> signal out is going to be messy, so I'm not asking for a change here, but eww.
> :-)

Done.

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:314: // the profile due to
initialization ordering, as well as due to threading
On 2014/05/07 19:46:41, Charlie Reis wrote:
> nit: end with period.

Done.

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:315: // It is initialize on the
UI thread as early as possible once the
On 2014/05/07 19:46:41, Charlie Reis wrote:
> nit: initialized
> 
> (Also sounds like our understanding of this is still changing.)

Done.

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:315: // It is initialize on the
UI thread as early as possible once the
On 2014/05/07 23:17:42, David Benjamin wrote:
> On 2014/05/07 19:46:41, Charlie Reis wrote:
> > nit: initialized
> > 
> > (Also sounds like our understanding of this is still changing.)
> 
> I would maybe replace "as early as possible once the prerender tracker has
been
> created" to "when the ResourceDispatcherHost is created". The PrerenderTracker
> gets created lazily in BrowserProcessImpl::prerender_tracker() anyway, so
"once
> the prerender tracker has been created" doesn't really mean much.
Done.

> Actually, I'm slightly puzzled why you need the interesting initialization
> anyway. Is g_browser_process initialized after ChromeContentBrowserClient (I
> haven't checked)? If not, I'd think you could just set it in this guy's
> constructor.

No, I tried that, and it didn't work. If you initialize PrerenderTracker too
early, it will fail, because something else that needs to have finished
initializing hasn't completed yet. Dont remember the exact details.

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/extensio...
File chrome/browser/extensions/activity_log/activity_log_browsertest.cc (right):

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/extensio...
chrome/browser/extensions/activity_log/activity_log_browsertest.cc:110:
base::MessageLoop::current()->Run();
On 2014/05/07 19:57:00, David Benjamin wrote:
> Nit: I think base::RunLoop is the thing to be using these days?
> 
>   base::RunLoop loop;
>   [...]->set_[...](loop.QuitClosure());
>   loop.Run();

Done.

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/net/cook...
File chrome/browser/net/cookie_store_util.cc (right):

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/net/cook...
chrome/browser/net/cookie_store_util.cc:51: this, cookie, removed, cause));
On 2014/05/07 23:17:42, David Benjamin wrote:
> Would it work to use this hook to call
> PrerenderCookieStore::OnCookieChangedForURL? (Appropriately tweaked to check
> CanonicalCookie instead of the URL.) That seems like it might be easier than
> having to hook into two places for the different ways you can set a cookie.
> 
> It also means that the following would work. (Or does it already?)
> 
> 1. Prerender a.com/1. It sets a cookie.
> 2. Prerender a.com/2. It sets a cookie.
> 3. Swap a.com/1 in. Changes are replayed.
> 4. a.com/2's prerender should abort.
> 
> Mind adding a test for that too? :-)

(1) your concern is valid, it is not working right now.
(2) what you are suggesting is complicated, because at that point we no longer
know what process originated the change. Also, Erik was somewhat opposed to
changing this unless really necessary.
(3) the way we can easily fix it is by calling OnCookieChangedForURL after
ApplyChanges for all the cookies modified. I just added this fix.
(4) I will add the test in a later iteration of this changelist (not uploaded in
this pass), along with the other test fixes.

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_tracker.cc (right):

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/prerende...
chrome/browser/prerender/prerender_tracker.cc:23:
PrerenderTracker::~PrerenderTracker() {
On 2014/05/07 20:30:36, Charlie Reis wrote:
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

Done.

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/prerende...
chrome/browser/prerender/prerender_tracker.cc:146:
it->second->OnCookieChangedForURL(url);
On 2014/05/07 23:17:42, David Benjamin wrote:
> This is actually a somewhat more aggressive check that we actually need; if a
> cookie is changed in one profile, we don't need to kill prerenders in other
> profiles.
> 
> This can be a follow-up since it involves more refactoring, but we could maybe
> turn CookieMonster::Delegate into CookieMonster::Observer (all the methods are
> observer methods anyway). And then PrerenderCookieStore could register itself
as
> an observer of the CookieMonster it wraps. (This is assuming my other comment
of
> moving the OnCookieChangedForURL call to Chrome's CookieMonster::Delegate
> implementation works.)

Good point. As described above, I'd prefer to not use the Delegate, and Erik was
not fond of it either.

Instead, I came up with a fairly simple way to address this, passing the cookie
store in the URL. Please let know what you think.

Thanks.

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_tracker.h (right):

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/prerende...
chrome/browser/prerender/prerender_tracker.h:28: // Global object for
maintaining prerender state on the IO thread.
On 2014/05/07 20:30:36, Charlie Reis wrote:
> Let's update this comment if it's no longer true.

True again based on the change davidben suggested and which I just implemented.

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/prerende...
chrome/browser/prerender/prerender_tracker.h:122: PrerenderProcessSet
prerender_process_hosts_;
On 2014/05/07 23:17:42, David Benjamin wrote:
> I think I agree with creis that this should perhaps be better on the
> PrerenderManager. PrerenderManager is getting monstrously large, but I'm not
> sure putting new things in PrerenderTracker is the best way to fix that. (I'm
> thinking we can maybe pull PendingSwap out into a separate file.) After the
> various refactors from RenderView -> RenderFrame, PrerenderTracker has become
> some auxiliary object for tracking IO-thread-level state. Seems worth keeping
it
> that way so it's not unclear which component should do what.
> 
> (In ChromeContentBrowserClient, you can go from RPH to PrerenderManager via
> content::BrowserContext which you can exchange for a Profile with
> Profile::FromBrowserContext.)

Done.

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/net_internals/net_internals_ui_browsertest.cc
(right):

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/ui/webui...
chrome/browser/ui/webui/net_internals/net_internals_ui_browsertest.cc:406:
base::MessageLoop::current()->Run();
On 2014/05/07 19:57:00, David Benjamin wrote:
> Nit: Ditto to other comment; this can be a base::RunLoop.

Done.

[tburkard, 2014-05-08 12:49:33]

Fixed another small bug in prerender_tracker in the change I added in the last
iteration (correctly sending notifications about cookie changes only if they are
not from a prerender).

I also addressed all of David's test related requests.

This should have addressed all the feedback I got.

Please take another pass and let me know if there is anything else, or if this
is ok now.

Thanks.

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/net/cook...
File chrome/browser/net/cookie_store_util.cc (right):

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/net/cook...
chrome/browser/net/cookie_store_util.cc:51: this, cookie, removed, cause));
On 2014/05/07 23:17:42, David Benjamin wrote:
> Would it work to use this hook to call
> PrerenderCookieStore::OnCookieChangedForURL? (Appropriately tweaked to check
> CanonicalCookie instead of the URL.) That seems like it might be easier than
> having to hook into two places for the different ways you can set a cookie.
> 
> It also means that the following would work. (Or does it already?)
> 
> 1. Prerender a.com/1. It sets a cookie.
> 2. Prerender a.com/2. It sets a cookie.
> 3. Swap a.com/1 in. Changes are replayed.
> 4. a.com/2's prerender should abort.
> 
> Mind adding a test for that too? :-)

Added test.

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:1115:
ui_test_utils::NavigateToURLWithDisposition(
On 2014/05/07 23:17:42, David Benjamin wrote:
> Nit: This can probably just be
> 
>   ui_test_utils::NavigateToURL(current_browser(),
> test_server()->GetURL(dest_html_file));

Done.

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:4212:
prerender->WaitForLoads(1);
On 2014/05/07 23:17:42, David Benjamin wrote:
> Could you also make this test assert that the prerender sees the new cookie
> value prior to swap?

Done.

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:4232: 
On 2014/05/07 23:17:42, David Benjamin wrote:
> Could you add variants of these tests for cookies set by HTTP headers? You can
> probably do it with some of these test server handlers.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/tools/testserv...

Done.

[davidben, 2014-05-08 21:26:24]

LGTM with comments. Thanks for the tests!

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:315: // It is initialize on the
UI thread as early as possible once the
On 2014/05/08 10:59:54, tburkard wrote:
> On 2014/05/07 23:17:42, David Benjamin wrote:
> > Actually, I'm slightly puzzled why you need the interesting initialization
> > anyway. Is g_browser_process initialized after ChromeContentBrowserClient (I
> > haven't checked)? If not, I'd think you could just set it in this guy's
> > constructor.
> 
> No, I tried that, and it didn't work. If you initialize PrerenderTracker too
> early, it will fail, because something else that needs to have finished
> initializing hasn't completed yet. Dont remember the exact details.

Huh. Strange... PrerenderTracker's constructor doesn't actually do anything. Do
some of the NULL checks in ChromeContentBrowserClient actually fire?

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/net/cook...
File chrome/browser/net/cookie_store_util.cc (right):

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/net/cook...
chrome/browser/net/cookie_store_util.cc:51: this, cookie, removed, cause));
On 2014/05/08 10:59:54, tburkard wrote:
> (1) your concern is valid, it is not working right now.
> (2) what you are suggesting is complicated, because at that point we no longer
> know what process originated the change. Also, Erik was somewhat opposed to
> changing this unless really necessary.

Do we even need to know what process originated it? PrerenderCookieStore isn't
going to write to actual cookie store (unlike with the hooks you're currently
using which won't ignore prerenders) except during
PrerenderCookieStore::ApplyChanges, at which point it can ignore everything with
in_forwarding_mode_.

But if Erik prefers it this way, I'll defer to him.

> (3) the way we can easily fix it is by calling OnCookieChangedForURL after
> ApplyChanges for all the cookies modified. I just added this fix.
> (4) I will add the test in a later iteration of this changelist (not uploaded
in
> this pass), along with the other test fixes.

https://codereview.chromium.org/233353003/diff/370001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/233353003/diff/370001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:4244: ASSERT_EQ(value, "2");
Add a prerender2->WaitForStop() at the end here. It probably doesn't actually
matter, but this way there's no risk of future flake if the test shuts down
before prerender2 exits and we get FINAL_STATUS_APP_TERMINATING.

https://codereview.chromium.org/233353003/diff/370001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_cookie_store.h (right):

https://codereview.chromium.org/233353003/diff/370001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.h:104: return
default_cookie_monster_; }
Style nit: I think we usually put the } on its own line if we have to wrap
accessors.

https://codereview.chromium.org/233353003/diff/370001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_tracker.cc (right):

https://codereview.chromium.org/233353003/diff/370001/chrome/browser/prerende...
chrome/browser/prerender/prerender_tracker.cc:137: }
Why not just do prerender_cookie_store_map_.find(process_id) to see if it's from
a prerender?

https://codereview.chromium.org/233353003/diff/370001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_tracker.h (right):

https://codereview.chromium.org/233353003/diff/370001/chrome/browser/prerende...
chrome/browser/prerender/prerender_tracker.h:29: class PrerenderTracker : public
content::RenderProcessHostObserver {
Nit: This doesn't need to be an RPHObserver anymore.

https://codereview.chromium.org/233353003/diff/370001/chrome/browser/prerende...
chrome/browser/prerender/prerender_tracker.h:64: bool
IsProcessPrerendering(content::RenderProcessHost* process_host);
Nit: This doesn't exist anymore.

[tburkard, 2014-05-09 09:19:16]

Addressed all of David's most recent items.

Once Erik is ok with this, I will commit the change (not before Monday to make
sure it will be past the branch point).

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.h:315: // It is initialize on the
UI thread as early as possible once the
Not sure of the details. Can revisit this again in a follow-up change once this
is committed.

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/net/cook...
File chrome/browser/net/cookie_store_util.cc (right):

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/net/cook...
chrome/browser/net/cookie_store_util.cc:51: this, cookie, removed, cause));
We do need to know what process originated it, because we should not cancel
prerenders based on cookie changes happening inside of prerenders (whether it's
the same prerender or a different prerender).

https://codereview.chromium.org/233353003/diff/370001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/233353003/diff/370001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:4244: ASSERT_EQ(value, "2");
On 2014/05/08 21:26:25, David Benjamin wrote:
> Add a prerender2->WaitForStop() at the end here. It probably doesn't actually
> matter, but this way there's no risk of future flake if the test shuts down
> before prerender2 exits and we get FINAL_STATUS_APP_TERMINATING.

Done.

https://codereview.chromium.org/233353003/diff/370001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_cookie_store.h (right):

https://codereview.chromium.org/233353003/diff/370001/chrome/browser/prerende...
chrome/browser/prerender/prerender_cookie_store.h:104: return
default_cookie_monster_; }
On 2014/05/08 21:26:25, David Benjamin wrote:
> Style nit: I think we usually put the } on its own line if we have to wrap
> accessors.

Done.

https://codereview.chromium.org/233353003/diff/370001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_tracker.cc (right):

https://codereview.chromium.org/233353003/diff/370001/chrome/browser/prerende...
chrome/browser/prerender/prerender_tracker.cc:137: }
On 2014/05/08 21:26:25, David Benjamin wrote:
> Why not just do prerender_cookie_store_map_.find(process_id) to see if it's
from
> a prerender?

Done.

https://codereview.chromium.org/233353003/diff/370001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_tracker.h (right):

https://codereview.chromium.org/233353003/diff/370001/chrome/browser/prerende...
chrome/browser/prerender/prerender_tracker.h:29: class PrerenderTracker : public
content::RenderProcessHostObserver {
On 2014/05/08 21:26:25, David Benjamin wrote:
> Nit: This doesn't need to be an RPHObserver anymore.

Done.

https://codereview.chromium.org/233353003/diff/370001/chrome/browser/prerende...
chrome/browser/prerender/prerender_tracker.h:64: bool
IsProcessPrerendering(content::RenderProcessHost* process_host);
On 2014/05/08 21:26:25, David Benjamin wrote:
> Nit: This doesn't exist anymore.

Done.

[davidben, 2014-05-09 15:56:47]

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/net/cook...
File chrome/browser/net/cookie_store_util.cc (right):

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/net/cook...
chrome/browser/net/cookie_store_util.cc:51: this, cookie, removed, cause));
On 2014/05/09 09:19:17, tburkard wrote:
> We do need to know what process originated it, because we should not cancel
> prerenders based on cookie changes happening inside of prerenders (whether
it's
> the same prerender or a different prerender).

Right, but does the CookieMonster::Delegate ever see cookie changes happening
inside of a prerender? Prerenders write to the PrerenderCookieStore which direct
their writes to changes_cookie_monster_ (which has a NULL delegate) and not
default_cookie_monster_, right? I guess you might get events for updating access
times on reads, but you'd probably want to filter those out, prerender or no.

(Anyway, again, this is all moot if Erik prefers it this way.)

[erikwright (departed), 2014-05-12 14:27:48]

LGTM. Please write a test for CopyCookiesToOtherCookieMonster in a follow-up.

https://codereview.chromium.org/233353003/diff/390001/net/cookies/cookie_mons...
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/233353003/diff/390001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:1438: if (delegate_.get())
Should this be run in the case where (store == NULL)? i.e., when we go
immediately into (loaded_ == true) state?

https://codereview.chromium.org/233353003/diff/390001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:2259: 
nit remove blank

https://codereview.chromium.org/233353003/diff/390001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:2259: 
nit: DCHECK(other) and remove the check on !other unless there is some reason it
could possibly be null in production.

https://codereview.chromium.org/233353003/diff/390001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:2271: ++its.first;
nit: I'm not sure why this is here instead of in the for (;;) header. If it
_can_ go there, I think it makes more sense.

https://codereview.chromium.org/233353003/diff/390001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:2278: base::AutoLock autlock(other->lock_);
nit autlock -> autolock

https://codereview.chromium.org/233353003/diff/390001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:2304: 
nit remove blank

[tburkard, 2014-05-12 14:42:59]

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/net/cook...
File chrome/browser/net/cookie_store_util.cc (right):

https://codereview.chromium.org/233353003/diff/290001/chrome/browser/net/cook...
chrome/browser/net/cookie_store_util.cc:51: this, cookie, removed, cause));
I agree with you that it could work.

But as discussed, Erik prefers it this way.

https://codereview.chromium.org/233353003/diff/390001/net/cookies/cookie_mons...
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/233353003/diff/390001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:1438: if (delegate_.get())
On 2014/05/12 14:27:49, erikwright wrote:
> Should this be run in the case where (store == NULL)? i.e., when we go
> immediately into (loaded_ == true) state?

Yes. We always want to notify when the load has completed - whether it was a
trivial (non-load), or whether it was an actual load.

https://codereview.chromium.org/233353003/diff/390001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:2259: 
On 2014/05/12 14:27:49, erikwright wrote:
> nit remove blank

Done.

https://codereview.chromium.org/233353003/diff/390001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:2259: 
On 2014/05/12 14:27:49, erikwright wrote:
> nit: DCHECK(other) and remove the check on !other unless there is some reason
it
> could possibly be null in production.

Done.

https://codereview.chromium.org/233353003/diff/390001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:2271: ++its.first;
On 2014/05/12 14:27:49, erikwright wrote:
> nit: I'm not sure why this is here instead of in the for (;;) header. If it
> _can_ go there, I think it makes more sense.

Fixed.
I agree with you! As to why the change was done this way: Search for ++its.first
in cookie_monster.cc, and you can see this all over the place :) I had just
copied some existing loop and modified it.

https://codereview.chromium.org/233353003/diff/390001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:2278: base::AutoLock autlock(other->lock_);
On 2014/05/12 14:27:49, erikwright wrote:
> nit autlock -> autolock

Done.

https://codereview.chromium.org/233353003/diff/390001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:2304: 
On 2014/05/12 14:27:49, erikwright wrote:
> nit remove blank

Done.

[tburkard, 2014-05-12 16:37:48]

Committed patchset #23 manually as r269798 (presubmit successful).