[Android] Fix race condition in BaseTestServer.

[Android] Fix race condition in BaseTestServer.

BUG=485417
Committed: https://crrev.com/1ce809f9a732b049e4c0bd7ea54d454ca48537d9
Cr-Commit-Position: refs/heads/master@{#329111}

[jbudorick, 2015-05-07 00:46:21]

jbudorick@chromium.org changed reviewers:
+ nyquist@chromium.org, rsleevi@chromium.org

[jbudorick, 2015-05-07 00:46:21]

In addition to partial duplication of the //net test server,
https://codereview.chromium.org/869083004/ also contained a fix for a race
condition in BaseTestServer. That CL didn't make it through reviews due to
larger issues with my approach to the //net test server and android, but the
code that triggers the race condition is still used in some scenarios (e.g. when
running instrumentation tests on Appurify).

[Ryan Sleevi, 2015-05-07 01:02:56]

rsleevi@chromium.org changed reviewers:
+ ellyjones@chromium.org, mef@chromium.org

[Ryan Sleevi, 2015-05-07 01:02:57]

Misha or Elly: Would you be suited to do this review? It's unclear who is
hacking on Java + Cronet these days, but it'd be great to get perspective of
someone with Java & //net experience to review this.

[Ryan Sleevi, 2015-05-07 01:03:55]

Also, can you make sure there is a bug on file documenting the issues here?

[jbudorick, 2015-05-07 03:44:18]

On 2015/05/07 01:03:55, Ryan Sleevi wrote:
> Also, can you make sure there is a bug on file documenting the issues here?

done

[chromium-reviews, 2015-05-07 14:55:45]

I'll be happy to review.

On Wed, May 6, 2015 at 11:44 PM, <jbudorick@chromium.org> wrote:

> On 2015/05/07 01:03:55, Ryan Sleevi wrote:
>
>> Also, can you make sure there is a bug on file documenting the issues
>> here?
>>
>
> done
>
> https://codereview.chromium.org/1131803002/
>

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

[mef, 2015-05-07 15:04:32]

https://codereview.chromium.org/1131803002/diff/1/net/test/android/javatests/...
File net/test/android/javatests/src/org/chromium/net/test/BaseTestServer.java
(left):

https://codereview.chromium.org/1131803002/diff/1/net/test/android/javatests/...
net/test/android/javatests/src/org/chromium/net/test/BaseTestServer.java:20:
mKeepRunning.set(true);
Don't you need this if server is stopped and restarted?

https://codereview.chromium.org/1131803002/diff/1/net/test/android/javatests/...
File net/test/android/javatests/src/org/chromium/net/test/BaseTestServer.java
(right):

https://codereview.chromium.org/1131803002/diff/1/net/test/android/javatests/...
net/test/android/javatests/src/org/chromium/net/test/BaseTestServer.java:26:
public abstract int getServerPort();
Does it belong to this CL?

If so, shouldn't there be a corresponding addition to the concrete class that
inherits from BaseTestServer?

https://codereview.chromium.org/1131803002/diff/1/net/test/android/javatests/...
net/test/android/javatests/src/org/chromium/net/test/BaseTestServer.java:43:
synchronized (mLock) {
I don't think this works. The wait loop is inside of synchronized(mLock), so the
server thread will be blocked inside of serverHasStarted().

[jbudorick, 2015-05-07 15:41:17]

https://codereview.chromium.org/1131803002/diff/1/net/test/android/javatests/...
File net/test/android/javatests/src/org/chromium/net/test/BaseTestServer.java
(left):

https://codereview.chromium.org/1131803002/diff/1/net/test/android/javatests/...
net/test/android/javatests/src/org/chromium/net/test/BaseTestServer.java:20:
mKeepRunning.set(true);
On 2015/05/07 15:04:32, mef wrote:
> Don't you need this if server is stopped and restarted?

Restarting isn't supported at all at the moment since Threads can't be started
more than once.

If it were, then yes.

https://codereview.chromium.org/1131803002/diff/1/net/test/android/javatests/...
File net/test/android/javatests/src/org/chromium/net/test/BaseTestServer.java
(right):

https://codereview.chromium.org/1131803002/diff/1/net/test/android/javatests/...
net/test/android/javatests/src/org/chromium/net/test/BaseTestServer.java:26:
public abstract int getServerPort();
On 2015/05/07 15:04:32, mef wrote:
> Does it belong to this CL?
> 
> If so, shouldn't there be a corresponding addition to the concrete class that
> inherits from BaseTestServer?

Nope, this is a renegade from the other CL. Removed.

https://codereview.chromium.org/1131803002/diff/1/net/test/android/javatests/...
net/test/android/javatests/src/org/chromium/net/test/BaseTestServer.java:43:
synchronized (mLock) {
On 2015/05/07 15:04:32, mef wrote:
> I don't think this works. The wait loop is inside of synchronized(mLock), so
the
> server thread will be blocked inside of serverHasStarted().

This definitely works. wait() doesn't hold the monitor while waiting to be
notified.

[mef, 2015-05-07 15:59:56]

Hrm, maybe I'm missing something, doesn't synchronized(object) {} hold an
exclusive lock on the object?

https://codereview.chromium.org/1131803002/diff/1/net/test/android/javatests/...
File net/test/android/javatests/src/org/chromium/net/test/BaseTestServer.java
(right):

https://codereview.chromium.org/1131803002/diff/1/net/test/android/javatests/...
net/test/android/javatests/src/org/chromium/net/test/BaseTestServer.java:43:
synchronized (mLock) {
On 2015/05/07 15:41:17, jbudorick wrote:
> On 2015/05/07 15:04:32, mef wrote:
> > I don't think this works. The wait loop is inside of synchronized(mLock), so
> the
> > server thread will be blocked inside of serverHasStarted().
> 
> This definitely works. wait() doesn't hold the monitor while waiting to be
> notified.

Right, but while this method is waiting in the loop the mLock is locked by
synchronized block.

https://codereview.chromium.org/1131803002/diff/20001/net/test/android/javate...
File net/test/android/javatests/src/org/chromium/net/test/BaseTestServer.java
(right):

https://codereview.chromium.org/1131803002/diff/20001/net/test/android/javate...
net/test/android/javatests/src/org/chromium/net/test/BaseTestServer.java:52:
synchronized (mLock) {
If this method is called after waitForServerToStart(), then this will be waiting
for the lock held in waitForServerToStart().

[jbudorick, 2015-05-07 16:24:55]

On 2015/05/07 15:59:56, mef wrote:
> Hrm, maybe I'm missing something, doesn't synchronized(object) {} hold an
> exclusive lock on the object?

synchronized (object) {} will grab the object's monitor (~ object lock). Waiting
on an object while holding its monitor (which is a precondition for waiting on
an object:
http://docs.oracle.com/javase/7/docs/api/java/lang/Object.html#wait()) will
release the monitor, wait for a notification on the monitor, then reacquire the
monitor before continuing.

> 
>
https://codereview.chromium.org/1131803002/diff/1/net/test/android/javatests/...
> File net/test/android/javatests/src/org/chromium/net/test/BaseTestServer.java
> (right):
> 
>
https://codereview.chromium.org/1131803002/diff/1/net/test/android/javatests/...
> net/test/android/javatests/src/org/chromium/net/test/BaseTestServer.java:43:
> synchronized (mLock) {
> On 2015/05/07 15:41:17, jbudorick wrote:
> > On 2015/05/07 15:04:32, mef wrote:
> > > I don't think this works. The wait loop is inside of synchronized(mLock),
so
> > the
> > > server thread will be blocked inside of serverHasStarted().
> > 
> > This definitely works. wait() doesn't hold the monitor while waiting to be
> > notified.
> 
> Right, but while this method is waiting in the loop the mLock is locked by
> synchronized block.
> 
>
https://codereview.chromium.org/1131803002/diff/20001/net/test/android/javate...
> File net/test/android/javatests/src/org/chromium/net/test/BaseTestServer.java
> (right):
> 
>
https://codereview.chromium.org/1131803002/diff/20001/net/test/android/javate...
> net/test/android/javatests/src/org/chromium/net/test/BaseTestServer.java:52:
> synchronized (mLock) {
> If this method is called after waitForServerToStart(), then this will be
waiting
> for the lock held in waitForServerToStart().

In this scenario:

1. waitForServerToStart is called. It acquires mLock's monitor on entering the
synchronized block.
2. serverHasStarted is called. It is initially unable to acquire mLock's
monitor, so it's blocked at the entry to the synchronzied block.
3. waitForServerToStart calls mLock.wait(), releasing mLock's monitor and
waiting for a notify() or notifyAll() call on mLock.
4. serverHasStarted now acquires mLock's monitor and enters the synchronized
block.
5. serverHasStarted sets mRunning to true.
6. serverHasStarted calls mLock.notifyAll(). waitForServerToStart is notified
and starts trying to reacquire mLock's monitor. It is initially unable to do so
because serverHasStarted still holds the monitor.
7. serverHasStarted exits the synchronized block, releasing mLock's monitor.
8. waitForServerToStart reacquires the monitor and mLock.wait() returns.
9. waitForServerToStart sees that mRunning has been set to true and completes.

[mef, 2015-05-07 16:34:44]

Ah, I've missed the part that wait() releases the lock. Thanks for the
explanation, LGTM.

[nyquist, 2015-05-07 16:37:25]

https://codereview.chromium.org/1131803002/diff/20001/net/test/android/javate...
File net/test/android/javatests/src/org/chromium/net/test/BaseTestServer.java
(right):

https://codereview.chromium.org/1131803002/diff/20001/net/test/android/javate...
net/test/android/javatests/src/org/chromium/net/test/BaseTestServer.java:52:
synchronized (mLock) {
On 2015/05/07 15:59:56, mef wrote:
> If this method is called after waitForServerToStart(), then this will be
waiting
> for the lock held in waitForServerToStart().

When wait is invoked, the thread releases the lock and suspends execution.

The part inside synchronized(THIS_OBJECT) must match the THIS_OBJECT.wait() and
THIS_OBJECT.notify() for this to work correctly.

See http://developer.android.com/reference/java/lang/Object.html#wait() or 
See https://docs.oracle.com/javase/tutorial/essential/concurrency/guardmeth.html
for some more helpful concurrency documentation.

[jbudorick, 2015-05-08 16:38:15]

Tommy, can you do an owners review for chrome/test/android/?

[nyquist, 2015-05-08 16:56:52]

chrome/test/android lgtm

[jbudorick, 2015-05-08 17:01:53]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2015-05-08 17:03:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1131803002/20001

[commit-bot: I haz the power, 2015-05-08 17:14:56]

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

[commit-bot: I haz the power, 2015-05-08 17:14:58]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)

[jbudorick, 2015-05-08 17:36:20]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2015-05-08 17:36:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, mef@chromium.org
Link to the patchset: https://codereview.chromium.org/1131803002/#ps40001
(title: "rebase + fix gyp")

[commit-bot: I haz the power, 2015-05-08 17:36:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1131803002/40001

[commit-bot: I haz the power, 2015-05-08 17:55:15]

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

[commit-bot: I haz the power, 2015-05-08 17:55:16]

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

[jbudorick, 2015-05-08 18:06:43]

https://codereview.chromium.org/1131803002/diff/1/net/test/android/javatests/...
File net/test/android/javatests/src/org/chromium/net/test/BaseTestServer.java
(left):

https://codereview.chromium.org/1131803002/diff/1/net/test/android/javatests/...
net/test/android/javatests/src/org/chromium/net/test/BaseTestServer.java:31:
public abstract int getServerPort();
(moved from here)

https://codereview.chromium.org/1131803002/diff/1/net/test/android/javatests/...
File net/test/android/javatests/src/org/chromium/net/test/BaseTestServer.java
(right):

https://codereview.chromium.org/1131803002/diff/1/net/test/android/javatests/...
net/test/android/javatests/src/org/chromium/net/test/BaseTestServer.java:26:
public abstract int getServerPort();
On 2015/05/07 15:41:17, jbudorick wrote:
> On 2015/05/07 15:04:32, mef wrote:
> > Does it belong to this CL?
> > 
> > If so, shouldn't there be a corresponding addition to the concrete class
that
> > inherits from BaseTestServer?
> 
> Nope, this is a renegade from the other CL. Removed.

Turns out this isn't a renegade, it just moved up from below. Deleting it causes
problems, unsurprisingly.

[jbudorick, 2015-05-08 19:12:47]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2015-05-08 19:12:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, mef@chromium.org
Link to the patchset: https://codereview.chromium.org/1131803002/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-05-08 19:13:17]

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

[commit-bot: I haz the power, 2015-05-09 01:22:58]

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

[commit-bot: I haz the power, 2015-05-09 01:22:59]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[nyquist, 2015-05-09 01:51:08]

The CQ bit was checked by nyquist@chromium.org

[commit-bot: I haz the power, 2015-05-09 01:55:07]

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

[commit-bot: I haz the power, 2015-05-09 06:10:18]

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

[commit-bot: I haz the power, 2015-05-09 06:10:22]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)

[jbudorick, 2015-05-09 09:22:40]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2015-05-09 09:26:11]

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

[commit-bot: I haz the power, 2015-05-09 13:30:21]

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

[commit-bot: I haz the power, 2015-05-09 13:30:23]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)

[jbudorick, 2015-05-11 00:50:11]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2015-05-11 00:51:36]

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

[commit-bot: I haz the power, 2015-05-11 07:45:07]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-11 07:46:00]

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