Reland "Use resource throttle to implement shouldOverrideUrlLoading, core change"

Reland "Use resource throttle to implement shouldOverrideUrlLoading, core change"

Now that https://codereview.chromium.org/1381003004/ has landed we can
reland this patch since we now receive a didFinishLoad callback when
the current navigation finishes even in the case where a new navigation
has reached a provisional state.

This reverts commit e5c3091df04749b232359b6ef22938006af840d5.

TBR=palmer@chromium.org,jam@chromium.org
BUG=325351
Committed: https://crrev.com/bae3ff13e6959b713b872df3c4a734d6a1ea01ad
Cr-Commit-Position: refs/heads/master@{#358719}

[gsennton, 2015-11-04 00:38:22]

Description was changed from

==========
Reland "Use resource throttle to implement shouldOverrideUrlLoading, core
change"

Now that https://codereview.chromium.org/1381003004/ has landed we can
reland this patch since we now receive a didFinishLoad callback when
the current navigation finishes even in the case where a new navigation
has reached a provisional state.

This reverts commit e5c3091df04749b232359b6ef22938006af840d5.

BUG=325351
==========

to

==========
Reland "Use resource throttle to implement shouldOverrideUrlLoading, core
change"

Now that https://codereview.chromium.org/1381003004/ has landed we can
reland this patch since we now receive a didFinishLoad callback when
the current navigation finishes even in the case where a new navigation
has reached a provisional state.

This reverts commit e5c3091df04749b232359b6ef22938006af840d5.

TBR=palmer@chromium.org,jam@chromium.org
BUG=325351
==========

[gsennton, 2015-11-04 00:38:22]

gsennton@chromium.org changed reviewers:
+ jam@chromium.org, mnaganov@chromium.org, palmer@chromium.org,
sgurun@chromium.org

[gsennton, 2015-11-04 00:42:09]

This is a reland of https://codereview.chromium.org/1155713005/

The reland initially failed two tests:
AwContentsClientShouldOverrideUrlLoadingTest.testCalledOn302Redirect
and
AwContentsClientShouldOverrideUrlLoadingTest.testCalledForIframeUnsupportedSchemeNavigations

the first because we were expecting all redirects to not have the userGesture
flag set.
The second because InterceptNavigationThrottle::CheckIfShouldIgnoreNavigation
was not passing the isMainFrame flag when converting from a navigation handle to
navigation params. Is there any reason for only passing true for that flag
rather than passing the actual flag?

I'm adding all the reviewers of the initial CL here, technically only mnaganov@
(for android_webview/ and components/navigation_interception/) and jam@ (for
content/) are needed (and jam@ is TBR'd since I'm removing the same stuff from
content/ as in the original CL).

[mnaganov (inactive), 2015-11-04 01:03:19]

LGTM with comments

https://codereview.chromium.org/1424263003/diff/20001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java
(right):

https://codereview.chromium.org/1424263003/diff/20001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java:1123:
assertEquals(1, mWebServer.getRequestCount(xhrPath));
You should also test that request count is 0 somewhere earlier.

https://codereview.chromium.org/1424263003/diff/20001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java:1127:
private boolean mValue = false;
This value is set on one thread (Java Bridge background thread) and then being
read on another thread. I understand that reading and writing is synchronized
via waitForEvent, but I guess you should mark this field as "volatile" to ensure
that both threads read and write the same memory location, and not CPU caches.

[gsennton, 2015-11-04 01:21:52]

https://codereview.chromium.org/1424263003/diff/20001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java
(right):

https://codereview.chromium.org/1424263003/diff/20001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java:1123:
assertEquals(1, mWebServer.getRequestCount(xhrPath));
On 2015/11/04 01:03:19, mnaganov wrote:
> You should also test that request count is 0 somewhere earlier.

Done.

https://codereview.chromium.org/1424263003/diff/20001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java:1127:
private boolean mValue = false;
On 2015/11/04 01:03:19, mnaganov wrote:
> This value is set on one thread (Java Bridge background thread) and then being
> read on another thread. I understand that reading and writing is synchronized
> via waitForEvent, but I guess you should mark this field as "volatile" to
ensure
> that both threads read and write the same memory location, and not CPU caches.

Done.

[Torne, 2015-11-04 16:21:58]

On 2015/11/04 01:03:19, mnaganov wrote:
> LGTM with comments
> 
>
https://codereview.chromium.org/1424263003/diff/20001/android_webview/javates...
> File
>
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java
> (right):
> 
>
https://codereview.chromium.org/1424263003/diff/20001/android_webview/javates...
>
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java:1123:
> assertEquals(1, mWebServer.getRequestCount(xhrPath));
> You should also test that request count is 0 somewhere earlier.
> 
>
https://codereview.chromium.org/1424263003/diff/20001/android_webview/javates...
>
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java:1127:
> private boolean mValue = false;
> This value is set on one thread (Java Bridge background thread) and then being
> read on another thread. I understand that reading and writing is synchronized
> via waitForEvent, but I guess you should mark this field as "volatile" to
ensure
> that both threads read and write the same memory location, and not CPU caches.

Need to look at this in more detail, but this doesn't sound likely; having to
use volatile in java is VERY uncommon, and probably it's either fine without it,
or is broken even with it. Will check the code in detail in a bit.

[mnaganov (inactive), 2015-11-04 18:04:28]

On 2015/11/04 16:21:58, Torne wrote:
> On 2015/11/04 01:03:19, mnaganov wrote:
> > LGTM with comments
> > 
> >
>
https://codereview.chromium.org/1424263003/diff/20001/android_webview/javates...
> > File
> >
>
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1424263003/diff/20001/android_webview/javates...
> >
>
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java:1123:
> > assertEquals(1, mWebServer.getRequestCount(xhrPath));
> > You should also test that request count is 0 somewhere earlier.
> > 
> >
>
https://codereview.chromium.org/1424263003/diff/20001/android_webview/javates...
> >
>
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java:1127:
> > private boolean mValue = false;
> > This value is set on one thread (Java Bridge background thread) and then
being
> > read on another thread. I understand that reading and writing is
synchronized
> > via waitForEvent, but I guess you should mark this field as "volatile" to
> ensure
> > that both threads read and write the same memory location, and not CPU
caches.
> 
> Need to look at this in more detail, but this doesn't sound likely; having to
> use volatile in java is VERY uncommon, and probably it's either fine without
it,
> or is broken even with it. Will check the code in detail in a bit.

There a 3 pages of results from CodeSearch on volatile in Java, so it's not
uncommon at least in Chromium. But I would greatly appreciate any detailed
explanations :)

[Torne, 2015-11-04 18:10:37]

On 2015/11/04 18:04:28, mnaganov wrote:
> On 2015/11/04 16:21:58, Torne wrote:
> > On 2015/11/04 01:03:19, mnaganov wrote:
> > > LGTM with comments
> > > 
> > >
> >
>
https://codereview.chromium.org/1424263003/diff/20001/android_webview/javates...
> > > File
> > >
> >
>
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1424263003/diff/20001/android_webview/javates...
> > >
> >
>
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java:1123:
> > > assertEquals(1, mWebServer.getRequestCount(xhrPath));
> > > You should also test that request count is 0 somewhere earlier.
> > > 
> > >
> >
>
https://codereview.chromium.org/1424263003/diff/20001/android_webview/javates...
> > >
> >
>
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java:1127:
> > > private boolean mValue = false;
> > > This value is set on one thread (Java Bridge background thread) and then
> being
> > > read on another thread. I understand that reading and writing is
> synchronized
> > > via waitForEvent, but I guess you should mark this field as "volatile" to
> > ensure
> > > that both threads read and write the same memory location, and not CPU
> caches.
> > 
> > Need to look at this in more detail, but this doesn't sound likely; having
to
> > use volatile in java is VERY uncommon, and probably it's either fine without
> it,
> > or is broken even with it. Will check the code in detail in a bit.
> 
> There a 3 pages of results from CodeSearch on volatile in Java, so it's not
> uncommon at least in Chromium. But I would greatly appreciate any detailed
> explanations :)

At a quick glance it looks like most of those are wrong. I've not had time to
look at this in more detail yet; will try to take a look before I go home if I
can, otherwise tomorrow morning.

[sgurun-gerrit only, 2015-11-04 22:43:59]

On 2015/11/04 18:10:37, Torne wrote:
> On 2015/11/04 18:04:28, mnaganov wrote:
> > On 2015/11/04 16:21:58, Torne wrote:
> > > On 2015/11/04 01:03:19, mnaganov wrote:
> > > > LGTM with comments
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1424263003/diff/20001/android_webview/javates...
> > > > File
> > > >
> > >
> >
>
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1424263003/diff/20001/android_webview/javates...
> > > >
> > >
> >
>
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java:1123:
> > > > assertEquals(1, mWebServer.getRequestCount(xhrPath));
> > > > You should also test that request count is 0 somewhere earlier.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1424263003/diff/20001/android_webview/javates...
> > > >
> > >
> >
>
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java:1127:
> > > > private boolean mValue = false;
> > > > This value is set on one thread (Java Bridge background thread) and then
> > being
> > > > read on another thread. I understand that reading and writing is
> > synchronized
> > > > via waitForEvent, but I guess you should mark this field as "volatile"
to
> > > ensure
> > > > that both threads read and write the same memory location, and not CPU
> > caches.
> > > 
> > > Need to look at this in more detail, but this doesn't sound likely; having
> to
> > > use volatile in java is VERY uncommon, and probably it's either fine
without
> > it,
> > > or is broken even with it. Will check the code in detail in a bit.
> > 
> > There a 3 pages of results from CodeSearch on volatile in Java, so it's not
> > uncommon at least in Chromium. But I would greatly appreciate any detailed
> > explanations :)
> 
> At a quick glance it looks like most of those are wrong. I've not had time to
> look at this in more detail yet; will try to take a look before I go home if I
> can, otherwise tomorrow morning.

What I remember from Android SMP primer (and I looked briefly again) is that
volatile was a very weak guarantee in Java 1.4 but then became a lot stronger in
synchronization guarantees in Java 1.5.

I would think in this case volatile is fine, but I would use synchronized or
something from concurrency class for a cleaner code.

[mnaganov (inactive), 2015-11-05 00:45:33]

On 2015/11/04 22:43:59, sgurun wrote:
> On 2015/11/04 18:10:37, Torne wrote:
> > On 2015/11/04 18:04:28, mnaganov wrote:
> > > On 2015/11/04 16:21:58, Torne wrote:
> > > > On 2015/11/04 01:03:19, mnaganov wrote:
> > > > > LGTM with comments
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1424263003/diff/20001/android_webview/javates...
> > > > > File
> > > > >
> > > >
> > >
> >
>
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java
> > > > > (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1424263003/diff/20001/android_webview/javates...
> > > > >
> > > >
> > >
> >
>
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java:1123:
> > > > > assertEquals(1, mWebServer.getRequestCount(xhrPath));
> > > > > You should also test that request count is 0 somewhere earlier.
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1424263003/diff/20001/android_webview/javates...
> > > > >
> > > >
> > >
> >
>
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java:1127:
> > > > > private boolean mValue = false;
> > > > > This value is set on one thread (Java Bridge background thread) and
then
> > > being
> > > > > read on another thread. I understand that reading and writing is
> > > synchronized
> > > > > via waitForEvent, but I guess you should mark this field as "volatile"
> to
> > > > ensure
> > > > > that both threads read and write the same memory location, and not CPU
> > > caches.
> > > > 
> > > > Need to look at this in more detail, but this doesn't sound likely;
having
> > to
> > > > use volatile in java is VERY uncommon, and probably it's either fine
> without
> > > it,
> > > > or is broken even with it. Will check the code in detail in a bit.
> > > 
> > > There a 3 pages of results from CodeSearch on volatile in Java, so it's
not
> > > uncommon at least in Chromium. But I would greatly appreciate any detailed
> > > explanations :)
> > 
> > At a quick glance it looks like most of those are wrong. I've not had time
to
> > look at this in more detail yet; will try to take a look before I go home if
I
> > can, otherwise tomorrow morning.
> 
> What I remember from Android SMP primer (and I looked briefly again) is that
> volatile was a very weak guarantee in Java 1.4 but then became a lot stronger
in
> synchronization guarantees in Java 1.5.
> 
> I would think in this case volatile is fine, but I would use synchronized or
> something from concurrency class for a cleaner code.

OK, I've read through this article (thanks, Selim!):
http://developer.android.com/training/articles/smp.html,
and I think I can see what torne@ meant.

So, the code in BooleanValueJavascriptObserver.setValue sets the flag, and then
calls
JavascriptEventObserver.notifyJava which does synchronization. But,
according to the article, "you must not conclude that code inside a synchronized
block
happens after the stuff above it or before the stuff below it in a function".
The same applies to testing the flag inside the test method.

"volatile" ensures that "If thread 1 writes to a volatile field, and thread 2
subsequently reads from that same field, then thread 2 is guaranteed to see that
write
and all writes previously made by thread 1". But since there is no guarantee
that
these reads and writes happen strictly after the synchronization code, this
"volatile" contract does not help us.

Thus, what I would recommend doing here: either properly synchronize access to
mValue
in BooleanValueJavascriptObserver, or instead of using a Java flag, set a
"window"
object property in JavaScript code, and then read it using "evaluateJavaScript"
in
the test method. I guess, the single-threaded model of JavaScript and all
the synchronizations used by "evaluateJavaScript" must guarantee that we will
read
the updated value.

Oh, man, I wish this stuff would be simpler :)

[gsennton, 2015-11-05 01:35:12]

Ok, added the 'synchronized' keyword to the getter and the setter so that they
cannot be interleaved :)

[Torne, 2015-11-05 14:30:22]

On 2015/11/05 00:45:33, mnaganov wrote:
> OK, I've read through this article (thanks, Selim!):
> http://developer.android.com/training/articles/smp.html,
> and I think I can see what torne@ meant.

Unfortunately, I think you've misread the article. What I actually meant was
that the correct thing to do here is probably to *neither* use synchronised nor
volatile, and on inspecting the code closer, this appears to be correct.

> So, the code in BooleanValueJavascriptObserver.setValue sets the flag, and
then
> calls
> JavascriptEventObserver.notifyJava which does synchronization. But,
> according to the article, "you must not conclude that code inside a
synchronized
> block
> happens after the stuff above it or before the stuff below it in a function".
> The same applies to testing the flag inside the test method.

You've misinterpreted this. What it means is that it's possible for stuff before
the synchronized block to happen *during* it, instead of beforehand. It *is*
guaranteed that once the synchronized block ends, everything up to that point
(both before and during the block) will have happened, *from the perspective of
another thread which synchronizes on the same object*. The advice here is about
avoiding potential deadlock scenarios and other problems, not creating safety.

It would probably be most clear with a sequence diagram but those are awkward to
draw over the internet, so I'll try a textual description:

The thread which calls setValue (let's call it WRITER) sets the flag, then calls
notifyJava, which acquires a lock and then notifies a condition variable
associated with that lock before releasing it.
The thread which wants to know the value (READER) first calls waitForEvent,
which acquires the same lock and then waits on the same condition variable
(repeatedly, until mHappened becomes true). As soon as READER, holding the lock,
observes mHappened being set to true, it is now *guaranteed* that all memory
accesses performed by WRITER up to the point where it released the lock are now
visible to READER - *even* if WRITER's memory accesses were before the
synchronized block.

The reason this works is because the process of entering a synchronized block is
an "acquire barrier", which is a memory barrier that prevents all *later* memory
accesses from being reordered before the barrier, and the process of leaving a
synchronized block is a "release barrier", which prevents all *earlier* memory
accesses from being reordered after the barrier.

> "volatile" ensures that "If thread 1 writes to a volatile field, and thread 2
> subsequently reads from that same field, then thread 2 is guaranteed to see
that
> write
> and all writes previously made by thread 1". But since there is no guarantee
> that
> these reads and writes happen strictly after the synchronization code, this
> "volatile" contract does not help us.

volatile is generally only relevant in java when you are trying to write
lock-free code, which is almost always a terrible decision, and I would just
ignore it completely unless you are trying to convert a piece of lock-dependent
code to lock-free for performance reasons (and even then, make sure to bring an
old priest and a young priest).

volatile memory accesses, in Java 1.5 onward, do act as barriers, but correctly
composing a synchronisation algorithm using only atomic accesses and barriers is
*hard*, and it's really best not to try if you don't have to - it's much easier
and simpler to use locks and condition variables if performance is not
ultra-critical.

[mnaganov (inactive), 2015-11-05 16:24:24]

On 2015/11/05 14:30:22, Torne wrote:
> On 2015/11/05 00:45:33, mnaganov wrote:
> > OK, I've read through this article (thanks, Selim!):
> > http://developer.android.com/training/articles/smp.html,
> > and I think I can see what torne@ meant.
> 
> Unfortunately, I think you've misread the article. What I actually meant was
> that the correct thing to do here is probably to *neither* use synchronised
nor
> volatile, and on inspecting the code closer, this appears to be correct.
> 
> > So, the code in BooleanValueJavascriptObserver.setValue sets the flag, and
> then
> > calls
> > JavascriptEventObserver.notifyJava which does synchronization. But,
> > according to the article, "you must not conclude that code inside a
> synchronized
> > block
> > happens after the stuff above it or before the stuff below it in a
function".
> > The same applies to testing the flag inside the test method.
> 
> You've misinterpreted this. What it means is that it's possible for stuff
before
> the synchronized block to happen *during* it, instead of beforehand. It *is*
> guaranteed that once the synchronized block ends, everything up to that point
> (both before and during the block) will have happened, *from the perspective
of
> another thread which synchronizes on the same object*. The advice here is
about
> avoiding potential deadlock scenarios and other problems, not creating safety.
> 
> It would probably be most clear with a sequence diagram but those are awkward
to
> draw over the internet, so I'll try a textual description:
> 
> The thread which calls setValue (let's call it WRITER) sets the flag, then
calls
> notifyJava, which acquires a lock and then notifies a condition variable
> associated with that lock before releasing it.
> The thread which wants to know the value (READER) first calls waitForEvent,
> which acquires the same lock and then waits on the same condition variable
> (repeatedly, until mHappened becomes true). As soon as READER, holding the
lock,
> observes mHappened being set to true, it is now *guaranteed* that all memory
> accesses performed by WRITER up to the point where it released the lock are
now
> visible to READER - *even* if WRITER's memory accesses were before the
> synchronized block.
> 
> The reason this works is because the process of entering a synchronized block
is
> an "acquire barrier", which is a memory barrier that prevents all *later*
memory
> accesses from being reordered before the barrier, and the process of leaving a
> synchronized block is a "release barrier", which prevents all *earlier* memory
> accesses from being reordered after the barrier.
>

I see. Yes, the bulk of the article tells exactly this, it's just one sentence
that
was formulated in a more vague fashion, and I extrapolated it even further,
sorry.

OK, it's good to know that at least we can have some sane guarantees on
what happens w.r.t. synchronization.

> > "volatile" ensures that "If thread 1 writes to a volatile field, and thread
2
> > subsequently reads from that same field, then thread 2 is guaranteed to see
> that
> > write
> > and all writes previously made by thread 1". But since there is no guarantee
> > that
> > these reads and writes happen strictly after the synchronization code, this
> > "volatile" contract does not help us.
> 
> volatile is generally only relevant in java when you are trying to write
> lock-free code, which is almost always a terrible decision, and I would just
> ignore it completely unless you are trying to convert a piece of
lock-dependent
> code to lock-free for performance reasons (and even then, make sure to bring
an
> old priest and a young priest).
> 
> volatile memory accesses, in Java 1.5 onward, do act as barriers, but
correctly
> composing a synchronisation algorithm using only atomic accesses and barriers
is
> *hard*, and it's really best not to try if you don't have to - it's much
easier
> and simpler to use locks and condition variables if performance is not
> ultra-critical.

[gsennton, 2015-11-05 18:53:30]

So, it should be fine to land this now? :)

[gsennton, 2015-11-06 01:54:26]

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

[commit-bot: I haz the power, 2015-11-06 01:55:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1424263003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1424263003/80001

[commit-bot: I haz the power, 2015-11-06 04:27:18]

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

[commit-bot: I haz the power, 2015-11-06 04:27:19]

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

[gsennton, 2015-11-09 22:47:33]

The CQ bit was checked by gsennton@chromium.org

[gsennton, 2015-11-09 22:47:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
mnaganov@chromium.org
Link to the patchset: https://codereview.chromium.org/1424263003/#ps80001
(title: "Revert unnecessary change for making XHR test thread-safe (it was safe
before the change).")

[commit-bot: I haz the power, 2015-11-09 22:48:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1424263003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1424263003/80001

[commit-bot: I haz the power, 2015-11-10 00:49:52]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-10 00:50:52]

Patchset 5 (id:??) landed as
https://crrev.com/bae3ff13e6959b713b872df3c4a734d6a1ea01ad
Cr-Commit-Position: refs/heads/master@{#358719}