Add isolate for content_browsertests with --site-per-process.

Add isolate for content_browsertests with --site-per-process.

(Patch from issue 1572633002 at patchset 200001 (http://crrev.com/1572633002#ps200001))

BUG=579704
R=nasko@chromium.org, maruel@chromium.org, phajdan.jr@chromium.org

Committed: https://crrev.com/fe0d35e9672feaaa8c19612dcc33ede70738c0e0
Cr-Commit-Position: refs/heads/master@{#373727}

[Dirk Pranke, 2016-01-22 01:54:42]

Description was changed from

==========
Add isolate for content_browsertests with --site-per-process.

BUG=579704

patch from issue 1572633002 at patchset 200001
(http://crrev.com/1572633002#ps200001)
==========

to

==========
Add isolate for content_browsertests with --site-per-process.

(Patch from issue 1572633002 at patchset 200001
(http://crrev.com/1572633002#ps200001))

BUG=579704
R=nasko@chromium.org, maruel@chromium.org, phajdan.jr@chromium.org
==========

[Dirk Pranke, 2016-01-22 01:54:42]

dpranke@chromium.org changed reviewers:
+ maruel@chromium.org, nasko@chromium.org, phajdan.jr@chromium.org

[Dirk Pranke, 2016-01-22 01:55:11]

Original patchset is from Nasko's original CL; the second patchset has the
necessary fixes and should (?) work. I'll send out a try job now.

[nasko, 2016-01-22 04:52:45]

On 2016/01/22 01:55:11, Dirk Pranke (slow) wrote:
> Original patchset is from Nasko's original CL; the second patchset has the
> necessary fixes and should (?) work. I'll send out a try job now.

It is looking for the actual target to be a binary, which it isn't:

Failed to start ['./content_site_isolation_browsertests',
'--brave-new-test-launcher', '--test-launcher-bot-mode', '--site-per-process',
'--test-launcher-filter-file=src/testing/buildbot/filters/site-per-process.content_browsertests.filter',
'--test-launcher-summary-output=/tmp/out7jId5l/output.json']
Additional test environment:
    CHROME_DEVEL_SANDBOX=/opt/chromium/chrome_sandbox
    LANG=en_US.UTF-8
Command: ./content_site_isolation_browsertests --brave-new-test-launcher
--test-launcher-bot-mode --site-per-process
--test-launcher-filter-file=src/testing/buildbot/filters/site-per-process.content_browsertests.filter
--test-launcher-summary-output=/tmp/out7jId5l/output.json

Traceback (most recent call last):
  File "../../testing/xvfb.py", line 142, in <module>
    sys.exit(main())
  File "../../testing/xvfb.py", line 138, in main
    return run_executable(sys.argv[2:], sys.argv[1], os.environ.copy())
  File "../../testing/xvfb.py", line 127, in run_executable
    return test_env.run_executable(cmd, env)
  File "/tmp/runFy8S3s/testing/test_env.py", line 228, in run_executable
    return subprocess.call(cmd, env=env)
  File "/usr/lib/python2.7/subprocess.py", line 493, in call
    return Popen(*popenargs, **kwargs).wait()
  File "/usr/lib/python2.7/subprocess.py", line 679, in __init__
    errread, errwrite)
  File "/usr/lib/python2.7/subprocess.py", line 1249, in _execute_child
    raise child_exception
OSError: [Errno 2] No such file or directory

[M-A Ruel, 2016-01-22 18:52:29]

https://codereview.chromium.org/1620513002/diff/20001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/1620513002/diff/20001/content/content_tests.g...
content/content_tests.gypi:1606: 'content_browsertests_run',
'content_browsertests',
?

[nasko, 2016-01-26 19:47:26]

On 2016/01/22 18:52:29, M-A Ruel wrote:
>
https://codereview.chromium.org/1620513002/diff/20001/content/content_tests.gypi
> File content/content_tests.gypi (right):
> 
>
https://codereview.chromium.org/1620513002/diff/20001/content/content_tests.g...
> content/content_tests.gypi:1606: 'content_browsertests_run',
> 'content_browsertests',
> ?

I tried that in a separate CL, since we were moving offices and it didn't help.

Any other ideas?

[Dirk Pranke, 2016-01-26 21:48:38]

Sorry (again) for the delays ... I'll look at this more this afternoon.

[Dirk Pranke, 2016-01-27 01:03:35]

On 2016/01/26 21:48:38, Dirk Pranke wrote:
> Sorry (again) for the delays ... I'll look at this more this afternoon.

Urk, I wasn't able to get to this this afternoon. I might be able to get to it
tonight, but if not, first thing tomorrow. Sorry!

[nasko, 2016-01-28 22:09:20]

On 2016/01/27 01:03:35, Dirk Pranke wrote:
> On 2016/01/26 21:48:38, Dirk Pranke wrote:
> > Sorry (again) for the delays ... I'll look at this more this afternoon.
> 
> Urk, I wasn't able to get to this this afternoon. I might be able to get to it
> tonight, but if not, first thing tomorrow. Sorry!

Anything I can do to help?

[Dirk Pranke, 2016-01-29 01:41:10]

On 2016/01/28 22:09:20, nasko wrote:
> Anything I can do to help?

Not at the moment. Sorry again for the delay; I finally got time again and fixed
what I think is the latest bug and am trying another job now ...

[Dirk Pranke, 2016-01-29 02:23:37]

On 2016/01/29 01:41:10, Dirk Pranke wrote:
> On 2016/01/28 22:09:20, nasko wrote:
> > Anything I can do to help?
> 
> Not at the moment. Sorry again for the delay; I finally got time again and
fixed
> what I think is the latest bug and am trying another job now ...

Hmm. The latest try job passed, but it looks like
content_site_isolation_browsertests 
didn't build or run at all because analyze didn't report it as needing to be
run.

So, my latest attempt at things broke 'analyze', but I don't yet know exactly
why
(though I have some guesses).

I'll look at this more again tomorrow. Sigh.

[Dirk Pranke, 2016-01-29 22:49:11]

strange things are afoot. I don't understand why patchset #3 failed the way it
did, and attempting to revert that change (and re-run) patchset #2,
patchset #4 fails in the same way (the list of compile_targets is wrong). This
suggests that something changed in the build repo in between 4:30 and
5:30 yesterday that broke this, but I'm not seeing any suspicious changes.

I am digging into things further.

[Dirk Pranke, 2016-01-30 00:47:12]

okay, the problem in patchset #3 and #4 was weird (crbug.com/582659) but I've
gotten past that now and we're getting closer.

Looks like we need to add the filter file to the isolate now.

[Dirk Pranke, 2016-02-02 01:45:43]

On 2016/01/30 00:47:12, Dirk Pranke wrote:
> okay, the problem in patchset #3 and #4 was weird (crbug.com/582659) but I've
> gotten past that now and we're getting closer.
> 
> Looks like we need to add the filter file to the isolate now.

Okay, I've added the file to the isolate and as far as I can tell, the command
line is correct, 
but the step is still failing.

nasko@, phajdan.jr@: is it possible the syntax of the filter file is wrong? 

If I download the isolate and try to run things locally, I get the same error,
so maybe one of you two can debug into it and see what's going on?

See: https://chromium-swarm.appspot.com/user/task/2cb850441c4ccf10

[nasko, 2016-02-02 01:59:12]

On 2016/02/02 01:45:43, Dirk Pranke wrote:
> On 2016/01/30 00:47:12, Dirk Pranke wrote:
> > okay, the problem in patchset #3 and #4 was weird (crbug.com/582659) but
I've
> > gotten past that now and we're getting closer.
> > 
> > Looks like we need to add the filter file to the isolate now.
> 
> Okay, I've added the file to the isolate and as far as I can tell, the command
> line is correct, 
> but the step is still failing.
> 
> nasko@, phajdan.jr@: is it possible the syntax of the filter file is wrong? 
> 
> If I download the isolate and try to run things locally, I get the same error,
> so maybe one of you two can debug into it and see what's going on?
> 
> See: https://chromium-swarm.appspot.com/user/task/2cb850441c4ccf10

The filter file is used on our FYI and trybots, so it is unlikely that it is
incorrect:
https://code.google.com/p/chromium/codesearch#chromium/src/testing/buildbot/c...

However, I do see that the paths seem to be different. Shouldn't we use the same
src/ based path instead of relative paths?

[nasko, 2016-02-02 02:03:21]

On 2016/02/02 01:59:12, nasko wrote:
> On 2016/02/02 01:45:43, Dirk Pranke wrote:
> > On 2016/01/30 00:47:12, Dirk Pranke wrote:
> > > okay, the problem in patchset #3 and #4 was weird (crbug.com/582659) but
> I've
> > > gotten past that now and we're getting closer.
> > > 
> > > Looks like we need to add the filter file to the isolate now.
> > 
> > Okay, I've added the file to the isolate and as far as I can tell, the
command
> > line is correct, 
> > but the step is still failing.
> > 
> > nasko@, phajdan.jr@: is it possible the syntax of the filter file is wrong? 
> > 
> > If I download the isolate and try to run things locally, I get the same
error,
> > so maybe one of you two can debug into it and see what's going on?
> > 
> > See: https://chromium-swarm.appspot.com/user/task/2cb850441c4ccf10
> 
> The filter file is used on our FYI and trybots, so it is unlikely that it is
> incorrect:
>
https://code.google.com/p/chromium/codesearch#chromium/src/testing/buildbot/c...
> 
> However, I do see that the paths seem to be different. Shouldn't we use the
same
> src/ based path instead of relative paths?

I just tried the command as listed and it failed for me too. However running it
from the src/ directory without relative path, it seems to work just fine:

./out/asan/content_browsertests --brave-new-test-launcher
--test-launcher-bot-mode --site-per-process
--test-launcher-filter-file=testing/buildbot/filters/site-per-process.content_browsertests.filter
--test-launcher-summary-output=/tmp/outESZINc/output.json

Pawel, is there a reason why relative paths don't work for reading the filter
file?

[Łukasz Anforowicz, 2016-02-02 20:25:15]

On 2016/02/02 02:03:21, nasko wrote:
> I just tried the command as listed and it failed for me too. However running
it
> from the src/ directory without relative path, it seems to work just fine:
> 
> ./out/asan/content_browsertests --brave-new-test-launcher
> --test-launcher-bot-mode --site-per-process
>
--test-launcher-filter-file=testing/buildbot/filters/site-per-process.content_browsertests.filter
> --test-launcher-summary-output=/tmp/outESZINc/output.json
> 
> Pawel, is there a reason why relative paths don't work for reading the filter
> file?

I think this can be explained by:
base/test/launcher/test_launcher.cc:

  if (command_line->HasSwitch(switches::kTestLauncherFilterFile)) {
    std::string filter;
    if (!ReadFileToString(
            command_line->GetSwitchValuePath(switches::kTestLauncherFilterFile),
            &filter)) {
      LOG(ERROR) << "Failed to read the filter file.";
      return false;
    }

and by
base/files/file_util.cc:
bool ReadFileToString(const FilePath& path,
                      std::string* contents,
                      size_t max_size) {
  if (contents)
    contents->clear();
  if (path.ReferencesParent())
    return false;

Fix proposal in crrev.com/1659943003.

[Dirk Pranke, 2016-02-02 21:35:57]

On 2016/02/02 20:25:15, Łukasz Anforowicz wrote:
> I think this can be explained by:
> base/test/launcher/test_launcher.cc:

...

> Fix proposal in crrev.com/1659943003.

Nice catch! I'll post a version w/ that fix here so we can see if the CL
passes with it.

[nasko, 2016-02-03 04:14:27]

On 2016/02/02 21:35:57, Dirk Pranke wrote:
> On 2016/02/02 20:25:15, Łukasz Anforowicz wrote:
> > I think this can be explained by:
> > base/test/launcher/test_launcher.cc:
> 
> ...
> 
> > Fix proposal in crrev.com/1659943003.
> 
> Nice catch! I'll post a version w/ that fix here so we can see if the CL
> passes with it.

Awesome! It passed *and* ran properly :).

[Dirk Pranke, 2016-02-03 05:32:08]

Progress!

Nasko, can you review/approve this, and I can land a version that doesn't have
the patch to //base ?

[nasko, 2016-02-03 05:37:56]

On 2016/02/03 05:32:08, Dirk Pranke wrote:
> Progress!
> 
> Nasko, can you review/approve this, and I can land a version that doesn't have
> the patch to //base ?

I'm not sure I'm a qualified reviewer, but FWIW, LGTM.

[Dirk Pranke, 2016-02-03 05:41:20]

On 2016/02/03 05:37:56, nasko wrote:
> On 2016/02/03 05:32:08, Dirk Pranke wrote:
> > Progress!
> > 
> > Nasko, can you review/approve this, and I can land a version that doesn't
have
> > the patch to //base ?
> 
> I'm not sure I'm a qualified reviewer, but FWIW, LGTM.

You validated the site isolation-specific stuff, which was the important part.
I'll get OWNERS for the rest.

[nasko, 2016-02-03 05:42:13]

On 2016/02/03 05:41:20, Dirk Pranke wrote:
> On 2016/02/03 05:37:56, nasko wrote:
> > On 2016/02/03 05:32:08, Dirk Pranke wrote:
> > > Progress!
> > > 
> > > Nasko, can you review/approve this, and I can land a version that doesn't
> have
> > > the patch to //base ?
> > 
> > I'm not sure I'm a qualified reviewer, but FWIW, LGTM.
> 
> You validated the site isolation-specific stuff, which was the important part.
> I'll get OWNERS for the rest.

Just one concern - should we send heads up to chromium-dev@, since this a new
set of tests?

[Dirk Pranke, 2016-02-03 05:44:53]

I wouldn't bother. We add new test binaries fairly often and don't send out
announcements ...

[Paweł Hajdan Jr., 2016-02-04 12:20:54]

LGTM

[Dirk Pranke, 2016-02-05 00:28:42]

The CQ bit was checked by dpranke@chromium.org

[Dirk Pranke, 2016-02-05 00:28:43]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1620513002/#ps200001
(title: "merge to #373467")

[commit-bot: I haz the power, 2016-02-05 00:29:41]

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

[commit-bot: I haz the power, 2016-02-05 00:42:42]

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

[commit-bot: I haz the power, 2016-02-05 00:42:43]

Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Dirk Pranke, 2016-02-05 00:59:53]

The CQ bit was checked by dpranke@chromium.org

[Dirk Pranke, 2016-02-05 00:59:53]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org,
phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/1620513002/#ps220001
(title: "merge to #373669")

[commit-bot: I haz the power, 2016-02-05 01:03:57]

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

[commit-bot: I haz the power, 2016-02-05 01:12:38]

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

[commit-bot: I haz the power, 2016-02-05 01:12:38]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Dirk Pranke, 2016-02-05 01:26:47]

The CQ bit was checked by dpranke@chromium.org

[Dirk Pranke, 2016-02-05 01:26:47]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org,
phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/1620513002/#ps240001
(title: "fix gyp declaration")

[commit-bot: I haz the power, 2016-02-05 01:27:14]

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

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

Description was changed from

==========
Add isolate for content_browsertests with --site-per-process.

(Patch from issue 1572633002 at patchset 200001
(http://crrev.com/1572633002#ps200001))

BUG=579704
R=nasko@chromium.org, maruel@chromium.org, phajdan.jr@chromium.org
==========

to

==========
Add isolate for content_browsertests with --site-per-process.

(Patch from issue 1572633002 at patchset 200001
(http://crrev.com/1572633002#ps200001))

BUG=579704
R=nasko@chromium.org, maruel@chromium.org, phajdan.jr@chromium.org
==========

[commit-bot: I haz the power, 2016-02-05 02:44:11]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-02-05 02:45:14]

Description was changed from

==========
Add isolate for content_browsertests with --site-per-process.

(Patch from issue 1572633002 at patchset 200001
(http://crrev.com/1572633002#ps200001))

BUG=579704
R=nasko@chromium.org, maruel@chromium.org, phajdan.jr@chromium.org
==========

to

==========
Add isolate for content_browsertests with --site-per-process.

(Patch from issue 1572633002 at patchset 200001
(http://crrev.com/1572633002#ps200001))

BUG=579704
R=nasko@chromium.org, maruel@chromium.org, phajdan.jr@chromium.org

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

[commit-bot: I haz the power, 2016-02-05 02:45:15]

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