Simplify WebContentsState::GetContentsStateAsByteBuffer

Simplify WebContentsState::GetContentsStateAsByteBuffer

This CL remove some conditions that must never be true:

* (entry_count == 0 && pending_index ==  0) because
  (entry_count == 0 => pending_index == -1)

* (current_index == -1 && entry_count >  0) because
  (current_index == -1 => entry_count == 0)

A similar simplification can be found in:
https://codereview.chromium.org/2762363002/

This CL was done while trying to understand the crashes in:
https://crbug.com/709431


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

Review-Url: https://codereview.chromium.org/2802213002
Cr-Commit-Position: refs/heads/master@{#469178}
Committed: https://chromium.googlesource.com/chromium/src/+/02084c771e830293de3cc4919d29a83341558035

[arthursonzogni, 2017-04-07 12:34:57]

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

[commit-bot: I haz the power, 2017-04-07 12:35:13]

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

[arthursonzogni, 2017-04-07 12:37:44]

Description was changed from

==========
Simplify WebContentsState::GetContentsStateAsByteBuffer

This CL remove some conditions that must never be true:

* (entry_count == 0 && pending_index ==  0) because
  (entry_count == 0 => pending_index == -1)

* (current_index == -1 && entry_count >  0) because
  (current_index == -1 => entry_count == 0)

This CL was done while trying to understand the crashes in:
https://crbug.com/709431

BUG=709431
==========

to

==========
Simplify WebContentsState::GetContentsStateAsByteBuffer

This CL remove some conditions that must never be true:

* (entry_count == 0 && pending_index ==  0) because
  (entry_count == 0 => pending_index == -1)

* (current_index == -1 && entry_count >  0) because
  (current_index == -1 => entry_count == 0)

A similar simplification can be found in:
https://codereview.chromium.org/2762363002/

This CL was done while trying to understand the crashes in:
https://crbug.com/709431


BUG=709431
==========

[arthursonzogni, 2017-04-07 13:02:40]

arthursonzogni@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[arthursonzogni, 2017-04-07 13:02:41]

Hi @creis and @dfalcantara

Please take a look. Thanks!

@dfalcantara: as an OWNER and main contributor of this file.
@creis: Can you confirm the assertions I have made about the
        navigation entries are true?

+CC @clamy and @eugenebut

[commit-bot: I haz the power, 2017-04-07 13:09:41]

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

[commit-bot: I haz the power, 2017-04-07 13:09:42]

Dry run: This issue passed the CQ dry run.

[Eugene But (OOO till 7-30), 2017-04-07 15:21:06]

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

[Eugene But (OOO till 7-30), 2017-04-07 15:21:07]

https://codereview.chromium.org/2802213002/diff/1/chrome/browser/android/tab_...
File chrome/browser/android/tab_state.cc (right):

https://codereview.chromium.org/2802213002/diff/1/chrome/browser/android/tab_...
chrome/browser/android/tab_state.cc:308: navigations[i] = (i == pending_index) ?
controller.GetPendingEntry()
I was under impression that if |i == pending_index)| then
|controller.GetPendingEntry() == controller.GetEntryAtIndex(i)|. And that this
code is equivalent to  |navigations[i] = controller.GetEntryAtIndex(i);|

[arthursonzogni, 2017-04-10 08:16:36]

Thanks Eugene.

https://codereview.chromium.org/2802213002/diff/1/chrome/browser/android/tab_...
File chrome/browser/android/tab_state.cc (right):

https://codereview.chromium.org/2802213002/diff/1/chrome/browser/android/tab_...
chrome/browser/android/tab_state.cc:308: navigations[i] = (i == pending_index) ?
controller.GetPendingEntry()
On 2017/04/07 15:21:07, Eugene But wrote:
> I was under impression that if |i == pending_index)| then
> |controller.GetPendingEntry() == controller.GetEntryAtIndex(i)|. And that this
> code is equivalent to  |navigations[i] = controller.GetEntryAtIndex(i);|

It would have been nice if the code could be simplified a little more like that.
I have taken a look, I don't think that |controller.GetPendingEntry()| and
|controller.GetEntryAtIndex(i)| returns the same object when i == pending_entry.

Take a look at the comment around the pointer returned by GetPendingEntry():
```
// An entry we haven't gotten a response for yet.  This will be discarded
// when we navigate again.  It's used only so we know what the currently
// displayed tab is.
//
// This may refer to an item in the entries_ list if the pending_entry_index_
// == -1, or it may be its own entry that should be deleted. Be careful with
// the memory management.
NavigationEntryImpl* pending_entry_;
```
So if |controller.GetPendingEntry() == controller.GetEntryAtIndex(i)|
Then  |pending_index == -1|
Then  |i != pending_index|

[Eugene But (OOO till 7-30), 2017-04-10 15:57:24]

> So if |controller.GetPendingEntry() == controller.GetEntryAtIndex(i)|
> Then  |pending_index == -1|
Thanks! I thought that if |controller.GetPendingEntry() ==
controller.GetEntryAtIndex(i)| then pending entry is committed and if pending
entry is committed then pending_entry_index_ != -1.

I feel that comment you referred conflicts with this comment:

  // The index of the pending entry if it is in entries_, or -1 if
  // pending_entry_ is a new entry (created by LoadURL).
  int pending_entry_index_;

but maybe I just misunderstood the comments :)

[gone, 2017-04-11 04:49:57]

I actually barely touch native code.  The only reason my fingerprint is on this
is because I moved it upstream:
https://chrome-internal-review.googlesource.com/c/178702/

FWIW that line seems to come from at least 2011.  Whoever the original owner of
that line is is long gone, so I'll have to defer to Eugene on your patch's
correctness.

[arthursonzogni, 2017-04-11 08:13:22]

On 2017/04/10 15:57:24, Eugene But wrote:
> > So if |controller.GetPendingEntry() == controller.GetEntryAtIndex(i)|
> > Then  |pending_index == -1|
> Thanks! I thought that if |controller.GetPendingEntry() ==
> controller.GetEntryAtIndex(i)| then pending entry is committed and if pending
> entry is committed then pending_entry_index_ != -1.
> 
> I feel that comment you referred conflicts with this comment:
> 
>   // The index of the pending entry if it is in entries_, or -1 if
>   // pending_entry_ is a new entry (created by LoadURL).
>   int pending_entry_index_;
> 
> but maybe I just misunderstood the comments :)

Yes, they are in conflict. I think there is an error in the first one.

By looking at the code, I think there is 2 cases:

1) The pending entry is a new navigation
* pending_index == -1
* GetPendingEntry() != GetEntryAtIndex(i)  (0 <= i <= entry_count)

2) The pending entry is an old navigation (BackForward/Reload?)
* pending_index != -1
* GetPendingEntry() != GetEntryAtIndex(pending_index) 

I will prepare another patch that will fix the first comment and
apply your suggestion.

[arthursonzogni, 2017-04-11 08:16:58]

On 2017/04/11 08:13:22, arthursonzogni wrote:
> On 2017/04/10 15:57:24, Eugene But wrote:
> > > So if |controller.GetPendingEntry() == controller.GetEntryAtIndex(i)|
> > > Then  |pending_index == -1|
> > Thanks! I thought that if |controller.GetPendingEntry() ==
> > controller.GetEntryAtIndex(i)| then pending entry is committed and if
pending
> > entry is committed then pending_entry_index_ != -1.
> > 
> > I feel that comment you referred conflicts with this comment:
> > 
> >   // The index of the pending entry if it is in entries_, or -1 if
> >   // pending_entry_ is a new entry (created by LoadURL).
> >   int pending_entry_index_;
> > 
> > but maybe I just misunderstood the comments :)
> 
> Yes, they are in conflict. I think there is an error in the first one.
> 
> By looking at the code, I think there is 2 cases:
> 
> 1) The pending entry is a new navigation
> * pending_index == -1
> * GetPendingEntry() != GetEntryAtIndex(i)  (0 <= i <= entry_count)
> 
> 2) The pending entry is an old navigation (BackForward/Reload?)
> * pending_index != -1
> * GetPendingEntry() != GetEntryAtIndex(pending_index) 
> 
> I will prepare another patch that will fix the first comment and
> apply your suggestion.

Oops, I wrote an error(s/!=/==) in the last message. A correction:

1) The pending entry is a new navigation
* pending_index == -1
* GetPendingEntry() != GetEntryAtIndex(i)  (0 <= i <= entry_count)

2) The pending entry is an old navigation (BackForward/Reload?)
* pending_index != -1
* GetPendingEntry() == GetEntryAtIndex(pending_index)

[arthursonzogni, 2017-04-11 08:34:19]

Description was changed from

==========
Simplify WebContentsState::GetContentsStateAsByteBuffer

This CL remove some conditions that must never be true:

* (entry_count == 0 && pending_index ==  0) because
  (entry_count == 0 => pending_index == -1)

* (current_index == -1 && entry_count >  0) because
  (current_index == -1 => entry_count == 0)

A similar simplification can be found in:
https://codereview.chromium.org/2762363002/

This CL was done while trying to understand the crashes in:
https://crbug.com/709431


BUG=709431
==========

to

==========
Simplify WebContentsState::GetContentsStateAsByteBuffer

This CL remove some conditions that must never be true:

* (entry_count == 0 && pending_index ==  0) because
  (entry_count == 0 => pending_index == -1)

* (current_index == -1 && entry_count >  0) because
  (current_index == -1 => entry_count == 0)

A similar simplification can be found in:
https://codereview.chromium.org/2762363002/

This CL was done while trying to understand the crashes in:
https://crbug.com/709431


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

[arthursonzogni, 2017-04-11 08:35:45]

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

[commit-bot: I haz the power, 2017-04-11 08:36: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, 2017-04-11 09:48:32]

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

[commit-bot: I haz the power, 2017-04-11 09:48:33]

Dry run: This issue passed the CQ dry run.

[Eugene But (OOO till 7-30), 2017-04-11 14:45:43]

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

[Eugene But (OOO till 7-30), 2017-04-11 14:45:44]

lgtm! Adding Charlie who better knows this code and owns NavigationController.

[Charlie Reis, 2017-04-11 16:22:41]

Thanks for adding me!  That definitely looks like a typo you're fixing in
navigation_controller_impl.h-- thanks.  I'll take a closer look this afternoon
to confirm the change is right.

[Charlie Reis, 2017-04-11 23:31:08]

It certainly looks like your change should be a no-op, and that the old code was
mistaken.  It's like it was written with the assumption that a pending entry in
a new tab would have GetPendingEntryIndex() == 0, but I expect it to be -1 in
that case.

The strange thing is that you're definitely pointing to real crashes that only
seem to be explained if GetPendingEntryIndex() is 0, GetEntryCount() is 0, and
GetPendingEntry() is null.  That shouldn't happen, but apparently it is
happening?

If so, there may also be real cases where GetPendingEntryIndex() is 0,
GetEntryCount() is 0, and GetPendingEntry() is a real entry.  If so, the old
code would have done something meaningful, and now your CL would make us skip
this hypothetical pending entry when creating the byte buffer.  Is that ok?  I'm
not sure how this function is used and what implications that will have.

I'm trying out a CL to add a bunch of these invariant DCHECKs inside
NavigationController to see if we're missing something.  Then again, that will
probably only help if we have test coverage for the unexpected cases.  We'll see
how it does, though:
https://codereview.chromium.org/2815753003/

I'll say LGTM since I can't think of a reason for this to be wrong, but the fact
that there's crashes suggests we might be causing a regression here.  Maybe it's
worth adding some crash keys in a preliminary CL (before landing this) to
confirm whether the pending index and entry count can both be 0 in the wild?

https://codereview.chromium.org/2802213002/diff/40001/chrome/browser/android/...
File chrome/browser/android/tab_state.cc (left):

https://codereview.chromium.org/2802213002/diff/40001/chrome/browser/android/...
chrome/browser/android/tab_state.cc:298: if (entry_count == 0 && pending_index
== 0)
Right.  pending_index should be -1 if entry_count is 0.

https://codereview.chromium.org/2802213002/diff/40001/chrome/browser/android/...
chrome/browser/android/tab_state.cc:306: current_entry = 0;
Yeah, I think this was in response to the entry_count++ line, which shouldn't
have been reachable.

[Charlie Reis, 2017-04-11 23:54:48]

Ah, I might have found it!  My try jobs for
https://codereview.chromium.org/2815753003/ failed in a few places for
PlzNavigate, including GetPendingEntry() in
NavigationControllerTest.StopOnHistoryNavigationToCurrentPage:
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

Can you look into the failures on that CL before landing this?  I'm guessing
they will give us repro steps for the crash, and they may point to real bugs in
PlzNavigate.  Thanks!

[arthursonzogni, 2017-04-26 12:10:36]

The CQ bit was checked by arthursonzogni@chromium.org

[commit-bot: I haz the power, 2017-04-26 12:10:56]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-04-26 12:18:28]

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

[commit-bot: I haz the power, 2017-04-26 12:18:29]

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...)

[Charlie Reis, 2017-04-26 23:50:09]

On 2017/04/26 12:18:29, commit-bot: I haz the power wrote:
> 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...)

I think this still needs a chrome/browser/android owner's approval.

However, I think we also still need to make a judgement call about my question
from before:

> the fact
> that there's crashes suggests we might be causing a regression here.  Maybe
it's
> worth adding some crash keys in a preliminary CL (before landing this) to
> confirm whether the pending index and entry count can both be 0 in the wild?

Even with the recent fix in https://codereview.chromium.org/2810173002/, I don't
think we have an explanation for how the pending index could be 0 when the entry
count isn't.  I think that would fail both DCHECKs you added to
GetPendingEntryIndex().  Also, the transient entry bug you fixed doesn't seem
like it would have explained it.

Maybe we can just hope that a developer will hit the DCHECKs we added and will
file a bug with repro steps, in the interest of prioritizing the crash fix?  I
guess I'd be ok with that.

[arthursonzogni, 2017-05-02 11:57:24]

On 2017/04/26 23:50:09, Charlie Reis wrote:
> On 2017/04/26 12:18:29, commit-bot: I haz the power wrote:
> > 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...)
> 
> I think this still needs a chrome/browser/android owner's approval.
> 
> However, I think we also still need to make a judgement call about my question
> from before:
> 
> > the fact
> > that there's crashes suggests we might be causing a regression here.  Maybe
> it's
> > worth adding some crash keys in a preliminary CL (before landing this) to
> > confirm whether the pending index and entry count can both be 0 in the wild?
> 
> Even with the recent fix in https://codereview.chromium.org/2810173002/, I
don't
> think we have an explanation for how the pending index could be 0 when the
entry
> count isn't.  I think that would fail both DCHECKs you added to
> GetPendingEntryIndex().  Also, the transient entry bug you fixed doesn't seem
> like it would have explained it.
> 
> Maybe we can just hope that a developer will hit the DCHECKs we added and will
> file a bug with repro steps, in the interest of prioritizing the crash fix?  I
> guess I'd be ok with that.

I think the problem doesn't come from the lines:
```
if (entry_count == 0 && pending_index == 0)	
  entry_count++;
```
Maybe the problem is in the loop when GetPendingEntry() is assigned to
navigation[i] when the NavigationControllerImpl was in a bad state.

For instance, my best guess was that |pending_entry_index| was set in
NavigationControllerImpl::GoToIndex without assigning something to
|pending_entry|.
Then in NavigateToPendingEntry(), |delegate_->Stop()| is called. The
NavigationController is in a bad state (GetPendingEntry() == nullptr and
GetPendingEntryIndex() != -1).
If WebContentsState::GetContentsStateAsByteBuffer is called from there, a
nullptr can be assigned into the |navigations| vector. I don't know if this path
is possible however.

Let's wait for a couple of weeks such that we can get some reports.
If there is none, it will probably mean that the recent fix solves the problem.

[Charlie Reis, 2017-05-03 05:32:35]

On 2017/05/02 11:57:24, arthursonzogni OOO until tue wrote:
> On 2017/04/26 23:50:09, Charlie Reis wrote:
> > On 2017/04/26 12:18:29, commit-bot: I haz the power wrote:
> > > 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...)
> > 
> > I think this still needs a chrome/browser/android owner's approval.
> > 
> > However, I think we also still need to make a judgement call about my
question
> > from before:
> > 
> > > the fact
> > > that there's crashes suggests we might be causing a regression here. 
Maybe
> > it's
> > > worth adding some crash keys in a preliminary CL (before landing this) to
> > > confirm whether the pending index and entry count can both be 0 in the
wild?
> > 
> > Even with the recent fix in https://codereview.chromium.org/2810173002/, I
> don't
> > think we have an explanation for how the pending index could be 0 when the
> entry
> > count isn't.  I think that would fail both DCHECKs you added to
> > GetPendingEntryIndex().  Also, the transient entry bug you fixed doesn't
seem
> > like it would have explained it.
> > 
> > Maybe we can just hope that a developer will hit the DCHECKs we added and
will
> > file a bug with repro steps, in the interest of prioritizing the crash fix? 
I
> > guess I'd be ok with that.
> 
> I think the problem doesn't come from the lines:
> ```
> if (entry_count == 0 && pending_index == 0)	
>   entry_count++;
> ```
> Maybe the problem is in the loop when GetPendingEntry() is assigned to
> navigation[i] when the NavigationControllerImpl was in a bad state.
> 
> For instance, my best guess was that |pending_entry_index| was set in
> NavigationControllerImpl::GoToIndex without assigning something to
> |pending_entry|.
> Then in NavigateToPendingEntry(), |delegate_->Stop()| is called. The
> NavigationController is in a bad state (GetPendingEntry() == nullptr and
> GetPendingEntryIndex() != -1).
> If WebContentsState::GetContentsStateAsByteBuffer is called from there, a
> nullptr can be assigned into the |navigations| vector. I don't know if this
path
> is possible however.
> 
> Let's wait for a couple of weeks such that we can get some reports.
> If there is none, it will probably mean that the recent fix solves the
problem.

That explanation makes sense to me, thanks.  I'm ok with landing this now or
later-- up to you.

[arthursonzogni, 2017-05-03 08:06:13]

On 2017/05/03 05:32:35, Charlie Reis wrote:
> On 2017/05/02 11:57:24, arthursonzogni OOO until tue wrote:
> > On 2017/04/26 23:50:09, Charlie Reis wrote:
> > > On 2017/04/26 12:18:29, commit-bot: I haz the power wrote:
> > > > 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...)
> > > 
> > > I think this still needs a chrome/browser/android owner's approval.
> > > 
> > > However, I think we also still need to make a judgement call about my
> question
> > > from before:
> > > 
> > > > the fact
> > > > that there's crashes suggests we might be causing a regression here. 
> Maybe
> > > it's
> > > > worth adding some crash keys in a preliminary CL (before landing this)
to
> > > > confirm whether the pending index and entry count can both be 0 in the
> wild?
> > > 
> > > Even with the recent fix in https://codereview.chromium.org/2810173002/, I
> > don't
> > > think we have an explanation for how the pending index could be 0 when the
> > entry
> > > count isn't.  I think that would fail both DCHECKs you added to
> > > GetPendingEntryIndex().  Also, the transient entry bug you fixed doesn't
> seem
> > > like it would have explained it.
> > > 
> > > Maybe we can just hope that a developer will hit the DCHECKs we added and
> will
> > > file a bug with repro steps, in the interest of prioritizing the crash
fix? 
> I
> > > guess I'd be ok with that.
> > 
> > I think the problem doesn't come from the lines:
> > ```
> > if (entry_count == 0 && pending_index == 0)	
> >   entry_count++;
> > ```
> > Maybe the problem is in the loop when GetPendingEntry() is assigned to
> > navigation[i] when the NavigationControllerImpl was in a bad state.
> > 
> > For instance, my best guess was that |pending_entry_index| was set in
> > NavigationControllerImpl::GoToIndex without assigning something to
> > |pending_entry|.
> > Then in NavigateToPendingEntry(), |delegate_->Stop()| is called. The
> > NavigationController is in a bad state (GetPendingEntry() == nullptr and
> > GetPendingEntryIndex() != -1).
> > If WebContentsState::GetContentsStateAsByteBuffer is called from there, a
> > nullptr can be assigned into the |navigations| vector. I don't know if this
> path
> > is possible however.
> > 
> > Let's wait for a couple of weeks such that we can get some reports.
> > If there is none, it will probably mean that the recent fix solves the
> problem.
> 
> That explanation makes sense to me, thanks.  I'm ok with landing this now or
> later-- up to you.

Thanks!

@dfalcantara: Can you take a look now that Charlie and Eugene reviewed it
please? I need an LGTM from an OWNER of
chrome/browser/android.

[gone, 2017-05-03 16:33:50]

owners lgtm

[Charlie Reis, 2017-05-03 21:49:30]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2017-05-03 21:50:29]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-03 22:52:43]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493848170900630,
"parent_rev": "2f32140a3bf95b2e2da567f53b5b5bdd8ece1508", "commit_rev":
"02084c771e830293de3cc4919d29a83341558035"}

[commit-bot: I haz the power, 2017-05-03 22:52:55]

Description was changed from

==========
Simplify WebContentsState::GetContentsStateAsByteBuffer

This CL remove some conditions that must never be true:

* (entry_count == 0 && pending_index ==  0) because
  (entry_count == 0 => pending_index == -1)

* (current_index == -1 && entry_count >  0) because
  (current_index == -1 => entry_count == 0)

A similar simplification can be found in:
https://codereview.chromium.org/2762363002/

This CL was done while trying to understand the crashes in:
https://crbug.com/709431


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

to

==========
Simplify WebContentsState::GetContentsStateAsByteBuffer

This CL remove some conditions that must never be true:

* (entry_count == 0 && pending_index ==  0) because
  (entry_count == 0 => pending_index == -1)

* (current_index == -1 && entry_count >  0) because
  (current_index == -1 => entry_count == 0)

A similar simplification can be found in:
https://codereview.chromium.org/2762363002/

This CL was done while trying to understand the crashes in:
https://crbug.com/709431


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

Review-Url: https://codereview.chromium.org/2802213002
Cr-Commit-Position: refs/heads/master@{#469178}
Committed:
https://chromium.googlesource.com/chromium/src/+/02084c771e830293de3cc4919d29...
==========

[commit-bot: I haz the power, 2017-05-03 22:53:01]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/02084c771e830293de3cc4919d29...