Properly throw java exceptions from shouldOverrideUrlLoading

Properly throw java exceptions from shouldOverrideUrlLoading

[WebView]
Since all our java methods called through the jni-interface are
annotated with @CalledByNative (rather than @CalledByNativeUnchecked)
any java crash in those methods will cause a native crash since we call
jni_android::CheckException (which aborts in native code) when returning
from those methods.

In this CL we annotate the jni-call for shouldOverrideUrlLoading with
@CalledByNativeUnchecked to avoid crashing directly after the jni-call.

We also add an Abort function for the android specific message loop and
message pump to ensure no new tasks will be run before we return to the
Java handler (at which point the original Java exception will be
rethrown and thus correctly reported).

BUG=592556
Committed: https://crrev.com/ebe2e203bd421bc1a5e4c28a48f7d54d407c6fac
Cr-Commit-Position: refs/heads/master@{#412956}

[gsennton, 2016-07-20 17:05:15]

gsennton@chromium.org changed reviewers:
+ thakis@chromium.org, torne@chromium.org

[gsennton, 2016-07-20 17:05:16]

torne@chromium.org:
android_webview/
base/*android*


thakis@chromium.org:
base/message_loop/message_loop.*
(I'm only doing android-specific changes here though)

[gsennton, 2016-07-20 18:09:46]

On 2016/07/20 17:05:16, gsennton wrote:
> mailto:torne@chromium.org:
> android_webview/
> base/*android*
> 
> 
> mailto:thakis@chromium.org:
> base/message_loop/message_loop.*
> (I'm only doing android-specific changes here though)

A note here:
I've checked the callstack for shouldOverrideUrlLoading [1] to ensure we don't
call any jni methods between the java-side callback failing and us returning up
to SystemMessageHandler, there are only a couple of places I'm a bit unsure
about:
TaskAnnotator::RunTask runs a couple of commands after running its task, but I
believe these are just minor tasks:

stopwatch.Stop();
tracked_objects::ThreadData::TallyRunOnNamedThreadIfTracking(
      pending_task, stopwatch);


Then we have MessageLoop::RunTask which calls DidProcessTask for each of its
TaskObservers (and from that code we don't really have any control over what
these TaskObservers are doing, though they should run code that executes very
quickly since it's being run after each native task).


All other parts of the stack essentially return immediately after running the
current task (though this could change in the future... :/).





[1] example callstack:
com.android.shouldoverrideurlloadingcrash.ShouldOverrideUrlLoadingCrashMainActivity$1.shouldOverrideUrlLoading(ShouldOverrideUrlLoadingCrashMainActivity.java:20)
   at
android.webkit.WebViewClient.shouldOverrideUrlLoading(WebViewClient.java:73)
   at
com.android.webview.chromium.WebViewContentsClientAdapter.shouldOverrideUrlLoading(WebViewContentsClientAdapter.java:339)
   at
org.chromium.android_webview.AwContentsClient.shouldIgnoreNavigation(AwContentsClient.java:168)
   at
org.chromium.android_webview.AwContentsClientBridge.shouldOverrideUrlLoading(AwContentsClientBridge.java:257)
   at org.chromium.base.SystemMessageHandler.nativeDoRunLoopOnce(Native Method)
   at
org.chromium.base.SystemMessageHandler.handleMessage(SystemMessageHandler.java:39)
   at android.os.Handler.dispatchMessage(Handler.java:102)
   at android.os.Looper.loop(Looper.java:154)
   at android.app.ActivityThread.main(ActivityThread.java:6077)
   at java.lang.reflect.Method.invoke(Native Method)
   at
com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:865)
   at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:755)

[gsennton, 2016-07-20 21:01:53]

gsennton@chromium.org changed reviewers:
+ danakj@chromium.org
- thakis@chromium.org

[gsennton, 2016-07-20 21:01:54]

Replaced thakis@ with danakj@ for base/message_loop/ since thakis@ is OOO.

[danakj, 2016-07-20 21:37:09]

Can you provide some unit tests for this?

https://codereview.chromium.org/2169553002/diff/1/base/message_loop/message_l...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/2169553002/diff/1/base/message_loop/message_l...
base/message_loop/message_loop.cc:627: if
(DeferOrRunPendingTask(std::move(pending_task))) {
?

https://codereview.chromium.org/2169553002/diff/1/base/message_loop/message_p...
File base/message_loop/message_pump_android.cc (right):

https://codereview.chromium.org/2169553002/diff/1/base/message_loop/message_p...
base/message_loop/message_pump_android.cc:98: delegate->DoIdleWork();
Why not after this too?

https://codereview.chromium.org/2169553002/diff/1/base/message_loop/message_p...
base/message_loop/message_pump_android.cc:103:
MessagePumpForUI::MessagePumpForUI() : run_loop_(NULL), should_abort_(false) {}
nit: nullptr while you're here?

https://codereview.chromium.org/2169553002/diff/1/base/message_loop/message_p...
base/message_loop/message_pump_android.cc:135: // We can't call back into java
if there is a pending jni exception
I'm not sure what this comment means to explain, can you expand it? It also
doesnt end with a period, so maybe you left something off?

[Torne, 2016-07-21 13:38:26]

https://codereview.chromium.org/2169553002/diff/1/android_webview/native/aw_c...
File android_webview/native/aw_contents_client_bridge.cc (right):

https://codereview.chromium.org/2169553002/diff/1/android_webview/native/aw_c...
android_webview/native/aw_contents_client_bridge.cc:366:
static_cast<base::MessageLoopForUI*>(base::MessageLoop::current())->Abort();
This stops later stages of DoRunLoopOnce from executing, but it's not obvious to
me that it prevents any other native chromium tasks from running - am I missing
something? What if there's already another queued task after this current one
for immediate execution?

https://codereview.chromium.org/2169553002/diff/1/base/message_loop/message_p...
File base/message_loop/message_pump_android.cc (right):

https://codereview.chromium.org/2169553002/diff/1/base/message_loop/message_p...
base/message_loop/message_pump_android.cc:98: delegate->DoIdleWork();
On 2016/07/20 21:37:08, danakj wrote:
> Why not after this too?

It's about to return to the JVM anyway, at the end of the function.

https://codereview.chromium.org/2169553002/diff/1/base/message_loop/message_p...
base/message_loop/message_pump_android.cc:135: // We can't call back into java
if there is a pending jni exception
On 2016/07/20 21:37:09, danakj wrote:
> I'm not sure what this comment means to explain, can you expand it? It also
> doesnt end with a period, so maybe you left something off?

We need to avoid calling any Java functions (like this one) if we have a pending
Java exception already, because the VM will crash. Thus, we need to avoid the
Quit() codepath when trying to abort, because it calls back into Java.

I think this comment is incomplete/misplaced, it's not obvious what it means
here.

[gsennton, 2016-07-26 14:42:39]

I'm not currently sure how we could unit test this, I guess what we are looking
for is a test that ensures that MessagePumpAndroid doesn't call any more methods
after we call MessageLoop.Abort()?
How do we usually add unit tests for something platform specific like this? Do
we have a standard way of dealing with each iteration of the chromium message
pump/loop being initiated from Java/Android?

To me it makes more sense to have something like an instrumentation test that
ensures that if we throw an exception in
WebViewClient.shouldOverrideUrlLoading(..) then that exception is thrown when
nativeDoRunLoopOnce returns (i.e. that we can then catch that exception and that
we don't crash in some other weird way). But I think such a test would only
cover the case where the crashing method is being called from DoWork rather than
from DoDelayedWork or DoIdleWork, and the test would live in a higher layer than
base/, like android_webview/   :/

https://codereview.chromium.org/2169553002/diff/1/android_webview/native/aw_c...
File android_webview/native/aw_contents_client_bridge.cc (right):

https://codereview.chromium.org/2169553002/diff/1/android_webview/native/aw_c...
android_webview/native/aw_contents_client_bridge.cc:366:
static_cast<base::MessageLoopForUI*>(base::MessageLoop::current())->Abort();
On 2016/07/21 13:38:25, Torne wrote:
> This stops later stages of DoRunLoopOnce from executing, but it's not obvious
to
> me that it prevents any other native chromium tasks from running - am I
missing
> something? What if there's already another queued task after this current one
> for immediate execution?

The current implementation of this (MessageLoop::DoWork() -
https://cs.chromium.org/chromium/src/base/message_loop/message_loop.cc?cl=GRO...)
runs one task and then returns immediately.

danakj@, is MessageLoop.DoWork / DoDelayedWork guaranteed to only run one task?
I.e. is this something we want to uphold in the future as well, and if so, could
we add a comment to the MessagePump::Delegate::DoWork/DoDelayedWork/DoIdleWork
declarations to show that these functions should indeed only run one task each?

https://codereview.chromium.org/2169553002/diff/1/base/message_loop/message_l...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/2169553002/diff/1/base/message_loop/message_l...
base/message_loop/message_loop.cc:627: if
(DeferOrRunPendingTask(std::move(pending_task))) {
On 2016/07/20 21:37:08, danakj wrote:
> ?

Ooops, was changing this code when testing/logging, though I have to admit I
kinda hate statements like

if (blah)
   do something on a single line;



:)

https://codereview.chromium.org/2169553002/diff/1/base/message_loop/message_p...
File base/message_loop/message_pump_android.cc (right):

https://codereview.chromium.org/2169553002/diff/1/base/message_loop/message_p...
base/message_loop/message_pump_android.cc:98: delegate->DoIdleWork();
On 2016/07/21 13:38:25, Torne wrote:
> On 2016/07/20 21:37:08, danakj wrote:
> > Why not after this too?
> 
> It's about to return to the JVM anyway, at the end of the function.

Added a comment about this here.

https://codereview.chromium.org/2169553002/diff/1/base/message_loop/message_p...
base/message_loop/message_pump_android.cc:103:
MessagePumpForUI::MessagePumpForUI() : run_loop_(NULL), should_abort_(false) {}
On 2016/07/20 21:37:08, danakj wrote:
> nit: nullptr while you're here?

Done.

https://codereview.chromium.org/2169553002/diff/1/base/message_loop/message_p...
base/message_loop/message_pump_android.cc:135: // We can't call back into java
if there is a pending jni exception
On 2016/07/21 13:38:25, Torne wrote:
> On 2016/07/20 21:37:09, danakj wrote:
> > I'm not sure what this comment means to explain, can you expand it? It also
> > doesnt end with a period, so maybe you left something off?
> 
> We need to avoid calling any Java functions (like this one) if we have a
pending
> Java exception already, because the VM will crash. Thus, we need to avoid the
> Quit() codepath when trying to abort, because it calls back into Java.
> 
> I think this comment is incomplete/misplaced, it's not obvious what it means
> here.

Removed this comment since it doesn't really make any sense here.

[gsennton, 2016-07-27 19:19:45]

I'm working on an instrumentation test for this but I'm still not sure how we
can test this in a unit test closer to the source.

We want to ensure that after calling MessageLoop::Abort() we won't call into
java again (so e.g. we don't want to process any new tasks, and we don't want to
trigger message_pump_android.Quit() since that function calls into java).


I think we can test that we don't process any more tasks by creating our own
MessagePumpAndroid::Delegate and have that count the number of times
DoWork/DoDelayedWork/DoIdleWork are called after we post some tasks to the
message loop. This delegate would have to be passed through some of the
initialization code for MessageLoop/MessagePumpAndroid (so we would need some
test-specific initialization code for these classes). I don't know whether we
can ensure that we don't call any other unwanted functions than those Delegate
functions after calling Abort (e.g. functions in MessagePumpAndroid).

danakj@, any thoughts?

[danakj, 2016-07-28 23:21:37]

On 2016/07/27 19:19:45, gsennton wrote:
> I'm working on an instrumentation test for this but I'm still not sure how we
> can test this in a unit test closer to the source.
> 
> We want to ensure that after calling MessageLoop::Abort() we won't call into
> java again (so e.g. we don't want to process any new tasks, and we don't want
to
> trigger message_pump_android.Quit() since that function calls into java).
> 
> 
> I think we can test that we don't process any more tasks by creating our own
> MessagePumpAndroid::Delegate and have that count the number of times
> DoWork/DoDelayedWork/DoIdleWork are called after we post some tasks to the
> message loop. This delegate would have to be passed through some of the
> initialization code for MessageLoop/MessagePumpAndroid (so we would need some
> test-specific initialization code for these classes). I don't know whether we
> can ensure that we don't call any other unwanted functions than those Delegate
> functions after calling Abort (e.g. functions in MessagePumpAndroid).
> 
> danakj@, any thoughts?

I was expecting something like "post 2 tasks, throw an exception in one, verify
2nd one doesn't run and that the java exception is seen correctly"

[danakj, 2016-07-28 23:22:10]

On 2016/07/28 23:21:37, danakj wrote:
> On 2016/07/27 19:19:45, gsennton wrote:
> > I'm working on an instrumentation test for this but I'm still not sure how
we
> > can test this in a unit test closer to the source.
> > 
> > We want to ensure that after calling MessageLoop::Abort() we won't call into
> > java again (so e.g. we don't want to process any new tasks, and we don't
want
> to
> > trigger message_pump_android.Quit() since that function calls into java).
> > 
> > 
> > I think we can test that we don't process any more tasks by creating our own
> > MessagePumpAndroid::Delegate and have that count the number of times
> > DoWork/DoDelayedWork/DoIdleWork are called after we post some tasks to the
> > message loop. This delegate would have to be passed through some of the
> > initialization code for MessageLoop/MessagePumpAndroid (so we would need
some
> > test-specific initialization code for these classes). I don't know whether
we
> > can ensure that we don't call any other unwanted functions than those
Delegate
> > functions after calling Abort (e.g. functions in MessagePumpAndroid).
> > 
> > danakj@, any thoughts?
> 
> I was expecting something like "post 2 tasks, throw an exception in one,
verify
> 2nd one doesn't run and that the java exception is seen correctly"

Specifically, instead of poking at DoWork/DoDelayedWork/DoIdleWork internals,
just posting a task that changes some state if it runs and check that it doesnt?

[gsennton, 2016-07-29 14:35:06]

Ok, so I've fiddled a bit with a unit test (posting a couple of tasks to the
message loop, one calling MessagePump::Abort and the next one just crashing -
asserting false), and here are some notes:

1. There is a significant difference between the testing environment and the
real-world case. In the real-world case we reach a Java exception that will
cause the current process to die. In a unit test we can't throw such an
exception because killing the current process (where the test is being run) is
not an option.
2. If we just call MessagePump::Abort in our unit test we won't stop the
execution of the the MessagePump/MessageLoop - in the current CL we are assuming
that when we return from native after calling MessagePump::Abort we won't return
to native again (since there is a pending Java exception that will cause the
current process to crash). This means that if we create a unit test where we
post a couple of tasks to our message loop, and the first task calls
MessagePump::Abort and the second one does something illegal (like
ASSERT_TRUE(false)), we will run the second task in the next iteration of
nativeDoRunLoopOnce (which wouldn't happen in the real-world case since
nativeDoRunLoopOnce won't be run again with a pending Java exception) and thus
crash.

Another annoying part of this is that in the unit-test case we would want to
properly shut down the MessageLoop we are using (which for Android means calling
into Java e.g. to remove pending messages). In the real world we don't want to
run these shut-down functions with a pending Java exception (since then calling
into Java causes a crash), i.e. when MessagePump::Abort() has been called. So,
if we decide to add some DCHECKs in the future to ensure these functions are
never called when the current MessageLoop has been aborted those DCHECKS would
fail during the unit test (this is not really a problem but it just annoys me
that we would be using call paths in the unit test that should not be used in
the real world).



So, what we want to do with a test is: after calling MessagePump::Abort from
nativeDoRunLoopOnce, we want to ensure that we don't run any other task in the
same iteration of nativeDoRunLoopOnce. But, posting two tasks to a message loop
and checking that only the first is run would instead ensure that we don't run
nativeDoRunLoopOnce several times (which is not something we need to worry about
in reality).

Am I making any sense? Does it sound like fiddling around with a unit test using
the different Do*Work functions would be valuable here or should we just add an
instrumentation test under android_webview/ instead? (IMO such a test would be
much more valuable since it would actually test the real-world scenario, but I
guess covering code in base/ with a test in a higher layer is not something we
should aim at..)

[Torne, 2016-08-02 13:08:07]

CL as implemented LGTM. Are you planning to add unit tests for the base-level
functionality? I'm not sure whether it's worth it either. We should definitely
integration-test this in WebView but you're already doing that in another CL..

[gsennton, 2016-08-03 11:56:52]

On 2016/08/02 13:08:07, Torne wrote:
> CL as implemented LGTM. Are you planning to add unit tests for the base-level
> functionality? I'm not sure whether it's worth it either. We should definitely
> integration-test this in WebView but you're already doing that in another CL..


Yeah, I've been fiddling with a unit-test for the last couple of days and been
running into problems with setting up a unit test with the production version of
the Clank message loop. I think I know what I've been doing wrong now (wuhuuu
;)).

First, let's take a step back and look at what we would actually test with a
base/ unit-test:
The unit-test would post a task that would throw a Java exception and that
exception would have to be caught before it causes the current thread to crash
(i.e. in SystemMessageHandler). This action in itself would ensure that we can
throw a java exception in native (entered through nativeDoRunLoopOnce) and have
it be re-thrown when returning from native (i.e. that nothing in
MessageLoop/MessagePump caused a different exception when unfolding the native
stack).

I've had a look at whether we could post a couple of tasks for testing - one to
cause a Java exception and call Abort(), and one task that is not allowed to run
(it just asserts false) to ensure that we don't run any tasks after Aborting.
The problem with this is that for the test to be useful we would have to time
the tasks so that the exception-throwing task is run in DoWork, and then the
failing task is run in DoDelayedWork in the same iteration of
nativeDoRunLoopOnce (since in reality the java-exception will cause us to end
the message loop after returning from nativeDoRunLoopOnce so having a task
posted to run in a later iteration of nativeDoRunLoopOnce isn't realistic). It
looks like calling the sequence

PostDelayedTask(FailingTask, 1ms)
PostTask(JavaExceptionThrowingTask)

does indeed cause the tasks to be scheduled in the same iteration of
nativeDoRunLoopOnce, but there is no guarantee this will work and adding this to
unit test could just cause ALL the flakiness... 


So the unit test will just ensure that a Java-exception thrown from a task run
inside nativeDoRunLoopOnce will propagate up to SystemMessageHandler (and by
having one test for posting a task directly and one for posting a delayed task
we could test both DoWork and DoDelayedWork code-paths - not sure if we ever use
DoDelayedWork in production though).

The unit test will add some test-specific code both to JavaHandlerThread and
SystemMessageHandler (or I could create my own version of JavaHandlerThread).

[gsennton, 2016-08-03 14:51:56]

Alright, I've added a draft unit test now, it starts the message loop and posts
a task that
1. throws a java exception
2. calls MessageLoop::Abort()

in SystemMessageHandler we catch that exception and notify a WaitableEvent on
the native side which the test is waiting for before completing. The point of
the test is to ensure we don't call into Java after throwing the java exception.

Haven't had time to compile Chrome/WebView with this patch yet as I'm going OOO
in about 2 minutes ;)

I think we could create a test version of JavaHandlerThread if we don't want to
pollute that class with this extra test-stuff.

ptal and let me know if this looks sane.

[gsennton, 2016-08-09 08:59:21]

On 2016/08/03 14:51:56, gsennton_OOO_back_8.15 wrote:
> Alright, I've added a draft unit test now, it starts the message loop and
posts
> a task that
> 1. throws a java exception
> 2. calls MessageLoop::Abort()
> 
> in SystemMessageHandler we catch that exception and notify a WaitableEvent on
> the native side which the test is waiting for before completing. The point of
> the test is to ensure we don't call into Java after throwing the java
exception.
> 
> Haven't had time to compile Chrome/WebView with this patch yet as I'm going
OOO
> in about 2 minutes ;)
> 
> I think we could create a test version of JavaHandlerThread if we don't want
to
> pollute that class with this extra test-stuff.
> 
> ptal and let me know if this looks sane.

Another question: do we have any guidelines regarding whether to put methods
only used for testing within a production-use class or whether to create a new
class just for testing? (e.g. in the case of JavaHandlerThread using methods
like StartForTesting and StopThreadForTesting, should we create a
JavaHandlerThreadForTesting class instead which we would only use during
testing?)

[Torne, 2016-08-09 11:16:36]

On 2016/08/09 08:59:21, gsennton_OOO_back_8.15 wrote:
> On 2016/08/03 14:51:56, gsennton_OOO_back_8.15 wrote:
> > Alright, I've added a draft unit test now, it starts the message loop and
> posts
> > a task that
> > 1. throws a java exception
> > 2. calls MessageLoop::Abort()
> > 
> > in SystemMessageHandler we catch that exception and notify a WaitableEvent
on
> > the native side which the test is waiting for before completing. The point
of
> > the test is to ensure we don't call into Java after throwing the java
> exception.
> > 
> > Haven't had time to compile Chrome/WebView with this patch yet as I'm going
> OOO
> > in about 2 minutes ;)
> > 
> > I think we could create a test version of JavaHandlerThread if we don't want
> to
> > pollute that class with this extra test-stuff.
> > 
> > ptal and let me know if this looks sane.
> 
> Another question: do we have any guidelines regarding whether to put methods
> only used for testing within a production-use class or whether to create a new
> class just for testing? (e.g. in the case of JavaHandlerThread using methods
> like StartForTesting and StopThreadForTesting, should we create a
> JavaHandlerThreadForTesting class instead which we would only use during
> testing?)

I think here we should probably create a separate class, unless doing so proves
to be really difficult/fiddly.

[gsennton, 2016-08-14 11:03:11]

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

[commit-bot: I haz the power, 2016-08-14 11:03:17]

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

[gsennton, 2016-08-14 11:10:44]

Ok, so I haven't created a separate class instead of JavaHandlerThread yet
(since I think that class would need to be a friend class of ScopedAllowWait
which requires approval from jam@ or brettw@). We should probably indeed create
our own class (though maybe it's better to get this CL landed asap and duplicate
JavaHandlerThread as a follow-up?).

The only other concern I have right now is having added a try-catch statement to
the production code in SystemMessageHandler just for the sake of testing (this
seems super ugly).

[commit-bot: I haz the power, 2016-08-14 12:18:49]

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

[commit-bot: I haz the power, 2016-08-14 12:18:50]

Dry run: This issue passed the CQ dry run.

[danakj, 2016-08-15 18:20:25]

On 2016/08/14 11:10:44, gsennton_OOO_back_8.15 wrote:
> Ok, so I haven't created a separate class instead of JavaHandlerThread yet
> (since I think that class would need to be a friend class of ScopedAllowWait
> which requires approval from jam@ or brettw@).

If you're just moving a ScopedAllowWait from one class to another, or its in a
class just for testing, this should not be a problem at all.

> We should probably indeed create
> our own class (though maybe it's better to get this CL landed asap and
duplicate
> JavaHandlerThread as a follow-up?).
> 
> The only other concern I have right now is having added a try-catch statement
to
> the production code in SystemMessageHandler just for the sake of testing (this
> seems super ugly).

Is it possible to replace SystemMessageHandler's handleMessage with a test
implementation?

[danakj, 2016-08-15 18:29:59]

Thanks for all the work to get a test for this. LGTM but if you can see a way to
override the handler and move the try/catch to test-only code that'd be ideal.

https://codereview.chromium.org/2169553002/diff/60001/base/message_loop/messa...
File base/message_loop/message_loop_unittest.cc (right):

https://codereview.chromium.org/2169553002/diff/60001/base/message_loop/messa...
base/message_loop/message_loop_unittest.cc:95: void
RunTest_Abort_Dont_Run_More_Tasks(bool delayed) {
normally for CapitilizedNames you don't put _ between words.

[nyquist, 2016-08-15 19:06:13]

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

[nyquist, 2016-08-15 19:06:14]

https://codereview.chromium.org/2169553002/diff/60001/base/message_loop/messa...
File base/message_loop/message_pump_android.cc (right):

https://codereview.chromium.org/2169553002/diff/60001/base/message_loop/messa...
base/message_loop/message_pump_android.cc:147: Start(delegate);
Could this instantiate a subclass of SystemMessageHandler that overrides the
handleMessage method to do what you want?
If so, maybe then it'd be possible to clean up some of the other code as well
since then it's more contained to that subclass?

[gsennton, 2016-08-16 15:27:36]

Alright!

I've now added sub-classes for JavaHandlerThread (only on the native side) and
SystemMessageHandler (added a java-side sub-class and an extra class on the
native side for communicating with the java side) so that we can use these sub
classes for testing and avoid adding a lot of test-code to the original classes.

The sub class for JavaHandlerThread is compiled in its own component that is
only added to the unittest component ("base_unittests").
The sub class of SystemMessageHandler, however, is added to the "base" component
because it is initiated from message_pump_android (which lives in the base
component). If this is too bad we could maybe sub-class message_loop and
message_pump_android (MessagePumpForUI) as well (and start the
TestSystemMessageHandler from there, while putting all those sub classes in its
own component) but I'm not sure we want to do that?


ptal again :)

https://codereview.chromium.org/2169553002/diff/60001/base/message_loop/messa...
File base/message_loop/message_loop_unittest.cc (right):

https://codereview.chromium.org/2169553002/diff/60001/base/message_loop/messa...
base/message_loop/message_loop_unittest.cc:95: void
RunTest_Abort_Dont_Run_More_Tasks(bool delayed) {
On 2016/08/15 18:29:59, danakj wrote:
> normally for CapitilizedNames you don't put _ between words.

Done.

https://codereview.chromium.org/2169553002/diff/60001/base/message_loop/messa...
File base/message_loop/message_pump_android.cc (right):

https://codereview.chromium.org/2169553002/diff/60001/base/message_loop/messa...
base/message_loop/message_pump_android.cc:147: Start(delegate);
On 2016/08/15 19:06:14, nyquist wrote:
> Could this instantiate a subclass of SystemMessageHandler that overrides the
> handleMessage method to do what you want?
> If so, maybe then it'd be possible to clean up some of the other code as well
> since then it's more contained to that subclass?

Done.

https://codereview.chromium.org/2169553002/diff/80001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/SystemMessageHandler.java (right):

https://codereview.chromium.org/2169553002/diff/80001/base/android/java/src/o...
base/android/java/src/org/chromium/base/SystemMessageHandler.java:45: protected
long getMessagePumpNative() {
Ooops, will remove this.

[nyquist, 2016-08-16 17:46:39]

https://codereview.chromium.org/2169553002/diff/80001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/2169553002/diff/80001/base/BUILD.gn#newcode2448
base/BUILD.gn:2448: component("base_native_unittest_support") {
Did you mean for this to be a source_set?

https://codereview.chromium.org/2169553002/diff/80001/base/message_loop/messa...
File base/message_loop/message_pump_android.cc (right):

https://codereview.chromium.org/2169553002/diff/80001/base/message_loop/messa...
base/message_loop/message_pump_android.cc:143:
TestSystemMessageHandlerLink::createTestSystemMessageHandler(
Would it be possible to plumb through a factory (or worst case, a jobject
directly) for this, so you don't have to compile in the test code with prod
code?

[gsennton, 2016-08-17 17:05:48]

Well, this turn into a bit of a monster...

ptal :)

https://codereview.chromium.org/2169553002/diff/80001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/2169553002/diff/80001/base/BUILD.gn#newcode2448
base/BUILD.gn:2448: component("base_native_unittest_support") {
On 2016/08/16 17:46:39, nyquist wrote:
> Did you mean for this to be a source_set?

yeah, I've removed this completely and put the files under base/test/ instead
(in the library named "test_support"

https://codereview.chromium.org/2169553002/diff/80001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/SystemMessageHandler.java (right):

https://codereview.chromium.org/2169553002/diff/80001/base/android/java/src/o...
base/android/java/src/org/chromium/base/SystemMessageHandler.java:45: protected
long getMessagePumpNative() {
On 2016/08/16 15:27:36, gsennton wrote:
> Ooops, will remove this.

Done.

https://codereview.chromium.org/2169553002/diff/80001/base/message_loop/messa...
File base/message_loop/message_pump_android.cc (right):

https://codereview.chromium.org/2169553002/diff/80001/base/message_loop/messa...
base/message_loop/message_pump_android.cc:143:
TestSystemMessageHandlerLink::createTestSystemMessageHandler(
On 2016/08/16 17:46:39, nyquist wrote:
> Would it be possible to plumb through a factory (or worst case, a jobject
> directly) for this, so you don't have to compile in the test code with prod
> code?

Done.

[danakj, 2016-08-17 18:40:15]

https://codereview.chromium.org/2169553002/diff/120001/base/android/java_mess...
File base/android/java_message_handler_factory.h (right):

https://codereview.chromium.org/2169553002/diff/120001/base/android/java_mess...
base/android/java_message_handler_factory.h:23: JavaMessageHandlerFactory() {}
dont need this

https://codereview.chromium.org/2169553002/diff/120001/base/android/java_mess...
base/android/java_message_handler_factory.h:32:
DISALLOW_COPY_AND_ASSIGN(JavaMessageHandlerFactory);
or this, for a pure virtual interface

https://codereview.chromium.org/2169553002/diff/120001/base/message_loop/mess...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/2169553002/diff/120001/base/message_loop/mess...
base/message_loop/message_loop.cc:726: ->StartForTesting(this, factory,
test_done_event);
Oh I missed this earlier, but you can't call "ForTesting" methods from methods
that are not in test targets, this will trigger presubmit warnings for future
people (and should be for you too?). It will need a different name.

But I'm getting confused. There's a call to MessagePumpForUI::StartForTesting
here, but also in the JavaHandlerThread::StartMessageLoop override? Are both
needed?

https://codereview.chromium.org/2169553002/diff/120001/base/message_loop/mess...
File base/message_loop/message_loop.h (right):

https://codereview.chromium.org/2169553002/diff/120001/base/message_loop/mess...
base/message_loop/message_loop.h:46: } // namespace android
git cl format?

https://codereview.chromium.org/2169553002/diff/120001/base/message_loop/mess...
base/message_loop/message_loop.h:48: #endif  // defined(OS_ANDROID
)

https://codereview.chromium.org/2169553002/diff/120001/base/test/android/java...
File base/test/android/java_handler_thread_for_testing.cc (right):

https://codereview.chromium.org/2169553002/diff/120001/base/test/android/java...
base/test/android/java_handler_thread_for_testing.cc:10: 
nit: drop whitespace

https://codereview.chromium.org/2169553002/diff/120001/base/test/android/java...
base/test/android/java_handler_thread_for_testing.cc:30:
JavaHandlerThreadForTesting::~JavaHandlerThreadForTesting() {}
nit: = default;

[gsennton, 2016-08-17 19:23:47]

https://codereview.chromium.org/2169553002/diff/120001/base/android/java_mess...
File base/android/java_message_handler_factory.h (right):

https://codereview.chromium.org/2169553002/diff/120001/base/android/java_mess...
base/android/java_message_handler_factory.h:23: JavaMessageHandlerFactory() {}
On 2016/08/17 18:40:15, danakj wrote:
> dont need this

Done.

https://codereview.chromium.org/2169553002/diff/120001/base/android/java_mess...
base/android/java_message_handler_factory.h:32:
DISALLOW_COPY_AND_ASSIGN(JavaMessageHandlerFactory);
On 2016/08/17 18:40:15, danakj wrote:
> or this, for a pure virtual interface

Done.

https://codereview.chromium.org/2169553002/diff/120001/base/message_loop/mess...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/2169553002/diff/120001/base/message_loop/mess...
base/message_loop/message_loop.cc:726: ->StartForTesting(this, factory,
test_done_event);
On 2016/08/17 18:40:15, danakj wrote:
> Oh I missed this earlier, but you can't call "ForTesting" methods from methods
> that are not in test targets, this will trigger presubmit warnings for future
> people (and should be for you too?). It will need a different name.

Yeah, it's causing a presubmit warning for me (though it told me I could ignore
it if I knew what I was doing), changed to StartForUnitTest.

> 
> But I'm getting confused. There's a call to MessagePumpForUI::StartForTesting
> here, but also in the JavaHandlerThread::StartMessageLoop override? Are both
> needed?

JavaHandlerThread::StartMessageLoop is calling
MessageLoopForUI::StartForTesting, i.e. the method here, not the one in
MessagePumpForUI. MessageLoopForUI is just the middle-man in this case.

https://codereview.chromium.org/2169553002/diff/120001/base/message_loop/mess...
File base/message_loop/message_loop.h (right):

https://codereview.chromium.org/2169553002/diff/120001/base/message_loop/mess...
base/message_loop/message_loop.h:46: } // namespace android
On 2016/08/17 18:40:15, danakj wrote:
> git cl format?

Done.

https://codereview.chromium.org/2169553002/diff/120001/base/message_loop/mess...
base/message_loop/message_loop.h:48: #endif  // defined(OS_ANDROID
On 2016/08/17 18:40:15, danakj wrote:
> )

Done.

https://codereview.chromium.org/2169553002/diff/120001/base/test/android/java...
File base/test/android/java_handler_thread_for_testing.cc (right):

https://codereview.chromium.org/2169553002/diff/120001/base/test/android/java...
base/test/android/java_handler_thread_for_testing.cc:10: 
On 2016/08/17 18:40:15, danakj wrote:
> nit: drop whitespace

Done.

https://codereview.chromium.org/2169553002/diff/120001/base/test/android/java...
base/test/android/java_handler_thread_for_testing.cc:30:
JavaHandlerThreadForTesting::~JavaHandlerThreadForTesting() {}
On 2016/08/17 18:40:15, danakj wrote:
> nit: = default;

Done.

[danakj, 2016-08-17 20:22:30]

LGTM % android folks

[gsennton, 2016-08-18 13:03:09]

Thanks danakj@, I had to make an additional small change (see
base/run_all_base_unittests.cc), apart from that this CL should be done.

https://codereview.chromium.org/2169553002/diff/160001/base/run_all_base_unit...
File base/run_all_base_unittests.cc (right):

https://codereview.chromium.org/2169553002/diff/160001/base/run_all_base_unit...
base/run_all_base_unittests.cc:17:
base::android::TestSystemMessageHandlerLink::RegisterJNI(
Added this to register the jni bindings only in base_unittests - PS 8 fails
because it tries to add bindings for TestSystemMessageHandler to a lot of other
components that do not include the TestSystemMessageHandler.java class.

[Torne, 2016-08-18 14:33:43]

LGTM - thanks for the effort to get a test here!

[nyquist, 2016-08-18 15:50:03]

lgtm % nits

https://codereview.chromium.org/2169553002/diff/180001/android_webview/native...
File android_webview/native/aw_contents_client_bridge.cc (right):

https://codereview.chromium.org/2169553002/diff/180001/android_webview/native...
android_webview/native/aw_contents_client_bridge.cc:355: // any new jni calls.
Nit: JNI

https://codereview.chromium.org/2169553002/diff/180001/base/android/java_mess...
File base/android/java_message_handler_factory.h (right):

https://codereview.chromium.org/2169553002/diff/180001/base/android/java_mess...
base/android/java_message_handler_factory.h:18: * Factory for creating the
Java-side system message handler - only used for
Nit: Should be C++-style comment

https://codereview.chromium.org/2169553002/diff/180001/base/message_loop/mess...
File base/message_loop/message_pump_android.h (right):

https://codereview.chromium.org/2169553002/diff/180001/base/message_loop/mess...
base/message_loop/message_pump_android.h:45: // We can't call any jni-methods
before returning to Java as we would then
Nit: JNI and missing period at end of sentence.

https://codereview.chromium.org/2169553002/diff/180001/base/run_all_base_unit...
File base/run_all_base_unittests.cc (right):

https://codereview.chromium.org/2169553002/diff/180001/base/run_all_base_unit...
base/run_all_base_unittests.cc:1: // Copyright (c) 2016 The Chromium Authors.
All rights reserved.
Nit: No (c) here or in any other added file.

https://codereview.chromium.org/2169553002/diff/180001/base/test/android/java...
File base/test/android/java/src/org/chromium/base/TestSystemMessageHandler.java
(right):

https://codereview.chromium.org/2169553002/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/TestSystemMessageHandler.java:13:
class TestSystemMessageHandler extends SystemMessageHandler {
Nit: Could you explain how this class is different than the normal
SystemMessageHandler?

https://codereview.chromium.org/2169553002/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/TestSystemMessageHandler.java:14:
private static final String TAG = "cr.TestSysMessageHandler";
Nit: Is this used?

https://codereview.chromium.org/2169553002/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/TestSystemMessageHandler.java:15:
private long mWaitableTestEvent;
Nit: Could this also have Native in its name to clarify it's a pointer?

https://codereview.chromium.org/2169553002/diff/180001/base/test/android/java...
File base/test/android/java_handler_thread_for_testing.h (right):

https://codereview.chromium.org/2169553002/diff/180001/base/test/android/java...
base/test/android/java_handler_thread_for_testing.h:17: class
TestJavaMessageHandlerFactory : public JavaMessageHandlerFactory {
Nit: Could you explain why this class exists?

https://codereview.chromium.org/2169553002/diff/180001/base/test/android/java...
base/test/android/java_handler_thread_for_testing.h:25: class
JavaHandlerThreadForTesting : public JavaHandlerThread {
Nit: Could you add a comment as to what's special about this test-version? I.e.
how does this operate differently than the normal JavaHandlerThread?

https://codereview.chromium.org/2169553002/diff/180001/base/test/android/test...
File base/test/android/test_system_message_handler_link_android.cc (right):

https://codereview.chromium.org/2169553002/diff/180001/base/test/android/test...
base/test/android/test_system_message_handler_link_android.cc:9: 
Nit: Remove empty line.

https://codereview.chromium.org/2169553002/diff/180001/base/test/android/test...
File base/test/android/test_system_message_handler_link_android.h (right):

https://codereview.chromium.org/2169553002/diff/180001/base/test/android/test...
base/test/android/test_system_message_handler_link_android.h:22: * This provides
a link to (a way to create) the java-side
Nit: This should use // style comments

https://codereview.chromium.org/2169553002/diff/180001/base/test/android/test...
base/test/android/test_system_message_handler_link_android.h:36:
createTestSystemMessageHandler(JNIEnv* env,
Nit: CreateTestSystemMessageHandler

[danakj, 2016-08-18 17:50:16]

https://codereview.chromium.org/2169553002/diff/180001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/2169553002/diff/180001/base/BUILD.gn#newcode2198
base/BUILD.gn:2198: static_library("run_all_base_unittests") {
Please move this to base/test/BUILD.gn

https://codereview.chromium.org/2169553002/diff/180001/base/run_all_base_unit...
File base/run_all_base_unittests.cc (right):

https://codereview.chromium.org/2169553002/diff/180001/base/run_all_base_unit...
base/run_all_base_unittests.cc:10: #if defined(OS_ANDROID)
And plz move this file to base/test/ also

[danakj, 2016-08-18 18:53:57]

Thanks, LGTM

https://codereview.chromium.org/2169553002/diff/220001/base/test/BUILD.gn
File base/test/BUILD.gn (right):

https://codereview.chromium.org/2169553002/diff/220001/base/test/BUILD.gn#new...
base/test/BUILD.gn:270: # should depend on run_all_unittests above
nit: add period

[gsennton, 2016-08-18 18:56:55]

Alright! Commit queue inc.

https://codereview.chromium.org/2169553002/diff/180001/android_webview/native...
File android_webview/native/aw_contents_client_bridge.cc (right):

https://codereview.chromium.org/2169553002/diff/180001/android_webview/native...
android_webview/native/aw_contents_client_bridge.cc:355: // any new jni calls.
On 2016/08/18 15:50:02, nyquist wrote:
> Nit: JNI

Done.

https://codereview.chromium.org/2169553002/diff/180001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/2169553002/diff/180001/base/BUILD.gn#newcode2198
base/BUILD.gn:2198: static_library("run_all_base_unittests") {
On 2016/08/18 17:50:16, danakj wrote:
> Please move this to base/test/BUILD.gn

Done.

https://codereview.chromium.org/2169553002/diff/180001/base/android/java_mess...
File base/android/java_message_handler_factory.h (right):

https://codereview.chromium.org/2169553002/diff/180001/base/android/java_mess...
base/android/java_message_handler_factory.h:18: * Factory for creating the
Java-side system message handler - only used for
On 2016/08/18 15:50:02, nyquist wrote:
> Nit: Should be C++-style comment

Done.

https://codereview.chromium.org/2169553002/diff/180001/base/message_loop/mess...
File base/message_loop/message_pump_android.h (right):

https://codereview.chromium.org/2169553002/diff/180001/base/message_loop/mess...
base/message_loop/message_pump_android.h:45: // We can't call any jni-methods
before returning to Java as we would then
On 2016/08/18 15:50:02, nyquist wrote:
> Nit: JNI and missing period at end of sentence.

Done.

https://codereview.chromium.org/2169553002/diff/180001/base/run_all_base_unit...
File base/run_all_base_unittests.cc (right):

https://codereview.chromium.org/2169553002/diff/180001/base/run_all_base_unit...
base/run_all_base_unittests.cc:1: // Copyright (c) 2016 The Chromium Authors.
All rights reserved.
On 2016/08/18 15:50:02, nyquist wrote:
> Nit: No (c) here or in any other added file.

Done.

https://codereview.chromium.org/2169553002/diff/180001/base/run_all_base_unit...
base/run_all_base_unittests.cc:10: #if defined(OS_ANDROID)
On 2016/08/18 17:50:16, danakj wrote:
> And plz move this file to base/test/ also

Done.

https://codereview.chromium.org/2169553002/diff/180001/base/test/android/java...
File base/test/android/java/src/org/chromium/base/TestSystemMessageHandler.java
(right):

https://codereview.chromium.org/2169553002/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/TestSystemMessageHandler.java:13:
class TestSystemMessageHandler extends SystemMessageHandler {
On 2016/08/18 15:50:02, nyquist wrote:
> Nit: Could you explain how this class is different than the normal
> SystemMessageHandler?

Done.

https://codereview.chromium.org/2169553002/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/TestSystemMessageHandler.java:14:
private static final String TAG = "cr.TestSysMessageHandler";
On 2016/08/18 15:50:02, nyquist wrote:
> Nit: Is this used?

Nope, removed.

https://codereview.chromium.org/2169553002/diff/180001/base/test/android/java...
base/test/android/java/src/org/chromium/base/TestSystemMessageHandler.java:15:
private long mWaitableTestEvent;
On 2016/08/18 15:50:02, nyquist wrote:
> Nit: Could this also have Native in its name to clarify it's a pointer?

Done.

https://codereview.chromium.org/2169553002/diff/180001/base/test/android/java...
File base/test/android/java_handler_thread_for_testing.h (right):

https://codereview.chromium.org/2169553002/diff/180001/base/test/android/java...
base/test/android/java_handler_thread_for_testing.h:17: class
TestJavaMessageHandlerFactory : public JavaMessageHandlerFactory {
On 2016/08/18 15:50:03, nyquist wrote:
> Nit: Could you explain why this class exists?

Done.

https://codereview.chromium.org/2169553002/diff/180001/base/test/android/java...
base/test/android/java_handler_thread_for_testing.h:25: class
JavaHandlerThreadForTesting : public JavaHandlerThread {
On 2016/08/18 15:50:02, nyquist wrote:
> Nit: Could you add a comment as to what's special about this test-version?
I.e.
> how does this operate differently than the normal JavaHandlerThread?

Done.

https://codereview.chromium.org/2169553002/diff/180001/base/test/android/test...
File base/test/android/test_system_message_handler_link_android.cc (right):

https://codereview.chromium.org/2169553002/diff/180001/base/test/android/test...
base/test/android/test_system_message_handler_link_android.cc:9: 
On 2016/08/18 15:50:03, nyquist wrote:
> Nit: Remove empty line.

Done.

https://codereview.chromium.org/2169553002/diff/180001/base/test/android/test...
File base/test/android/test_system_message_handler_link_android.h (right):

https://codereview.chromium.org/2169553002/diff/180001/base/test/android/test...
base/test/android/test_system_message_handler_link_android.h:22: * This provides
a link to (a way to create) the java-side
On 2016/08/18 15:50:03, nyquist wrote:
> Nit: This should use // style comments

Done.

https://codereview.chromium.org/2169553002/diff/180001/base/test/android/test...
base/test/android/test_system_message_handler_link_android.h:36:
createTestSystemMessageHandler(JNIEnv* env,
On 2016/08/18 15:50:03, nyquist wrote:
> Nit: CreateTestSystemMessageHandler

Done.

https://codereview.chromium.org/2169553002/diff/220001/base/test/BUILD.gn
File base/test/BUILD.gn (right):

https://codereview.chromium.org/2169553002/diff/220001/base/test/BUILD.gn#new...
base/test/BUILD.gn:270: # should depend on run_all_unittests above
On 2016/08/18 18:53:57, danakj wrote:
> nit: add period

Done.

[gsennton, 2016-08-18 18:57:11]

The CQ bit was checked by gsennton@chromium.org

[gsennton, 2016-08-18 18:57:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, torne@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2169553002/#ps240001
(title: "Nit fix.")

[commit-bot: I haz the power, 2016-08-18 18:57:50]

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-08-18 19:55:47]

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

[commit-bot: I haz the power, 2016-08-18 19:55:49]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[gsennton, 2016-08-18 21:12:28]

The CQ bit was checked by gsennton@chromium.org

[gsennton, 2016-08-18 21:12:29]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
nyquist@chromium.org, torne@chromium.org
Link to the patchset: https://codereview.chromium.org/2169553002/#ps260001
(title: "Fix trybot compilation failure.")

[commit-bot: I haz the power, 2016-08-18 21:13:01]

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-08-18 22:34:33]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-08-18 23:24:18]

Description was changed from

==========
Properly throw java exceptions from shouldOverrideUrlLoading

[WebView]
Since all our java methods called through the jni-interface are
annotated with @CalledByNative (rather than @CalledByNativeUnchecked)
any java crash in those methods will cause a native crash since we call
jni_android::CheckException (which aborts in native code) when returning
from those methods.

In this CL we annotate the jni-call for shouldOverrideUrlLoading with
@CalledByNativeUnchecked to avoid crashing directly after the jni-call.

We also add an Abort function for the android specific message loop and
message pump to ensure no new tasks will be run before we return to the
Java handler (at which point the original Java exception will be
rethrown and thus correctly reported).

BUG=592556
==========

to

==========
Properly throw java exceptions from shouldOverrideUrlLoading

[WebView]
Since all our java methods called through the jni-interface are
annotated with @CalledByNative (rather than @CalledByNativeUnchecked)
any java crash in those methods will cause a native crash since we call
jni_android::CheckException (which aborts in native code) when returning
from those methods.

In this CL we annotate the jni-call for shouldOverrideUrlLoading with
@CalledByNativeUnchecked to avoid crashing directly after the jni-call.

We also add an Abort function for the android specific message loop and
message pump to ensure no new tasks will be run before we return to the
Java handler (at which point the original Java exception will be
rethrown and thus correctly reported).

BUG=592556

Committed: https://crrev.com/ebe2e203bd421bc1a5e4c28a48f7d54d407c6fac
Cr-Commit-Position: refs/heads/master@{#412956}
==========

[commit-bot: I haz the power, 2016-08-18 23:24:19]

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