PlzNavigate: Fix the failing ContinueWhereILeftOffTest.PostWithPassword test.

PlzNavigate: Fix the failing ContinueWhereILeftOffTest.PostWithPassword test.

This test exercises the session restore code path by posting passwords via a top level FORM post and
then exits the browser and restarts it. Assumption being that after restart we won't have the post data
available as it should have been stripped here https://cs.chromium.org/chromium/src/components/sessions/content/content_serialized_navigation_driver.cc?dr&q=GetSanitizedPageSta&sq=package:chromium&l=101
in the sesson restore code path during exit.

This fails in PlzNavigate because the FrameHostMsg_BeginNavigation IPC only sends the post data via the CommonNavigationParams structure.
Hence when the request eventually completes in the renderer via the FrameMsg_CommitNavigation IPC, we lose the fact that the post contains passwords.
This eventually makes it back to the browser via the PageState which also loses the flag.

Fix is to pass the flag which indicates the post sensitive data like passwords in the ResourceRequestBodyImpl structure.

BUG=655422
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/4dbb42fe4765527a3f0ed359caac5f3dce7b1440
Cr-Commit-Position: refs/heads/master@{#425533}

[ananta, 2016-10-10 23:47:35]

Description was changed from

==========
PlzNavigate: Fix the failing ContinueWhereILeftOffTest.PostWithPassword test.

This test exercises the session restore code path by posting passwords via a top
level FORM post and
then exits the browser and restarts it. Assumption being that the URL which is
restored will be
seviced via a GET request. With PlzNavigate enabled we initiate a top level POST
again which violates
the assumptions of the test.

In general we should be initiating top level requests only for renderer
initiated requests.

Fix is to check whether we are in the session restore code path before reading
the post datsa.

BUG=575210
==========

to

==========
PlzNavigate: Fix the failing ContinueWhereILeftOffTest.PostWithPassword test.

This test exercises the session restore code path by posting passwords via a top
level FORM post and
then exits the browser and restarts it. Assumption being that the URL which is
restored will be
seviced via a GET request. With PlzNavigate enabled we initiate a top level POST
again which violates
the assumptions of the test.

In general we should be initiating top level requests only for renderer
initiated requests.

Fix is to check whether we are in the session restore code path before reading
the post datsa.

BUG=575210
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[ananta, 2016-10-10 23:48:29]

ananta@chromium.org changed reviewers:
+ jam@chromium.org

[ananta, 2016-10-10 23:48:45]

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

[commit-bot: I haz the power, 2016-10-10 23:49:02]

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

[commit-bot: I haz the power, 2016-10-11 00:51:08]

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

[commit-bot: I haz the power, 2016-10-11 00:51:08]

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

[jam, 2016-10-11 15:42:40]

there are test failures?

[ananta, 2016-10-13 00:50:57]

Description was changed from

==========
PlzNavigate: Fix the failing ContinueWhereILeftOffTest.PostWithPassword test.

This test exercises the session restore code path by posting passwords via a top
level FORM post and
then exits the browser and restarts it. Assumption being that the URL which is
restored will be
seviced via a GET request. With PlzNavigate enabled we initiate a top level POST
again which violates
the assumptions of the test.

In general we should be initiating top level requests only for renderer
initiated requests.

Fix is to check whether we are in the session restore code path before reading
the post datsa.

BUG=575210
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
PlzNavigate: Fix the failing ContinueWhereILeftOffTest.PostWithPassword test.

This test exercises the session restore code path by posting passwords via a top
level FORM post and
then exits the browser and restarts it. Assumption being that after restart we
won't have the post data
available as it should have been stripped here
https://cs.chromium.org/chromium/src/components/sessions/content/content_seri...
in the sesson restore code path during exit.

This fails in PlzNavigate because the FrameHostMsg_BeginNavigation IPC only
sends the post data via the CommonNavigationParams structure.
Hence when the request eventually completes in the renderer via the
FrameMsg_CommitNavigation IPC, we lose the fact that the post contains
passwords.
This eventually makes it back to the browser via the PageState which also loses
the flag.

Fix is to pass the flag which indicates the post contains passwords in the
CommonNavigationParams structure.

BUG=655422
==========

[ananta, 2016-10-13 00:56:30]

ananta@chromium.org changed reviewers:
+ clamy@chromium.org, nasko@chromium.org

[ananta, 2016-10-13 00:56:43]

+nasko, clamy

[ananta, 2016-10-13 00:56:50]

Description was changed from

==========
PlzNavigate: Fix the failing ContinueWhereILeftOffTest.PostWithPassword test.

This test exercises the session restore code path by posting passwords via a top
level FORM post and
then exits the browser and restarts it. Assumption being that after restart we
won't have the post data
available as it should have been stripped here
https://cs.chromium.org/chromium/src/components/sessions/content/content_seri...
in the sesson restore code path during exit.

This fails in PlzNavigate because the FrameHostMsg_BeginNavigation IPC only
sends the post data via the CommonNavigationParams structure.
Hence when the request eventually completes in the renderer via the
FrameMsg_CommitNavigation IPC, we lose the fact that the post contains
passwords.
This eventually makes it back to the browser via the PageState which also loses
the flag.

Fix is to pass the flag which indicates the post contains passwords in the
CommonNavigationParams structure.

BUG=655422
==========

to

==========
PlzNavigate: Fix the failing ContinueWhereILeftOffTest.PostWithPassword test.

This test exercises the session restore code path by posting passwords via a top
level FORM post and
then exits the browser and restarts it. Assumption being that after restart we
won't have the post data
available as it should have been stripped here
https://cs.chromium.org/chromium/src/components/sessions/content/content_seri...
in the sesson restore code path during exit.

This fails in PlzNavigate because the FrameHostMsg_BeginNavigation IPC only
sends the post data via the CommonNavigationParams structure.
Hence when the request eventually completes in the renderer via the
FrameMsg_CommitNavigation IPC, we lose the fact that the post contains
passwords.
This eventually makes it back to the browser via the PageState which also loses
the flag.

Fix is to pass the flag which indicates the post contains passwords in the
CommonNavigationParams structure.

BUG=655422
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[ananta, 2016-10-13 00:57:01]

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

[commit-bot: I haz the power, 2016-10-13 00:57:11]

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

[ananta, 2016-10-13 00:58:19]

On 2016/10/11 15:42:40, jam wrote:
> there are test failures?

Yeah. The approach was wrong. Updated the patchset and logged a bug
http://crbug.com/655422 which describes the problem.

[commit-bot: I haz the power, 2016-10-13 01:19:04]

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

[commit-bot: I haz the power, 2016-10-13 01:19:05]

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

[ananta, 2016-10-13 01:42:41]

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

[commit-bot: I haz the power, 2016-10-13 01:42:55]

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

[commit-bot: I haz the power, 2016-10-13 02:32:43]

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

[commit-bot: I haz the power, 2016-10-13 02:32:44]

Dry run: This issue passed the CQ dry run.

[jam, 2016-10-13 04:34:30]

lgtm

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

https://codereview.chromium.org/2406053002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:633: (!info.urlRequest.httpBody().isNull()
?
nit: this would be more readable without the double negative, i.e.

(info.urlRequest.httpBody().isNull() ? false :
info.urlRequest.httpBody().containsPasswordData())

[clamy, 2016-10-13 12:55:13]

Description was changed from

==========
PlzNavigate: Fix the failing ContinueWhereILeftOffTest.PostWithPassword test.

This test exercises the session restore code path by posting passwords via a top
level FORM post and
then exits the browser and restarts it. Assumption being that after restart we
won't have the post data
available as it should have been stripped here
https://cs.chromium.org/chromium/src/components/sessions/content/content_seri...
in the sesson restore code path during exit.

This fails in PlzNavigate because the FrameHostMsg_BeginNavigation IPC only
sends the post data via the CommonNavigationParams structure.
Hence when the request eventually completes in the renderer via the
FrameMsg_CommitNavigation IPC, we lose the fact that the post contains
passwords.
This eventually makes it back to the browser via the PageState which also loses
the flag.

Fix is to pass the flag which indicates the post contains passwords in the
CommonNavigationParams structure.

BUG=655422
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
PlzNavigate: Fix the failing ContinueWhereILeftOffTest.PostWithPassword test.

This test exercises the session restore code path by posting passwords via a top
level FORM post and
then exits the browser and restarts it. Assumption being that after restart we
won't have the post data
available as it should have been stripped here
https://cs.chromium.org/chromium/src/components/sessions/content/content_seri...
in the sesson restore code path during exit.

This fails in PlzNavigate because the FrameHostMsg_BeginNavigation IPC only
sends the post data via the CommonNavigationParams structure.
Hence when the request eventually completes in the renderer via the
FrameMsg_CommitNavigation IPC, we lose the fact that the post contains
passwords.
This eventually makes it back to the browser via the PageState which also loses
the flag.

Fix is to pass the flag which indicates the post contains passwords in the
CommonNavigationParams structure.

BUG=655422
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[clamy, 2016-10-13 12:56:17]

Thanks! One comment below. I'm also ccing lukasza@ since he worked on other
cases involving the post body that may be affected by this issue as well (even
if there may be no test coverage for the affected uses cases).

https://codereview.chromium.org/2406053002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/2406053002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:692: navigation_start,
method, false, post_body ? post_body : post_data_);
Does the stripping of password data from the post_body happens on the renderer
side before we send the PageState to the renderer or does it happen when
serializing it to disk? If the later, it would be possible when doing a back
forward navigation that we lose the information that the post data containing a
password if we don't store it alongside the post body in the
FrameNavigationEntry.

Another place where we could lose this information is when the navigation goes
through the OpenURL path (and transfer navigations in the current architecture).
All in all, I wonder if it would not be better to add a boolean on the post body
explicitly rather than try to store it along and possibly miss edge cases.

[Łukasz Anforowicz, 2016-10-13 18:56:29]

On 2016/10/13 12:56:17, clamy wrote:
> Thanks! One comment below. I'm also ccing lukasza@ since he worked on other
> cases involving the post body that may be affected by this issue as well (even
> if there may be no test coverage for the affected uses cases).
> 
>
https://codereview.chromium.org/2406053002/diff/60001/content/browser/frame_h...
> File content/browser/frame_host/navigation_entry_impl.cc (right):
> 
>
https://codereview.chromium.org/2406053002/diff/60001/content/browser/frame_h...
> content/browser/frame_host/navigation_entry_impl.cc:692: navigation_start,
> method, false, post_body ? post_body : post_data_);
> Does the stripping of password data from the post_body happens on the renderer
> side before we send the PageState to the renderer or does it happen when
> serializing it to disk? If the later, it would be possible when doing a back
> forward navigation that we lose the information that the post data containing
a
> password if we don't store it alongside the post body in the
> FrameNavigationEntry.
> 
> Another place where we could lose this information is when the navigation goes
> through the OpenURL path (and transfer navigations in the current
architecture).
> All in all, I wonder if it would not be better to add a boolean on the post
body
> explicitly rather than try to store it along and possibly miss edge cases.

Yes - FrameHostMsg_OpenURL_Params passes original / unstripped POST body to the
browser, so it seems that the |post_contains_passwords| boolean flag would also
be required here.  I like clamy@'s idea to add the boolean flag to
content::ResourceRequestBodyImpl (it makes sense to keep the boolean flag next
to the POST body that is described by the flag).  Note that POST body sent via
FrameHostMsg_OpenURL_Params can travel all the way to the //chrome layer in the
browser, including Java land - therefore it might be necessary to ensure the new
boolean flag is preserved across the JNI (you can probably look at the
Java-related changes in https://codereview.chromium.org/2038233002 - for example
at content/common/android/resource_request_body_android.cc;  if
marshalling/serializing/flattening into Java seems undesirable then I guess we
can reopen the discussion from https://codereview.chromium.org/2038233002/#msg18
about ref-counting the native object instead).

Also - note that my comment above assumes that we really need to preserve the
new |post_contains_passwords| boolean flag along with
content::ResourceRequestBodyImpl - this assumption seems reasonable to me, given
that POST data in FrameHostMsg_OpenURL_Params is unstripped / can contain
passwords.  OTOH, if this assumption is wrong, then my whole comment above is
probably invalid :-).

[ananta, 2016-10-13 20:13:11]

Description was changed from

==========
PlzNavigate: Fix the failing ContinueWhereILeftOffTest.PostWithPassword test.

This test exercises the session restore code path by posting passwords via a top
level FORM post and
then exits the browser and restarts it. Assumption being that after restart we
won't have the post data
available as it should have been stripped here
https://cs.chromium.org/chromium/src/components/sessions/content/content_seri...
in the sesson restore code path during exit.

This fails in PlzNavigate because the FrameHostMsg_BeginNavigation IPC only
sends the post data via the CommonNavigationParams structure.
Hence when the request eventually completes in the renderer via the
FrameMsg_CommitNavigation IPC, we lose the fact that the post contains
passwords.
This eventually makes it back to the browser via the PageState which also loses
the flag.

Fix is to pass the flag which indicates the post contains passwords in the
CommonNavigationParams structure.

BUG=655422
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
PlzNavigate: Fix the failing ContinueWhereILeftOffTest.PostWithPassword test.

This test exercises the session restore code path by posting passwords via a top
level FORM post and
then exits the browser and restarts it. Assumption being that after restart we
won't have the post data
available as it should have been stripped here
https://cs.chromium.org/chromium/src/components/sessions/content/content_seri...
in the sesson restore code path during exit.

This fails in PlzNavigate because the FrameHostMsg_BeginNavigation IPC only
sends the post data via the CommonNavigationParams structure.
Hence when the request eventually completes in the renderer via the
FrameMsg_CommitNavigation IPC, we lose the fact that the post contains
passwords.
This eventually makes it back to the browser via the PageState which also loses
the flag.

Fix is to pass the flag which indicates the post sensitive data like passwords
in the ResourceRequestBodyImpl structure.

BUG=655422
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[ananta, 2016-10-13 20:16:43]

https://codereview.chromium.org/2406053002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/2406053002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:692: navigation_start,
method, false, post_body ? post_body : post_data_);
On 2016/10/13 12:56:17, clamy wrote:
> Does the stripping of password data from the post_body happens on the renderer
> side before we send the PageState to the renderer or does it happen when
> serializing it to disk? If the later, it would be possible when doing a back
> forward navigation that we lose the information that the post data containing
a
> password if we don't store it alongside the post body in the
> FrameNavigationEntry.
> 
> Another place where we could lose this information is when the navigation goes
> through the OpenURL path (and transfer navigations in the current
architecture).
> All in all, I wonder if it would not be better to add a boolean on the post
body
> explicitly rather than try to store it along and possibly miss edge cases.

Thanks for pointing out the issue with OpenURL. As far as the stripping of the
password data from PageState is concerned, that happens in the browser during
shutdown. That works correctly here.

I added the flag to the ResourceRequestBodyImpl class which works correctly for
OpenURL and the BeginNavigation code path. I updated the relevant places which
need to ensure that the flag gets marshaled across. PTAL and let me know if we
missed something.

[ananta, 2016-10-13 20:17:47]

On 2016/10/13 18:56:29, Łukasz Anforowicz wrote:
> On 2016/10/13 12:56:17, clamy wrote:
> > Thanks! One comment below. I'm also ccing lukasza@ since he worked on other
> > cases involving the post body that may be affected by this issue as well
(even
> > if there may be no test coverage for the affected uses cases).
> > 
> >
>
https://codereview.chromium.org/2406053002/diff/60001/content/browser/frame_h...
> > File content/browser/frame_host/navigation_entry_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2406053002/diff/60001/content/browser/frame_h...
> > content/browser/frame_host/navigation_entry_impl.cc:692: navigation_start,
> > method, false, post_body ? post_body : post_data_);
> > Does the stripping of password data from the post_body happens on the
renderer
> > side before we send the PageState to the renderer or does it happen when
> > serializing it to disk? If the later, it would be possible when doing a back
> > forward navigation that we lose the information that the post data
containing
> a
> > password if we don't store it alongside the post body in the
> > FrameNavigationEntry.
> > 
> > Another place where we could lose this information is when the navigation
goes
> > through the OpenURL path (and transfer navigations in the current
> architecture).
> > All in all, I wonder if it would not be better to add a boolean on the post
> body
> > explicitly rather than try to store it along and possibly miss edge cases.
> 
> Yes - FrameHostMsg_OpenURL_Params passes original / unstripped POST body to
the
> browser, so it seems that the |post_contains_passwords| boolean flag would
also
> be required here.  I like clamy@'s idea to add the boolean flag to
> content::ResourceRequestBodyImpl (it makes sense to keep the boolean flag next
> to the POST body that is described by the flag).  Note that POST body sent via
> FrameHostMsg_OpenURL_Params can travel all the way to the //chrome layer in
the
> browser, including Java land - therefore it might be necessary to ensure the
new
> boolean flag is preserved across the JNI (you can probably look at the
> Java-related changes in https://codereview.chromium.org/2038233002 - for
example
> at content/common/android/resource_request_body_android.cc;  if
> marshalling/serializing/flattening into Java seems undesirable then I guess we
> can reopen the discussion from
https://codereview.chromium.org/2038233002/#msg18
> about ref-counting the native object instead).
> 
> Also - note that my comment above assumes that we really need to preserve the
> new |post_contains_passwords| boolean flag along with
> content::ResourceRequestBodyImpl - this assumption seems reasonable to me,
given
> that POST data in FrameHostMsg_OpenURL_Params is unstripped / can contain
> passwords.  OTOH, if this assumption is wrong, then my whole comment above is
> probably invalid :-).

Thanks Lukasz. I updated the patchset to send the flag via the
ResourceRequestBodyImpl class. From what I see, the android code
should work correctly. Please take a peek.

[ananta, 2016-10-13 20:17:58]

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

[commit-bot: I haz the power, 2016-10-13 20:18:27]

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

[commit-bot: I haz the power, 2016-10-13 20:42:01]

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

[commit-bot: I haz the power, 2016-10-13 20:42:02]

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

[ananta, 2016-10-13 22:14:23]

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

[commit-bot: I haz the power, 2016-10-13 22:14:41]

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

[ananta, 2016-10-13 22:21:38]

On 2016/10/13 20:17:47, ananta wrote:
> On 2016/10/13 18:56:29, Łukasz Anforowicz wrote:
> > On 2016/10/13 12:56:17, clamy wrote:
> > > Thanks! One comment below. I'm also ccing lukasza@ since he worked on
other
> > > cases involving the post body that may be affected by this issue as well
> (even
> > > if there may be no test coverage for the affected uses cases).
> > > 
> > >
> >
>
https://codereview.chromium.org/2406053002/diff/60001/content/browser/frame_h...
> > > File content/browser/frame_host/navigation_entry_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2406053002/diff/60001/content/browser/frame_h...
> > > content/browser/frame_host/navigation_entry_impl.cc:692: navigation_start,
> > > method, false, post_body ? post_body : post_data_);
> > > Does the stripping of password data from the post_body happens on the
> renderer
> > > side before we send the PageState to the renderer or does it happen when
> > > serializing it to disk? If the later, it would be possible when doing a
back
> > > forward navigation that we lose the information that the post data
> containing
> > a
> > > password if we don't store it alongside the post body in the
> > > FrameNavigationEntry.
> > > 
> > > Another place where we could lose this information is when the navigation
> goes
> > > through the OpenURL path (and transfer navigations in the current
> > architecture).
> > > All in all, I wonder if it would not be better to add a boolean on the
post
> > body
> > > explicitly rather than try to store it along and possibly miss edge cases.
> > 
> > Yes - FrameHostMsg_OpenURL_Params passes original / unstripped POST body to
> the
> > browser, so it seems that the |post_contains_passwords| boolean flag would
> also
> > be required here.  I like clamy@'s idea to add the boolean flag to
> > content::ResourceRequestBodyImpl (it makes sense to keep the boolean flag
next
> > to the POST body that is described by the flag).  Note that POST body sent
via
> > FrameHostMsg_OpenURL_Params can travel all the way to the //chrome layer in
> the
> > browser, including Java land - therefore it might be necessary to ensure the
> new
> > boolean flag is preserved across the JNI (you can probably look at the
> > Java-related changes in https://codereview.chromium.org/2038233002 - for
> example
> > at content/common/android/resource_request_body_android.cc;  if
> > marshalling/serializing/flattening into Java seems undesirable then I guess
we
> > can reopen the discussion from
> https://codereview.chromium.org/2038233002/#msg18
> > about ref-counting the native object instead).
> > 
> > Also - note that my comment above assumes that we really need to preserve
the
> > new |post_contains_passwords| boolean flag along with
> > content::ResourceRequestBodyImpl - this assumption seems reasonable to me,
> given
> > that POST data in FrameHostMsg_OpenURL_Params is unstripped / can contain
> > passwords.  OTOH, if this assumption is wrong, then my whole comment above
is
> > probably invalid :-).
> 
> Thanks Lukasz. I updated the patchset to send the flag via the
> ResourceRequestBodyImpl class. From what I see, the android code
> should work correctly. Please take a peek.

Lukasz, I reverted the change to the page_state_serialization.cc as we don't
want the PageState format to change.
Looking at the code here, I am not sure if we have a use case for the new flag?
https://cs.chromium.org/chromium/src/content/common/android/resource_request_...

[nasko, 2016-10-13 22:28:54]

The code as is in PS8 looks good. I'd defer to lukasza@ on the details about
Android/Java.

[commit-bot: I haz the power, 2016-10-13 23:28:00]

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

[commit-bot: I haz the power, 2016-10-13 23:28:01]

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

[clamy, 2016-10-14 12:25:36]

Thanks! Lgtm. I'm deferring to lukasza@ for the Android issues.

https://codereview.chromium.org/2406053002/diff/140001/content/common/resourc...
File content/common/resource_request_body_impl.h (right):

https://codereview.chromium.org/2406053002/diff/140001/content/common/resourc...
content/common/resource_request_body_impl.h:73: // Set to true if the post data
contains sensitive information like
nit: blank space above.

[ananta, 2016-10-14 20:08:42]

https://codereview.chromium.org/2406053002/diff/140001/content/common/resourc...
File content/common/resource_request_body_impl.h (right):

https://codereview.chromium.org/2406053002/diff/140001/content/common/resourc...
content/common/resource_request_body_impl.h:73: // Set to true if the post data
contains sensitive information like
On 2016/10/14 12:25:36, clamy wrote:
> nit: blank space above.

Done.

[ananta, 2016-10-14 20:08:52]

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

[commit-bot: I haz the power, 2016-10-14 20:09:16]

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

[commit-bot: I haz the power, 2016-10-14 21:36:08]

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

[commit-bot: I haz the power, 2016-10-14 21:36:09]

Dry run: This issue passed the CQ dry run.

[jam, 2016-10-14 22:36:04]

lgtm

https://codereview.chromium.org/2406053002/diff/160001/content/common/resourc...
File content/common/resource_messages.cc (right):

https://codereview.chromium.org/2406053002/diff/160001/content/common/resourc...
content/common/resource_messages.cc:428: bool contains_sensitive_info = false;
nit: no need to initialize

https://codereview.chromium.org/2406053002/diff/160001/content/common/resourc...
File content/common/resource_request_body_impl.h (right):

https://codereview.chromium.org/2406053002/diff/160001/content/common/resourc...
content/common/resource_request_body_impl.h:62: // information like passwords.
nit: no need to duplicate comment on setters and below. i guess put them just
here and remove line 74-75 for consistency

[Łukasz Anforowicz, 2016-10-14 22:45:08]

AFAIK getting ResourceRequestBody[Impl] across Java boundaries depends on
EncodeResourceRequestBody, which doesn't currently support the new boolean flag
- |contains_sensitive_info|.  I think Java support is needed, otherwise the flag
will get lost (and passwords won't get cleared) when ResourceRequestBody goes
through OpenURL path, through Java and then back to
NavigationController::LoadURLWithParams.  So - I think we shouldn't land this CL
without supporting passing the new flag through Java (unless I am missing
something?).

We could cover |contains_sensitive_info| flag in EncodeResourceRequestBody, but
this 1) would necessitate bumping up the version number of page state
serialization and 2) it is not clear if serializing this flag to disk is
desirable (it should always be |false| on disk AFAICT).  creis@ is the official
OWNER of the page state serialization code, so he would probably want to comment
on this.

Alternatively we could reconsider ref-counting ResourceRequestBody from Java
side (no need to serialize/deserialize in this case).  This approach was
implemented in patchset #6 in https://codereview.chromium.org/2038233002, but
was abandoned because it was frowned upon by boliu@.

[ananta, 2016-10-14 23:43:24]

On 2016/10/14 22:45:08, Łukasz Anforowicz wrote:
> AFAIK getting ResourceRequestBody[Impl] across Java boundaries depends on
> EncodeResourceRequestBody, which doesn't currently support the new boolean
flag
> - |contains_sensitive_info|.  I think Java support is needed, otherwise the
flag
> will get lost (and passwords won't get cleared) when ResourceRequestBody goes
> through OpenURL path, through Java and then back to
> NavigationController::LoadURLWithParams.  So - I think we shouldn't land this
CL
> without supporting passing the new flag through Java (unless I am missing
> something?).
> 
> We could cover |contains_sensitive_info| flag in EncodeResourceRequestBody,
but
> this 1) would necessitate bumping up the version number of page state
> serialization and 2) it is not clear if serializing this flag to disk is
> desirable (it should always be |false| on disk AFAICT).  creis@ is the
official
> OWNER of the page state serialization code, so he would probably want to
comment
> on this.
> 
> Alternatively we could reconsider ref-counting ResourceRequestBody from Java
> side (no need to serialize/deserialize in this case).  This approach was
> implemented in patchset #6 in https://codereview.chromium.org/2038233002, but
> was abandoned because it was frowned upon by boliu@.

Thanks. As per  our discussion I updated the EncodeResourceRequestBody and
DecodeResourceRequestBody to
serialize and deserialize the contains_sensitive_info bool. Checked with creis
about this.

[ananta, 2016-10-14 23:44:13]

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

[commit-bot: I haz the power, 2016-10-14 23:44:27]

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

[Łukasz Anforowicz, 2016-10-15 00:02:26]

lukasza@chromium.org changed reviewers:
+ lukasza@chromium.org

[Łukasz Anforowicz, 2016-10-15 00:02:27]

Thanks - I've shared a suggestion below for improving the comments (and asked
about one potential issue/omission in resource_messages.cc), but the Java/native
marshaling LGTM.

https://codereview.chromium.org/2406053002/diff/180001/content/common/page_st...
File content/common/page_state_serialization.cc (right):

https://codereview.chromium.org/2406053002/diff/180001/content/common/page_st...
content/common/page_state_serialization.cc:769: // this is being explicitl
deserialized.
nit:explicitly
nit:s/this/that/

https://codereview.chromium.org/2406053002/diff/180001/content/common/page_st...
content/common/page_state_serialization.cc:781: // PageState version update.
Maybe we could tweak the comment to say why EncodeResourceRequestBody is
different from WriteResourceRequestBody: EncodeResourceRequestBody covers
additional data (e.g. |contains_sensitive_info|) because it needs to marshal all
in-memory data between native code and Java (and WriteResourceRequestBody only
covers data that needs to be saved to disk).

https://codereview.chromium.org/2406053002/diff/180001/content/common/resourc...
File content/common/resource_messages.cc (right):

https://codereview.chromium.org/2406053002/diff/180001/content/common/resourc...
content/common/resource_messages.cc:409: WriteParam(m,
p->contains_sensitive_info());
Do we also need to modify ParamTraits<...>::GetSize?

[ananta, 2016-10-15 00:23:32]

https://codereview.chromium.org/2406053002/diff/160001/content/common/resourc...
File content/common/resource_messages.cc (right):

https://codereview.chromium.org/2406053002/diff/160001/content/common/resourc...
content/common/resource_messages.cc:428: bool contains_sensitive_info = false;
On 2016/10/14 22:36:03, jam wrote:
> nit: no need to initialize

Done.

https://codereview.chromium.org/2406053002/diff/160001/content/common/resourc...
File content/common/resource_request_body_impl.h (right):

https://codereview.chromium.org/2406053002/diff/160001/content/common/resourc...
content/common/resource_request_body_impl.h:62: // information like passwords.
On 2016/10/14 22:36:03, jam wrote:
> nit: no need to duplicate comment on setters and below. i guess put them just
> here and remove line 74-75 for consistency

Done.

https://codereview.chromium.org/2406053002/diff/180001/content/common/page_st...
File content/common/page_state_serialization.cc (right):

https://codereview.chromium.org/2406053002/diff/180001/content/common/page_st...
content/common/page_state_serialization.cc:769: // this is being explicitl
deserialized.
On 2016/10/15 00:02:27, Łukasz Anforowicz wrote:
> nit:explicitly
> nit:s/this/that/

Done.

https://codereview.chromium.org/2406053002/diff/180001/content/common/page_st...
content/common/page_state_serialization.cc:781: // PageState version update.
On 2016/10/15 00:02:27, Łukasz Anforowicz wrote:
> Maybe we could tweak the comment to say why EncodeResourceRequestBody is
> different from WriteResourceRequestBody: EncodeResourceRequestBody covers
> additional data (e.g. |contains_sensitive_info|) because it needs to marshal
all
> in-memory data between native code and Java (and WriteResourceRequestBody only
> covers data that needs to be saved to disk).

Done.

https://codereview.chromium.org/2406053002/diff/180001/content/common/resourc...
File content/common/resource_messages.cc (right):

https://codereview.chromium.org/2406053002/diff/180001/content/common/resourc...
content/common/resource_messages.cc:409: WriteParam(m,
p->contains_sensitive_info());
On 2016/10/15 00:02:27, Łukasz Anforowicz wrote:
> Do we also need to modify ParamTraits<...>::GetSize?

Thanks. Done.

[ananta, 2016-10-15 00:23:44]

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

[commit-bot: I haz the power, 2016-10-15 00:24:04]

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

[commit-bot: I haz the power, 2016-10-15 01:20:19]

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

[commit-bot: I haz the power, 2016-10-15 01:20:20]

Dry run: This issue passed the CQ dry run.

[ananta, 2016-10-15 01:46:43]

The CQ bit was checked by ananta@chromium.org

[ananta, 2016-10-15 01:46:43]

The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org,
jam@chromium.org, lukasza@chromium.org
Link to the patchset: https://codereview.chromium.org/2406053002/#ps200001
(title: "Address comments")

[ananta, 2016-10-15 01:46:46]

The CQ bit was unchecked by ananta@chromium.org

[ananta, 2016-10-15 01:47:46]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2016-10-15 01:47:58]

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

[commit-bot: I haz the power, 2016-10-15 01:53:42]

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

[commit-bot: I haz the power, 2016-10-15 01:53:44]

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

[Will Harris, 2016-10-15 02:02:43]

rs lgtm for resource_messages.cc

[ananta, 2016-10-15 02:03:31]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2016-10-15 02:03:44]

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

[commit-bot: I haz the power, 2016-10-15 02:09:51]

Description was changed from

==========
PlzNavigate: Fix the failing ContinueWhereILeftOffTest.PostWithPassword test.

This test exercises the session restore code path by posting passwords via a top
level FORM post and
then exits the browser and restarts it. Assumption being that after restart we
won't have the post data
available as it should have been stripped here
https://cs.chromium.org/chromium/src/components/sessions/content/content_seri...
in the sesson restore code path during exit.

This fails in PlzNavigate because the FrameHostMsg_BeginNavigation IPC only
sends the post data via the CommonNavigationParams structure.
Hence when the request eventually completes in the renderer via the
FrameMsg_CommitNavigation IPC, we lose the fact that the post contains
passwords.
This eventually makes it back to the browser via the PageState which also loses
the flag.

Fix is to pass the flag which indicates the post sensitive data like passwords
in the ResourceRequestBodyImpl structure.

BUG=655422
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
PlzNavigate: Fix the failing ContinueWhereILeftOffTest.PostWithPassword test.

This test exercises the session restore code path by posting passwords via a top
level FORM post and
then exits the browser and restarts it. Assumption being that after restart we
won't have the post data
available as it should have been stripped here
https://cs.chromium.org/chromium/src/components/sessions/content/content_seri...
in the sesson restore code path during exit.

This fails in PlzNavigate because the FrameHostMsg_BeginNavigation IPC only
sends the post data via the CommonNavigationParams structure.
Hence when the request eventually completes in the renderer via the
FrameMsg_CommitNavigation IPC, we lose the fact that the post contains
passwords.
This eventually makes it back to the browser via the PageState which also loses
the flag.

Fix is to pass the flag which indicates the post sensitive data like passwords
in the ResourceRequestBodyImpl structure.

BUG=655422
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-10-15 02:09:52]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-10-15 02:11:41]

Description was changed from

==========
PlzNavigate: Fix the failing ContinueWhereILeftOffTest.PostWithPassword test.

This test exercises the session restore code path by posting passwords via a top
level FORM post and
then exits the browser and restarts it. Assumption being that after restart we
won't have the post data
available as it should have been stripped here
https://cs.chromium.org/chromium/src/components/sessions/content/content_seri...
in the sesson restore code path during exit.

This fails in PlzNavigate because the FrameHostMsg_BeginNavigation IPC only
sends the post data via the CommonNavigationParams structure.
Hence when the request eventually completes in the renderer via the
FrameMsg_CommitNavigation IPC, we lose the fact that the post contains
passwords.
This eventually makes it back to the browser via the PageState which also loses
the flag.

Fix is to pass the flag which indicates the post sensitive data like passwords
in the ResourceRequestBodyImpl structure.

BUG=655422
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
PlzNavigate: Fix the failing ContinueWhereILeftOffTest.PostWithPassword test.

This test exercises the session restore code path by posting passwords via a top
level FORM post and
then exits the browser and restarts it. Assumption being that after restart we
won't have the post data
available as it should have been stripped here
https://cs.chromium.org/chromium/src/components/sessions/content/content_seri...
in the sesson restore code path during exit.

This fails in PlzNavigate because the FrameHostMsg_BeginNavigation IPC only
sends the post data via the CommonNavigationParams structure.
Hence when the request eventually completes in the renderer via the
FrameMsg_CommitNavigation IPC, we lose the fact that the post contains
passwords.
This eventually makes it back to the browser via the PageState which also loses
the flag.

Fix is to pass the flag which indicates the post sensitive data like passwords
in the ResourceRequestBodyImpl structure.

BUG=655422
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/4dbb42fe4765527a3f0ed359caac5f3dce7b1440
Cr-Commit-Position: refs/heads/master@{#425533}
==========

[commit-bot: I haz the power, 2016-10-15 02:11:42]

Patchset 11 (id:??) landed as
https://crrev.com/4dbb42fe4765527a3f0ed359caac5f3dce7b1440
Cr-Commit-Position: refs/heads/master@{#425533}