CPM: revised unclean exit watching, revised renderer crash test

CPM: revised unclean exit watching, revised renderer crash test

Fixed unclean exit watching to work on CrOS
Added a cleaner waiting structure to renderer crash to see if it clears up the
CrOS ASAN error.

BUG=137726, 130212
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149697

[Devlin, 2012-07-24 19:50:18]

The only significant changes between this and
https://chromiumcodereview.appspot.com/10703078 should be in
c/b/performance_monitor/performance_monitor_browsertest.cc

[Devlin, 2012-07-26 17:09:44]

Yoyo,

Another for you, when you have the time. :)

[Yoyo Zhou, 2012-07-30 09:01:02]

http://chromiumcodereview.appspot.com/10815079/diff/11001/chrome/browser/perf...
File chrome/browser/performance_monitor/performance_monitor.h (right):

http://chromiumcodereview.appspot.com/10815079/diff/11001/chrome/browser/perf...
chrome/browser/performance_monitor/performance_monitor.h:118:
base::RepeatingTimer<PerformanceMonitor> timer_;
Comment what this is used for.

http://chromiumcodereview.appspot.com/10815079/diff/11001/chrome/browser/perf...
File chrome/browser/performance_monitor/performance_monitor_browsertest.cc
(right):

http://chromiumcodereview.appspot.com/10815079/diff/11001/chrome/browser/perf...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:212: //
The default profile directory can be changed on CrOS if we are logged in
What does 'can be changed' mean?

http://chromiumcodereview.appspot.com/10815079/diff/11001/chrome/browser/perf...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:217: if
(command_line.HasSwitch(switches::kLoginProfile)) {
What's the default setup for this switch on the ChromeOS bots?

http://chromiumcodereview.appspot.com/10815079/diff/11001/chrome/browser/perf...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:258:
std::string first_profile_name() { return first_profile_name_; }
nit: it doesn't look like these accessors are necessary.

http://chromiumcodereview.appspot.com/10815079/diff/11001/chrome/browser/perf...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:498: const
size_t kNumEvents = 1;
Define this closer to where it's used.

http://chromiumcodereview.appspot.com/10815079/diff/11001/chrome/browser/perf...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:523: const
size_t kNumEvents = 2;
ditto

[Devlin, 2012-07-30 17:22:25]

https://chromiumcodereview.appspot.com/10815079/diff/11001/chrome/browser/per...
File chrome/browser/performance_monitor/performance_monitor.h (right):

https://chromiumcodereview.appspot.com/10815079/diff/11001/chrome/browser/per...
chrome/browser/performance_monitor/performance_monitor.h:118:
base::RepeatingTimer<PerformanceMonitor> timer_;
On 2012/07/30 09:01:02, Yoyo Zhou wrote:
> Comment what this is used for.

Done.

https://chromiumcodereview.appspot.com/10815079/diff/11001/chrome/browser/per...
File chrome/browser/performance_monitor/performance_monitor_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10815079/diff/11001/chrome/browser/per...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:212: //
The default profile directory can be changed on CrOS if we are logged in
On 2012/07/30 09:01:02, Yoyo Zhou wrote:
> What does 'can be changed' mean?

I hope this is a bit more descriptive? The entire situation is a bit hazy, and
difficult to describe in a short comment...

https://chromiumcodereview.appspot.com/10815079/diff/11001/chrome/browser/per...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:217: if
(command_line.HasSwitch(switches::kLoginProfile)) {
On 2012/07/30 09:01:02, Yoyo Zhou wrote:
> What's the default setup for this switch on the ChromeOS bots?

Unfortunately, I couldn't find anything saying which switches were enabled on
the trybot runs - but I'm fairly certain trybots run with something like
--login-profile=user. This is why these tests failed on CrOS before - the tests
only used "Default" before. By sending this through the trybots with various
logs inserted (ugly, but effective), and using
ProfileManager::GetInitialProfileDir (would link to code, but codesearch is
refusing to open the file), I found that the CrOS trybots have a profile name of
"user" and do enter these if-statements.

Also, a side-note - if this configuration changes, then the result is just that
it won't enter this if statement, and will use "Default" again - so it won't
break anything (hopefully).

Does this make sense? If not, I can try to clear it up more in an email (with
trybot logs, etc).

https://chromiumcodereview.appspot.com/10815079/diff/11001/chrome/browser/per...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:258:
std::string first_profile_name() { return first_profile_name_; }
On 2012/07/30 09:01:02, Yoyo Zhou wrote:
> nit: it doesn't look like these accessors are necessary.

Whoops, sorry - habit to have accessors instead of use members directly in
tests. Done.

https://chromiumcodereview.appspot.com/10815079/diff/11001/chrome/browser/per...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:498: const
size_t kNumEvents = 1;
On 2012/07/30 09:01:02, Yoyo Zhou wrote:
> Define this closer to where it's used.

Done.

https://chromiumcodereview.appspot.com/10815079/diff/11001/chrome/browser/per...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:523: const
size_t kNumEvents = 2;
On 2012/07/30 09:01:02, Yoyo Zhou wrote:
> ditto

Done.

[Yoyo Zhou, 2012-07-30 20:01:04]

LGTM. I wish the ChromeOS folks would document this login profile stuff a little
better.

[commit-bot: I haz the power, 2012-07-31 17:44:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/10815079/1...

[commit-bot: I haz the power, 2012-07-31 17:44:38]

Presubmit check for 10815079-14006 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.2s to calculate.

[Devlin, 2012-07-31 17:51:35]

On 2012/07/31 17:44:38, I haz the power (commit-bot) wrote:
> Presubmit check for 10815079-14006 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
>     chrome
> 
> Presubmit checks took 1.2s to calculate.

Hmm...
Here's my understanding: c/b/performance_monitor has no OWNERS file, so even
though I'm not modifying stuff in chrome/ directly (unless it counts
c/test/data/), it is defaulting to that for an OWNER. I know we can TBR for
simple gypi changes, but this doesn't really fall into that category... Should
we add an OWNERS to c/b/performance_monitor?

[Yoyo Zhou, 2012-07-31 21:26:56]

On 2012/07/31 17:51:35, D Cronin wrote:
> On 2012/07/31 17:44:38, I haz the power (commit-bot) wrote:
> > Presubmit check for 10815079-14006 failed and returned exit status 1.
> > 
> > Running presubmit commit checks ...
> > 
> > ** Presubmit ERRORS **
> > Missing LGTM from an OWNER for files in these directories:
> >     chrome
> > 
> > Presubmit checks took 1.2s to calculate.
> 
> Hmm...
> Here's my understanding: c/b/performance_monitor has no OWNERS file, so even
> though I'm not modifying stuff in chrome/ directly (unless it counts
> c/test/data/), it is defaulting to that for an OWNER. I know we can TBR for
> simple gypi changes, but this doesn't really fall into that category... Should
> we add an OWNERS to c/b/performance_monitor?

Yes, we should. I hear there are some new committers who could go in that file.
=)

[Devlin, 2012-07-31 22:39:39]

On 2012/07/31 21:26:56, Yoyo Zhou wrote:
> On 2012/07/31 17:51:35, D Cronin wrote:
> > On 2012/07/31 17:44:38, I haz the power (commit-bot) wrote:
> > > Presubmit check for 10815079-14006 failed and returned exit status 1.
> > > 
> > > Running presubmit commit checks ...
> > > 
> > > ** Presubmit ERRORS **
> > > Missing LGTM from an OWNER for files in these directories:
> > >     chrome
> > > 
> > > Presubmit checks took 1.2s to calculate.
> > 
> > Hmm...
> > Here's my understanding: c/b/performance_monitor has no OWNERS file, so even
> > though I'm not modifying stuff in chrome/ directly (unless it counts
> > c/test/data/), it is defaulting to that for an OWNER. I know we can TBR for
> > simple gypi changes, but this doesn't really fall into that category...
Should
> > we add an OWNERS to c/b/performance_monitor?
> 
> Yes, we should. I hear there are some new committers who could go in that
file.
> =)

Haha thanks - I'm hoping I'm in that group :) OWNERS file rubberstamp, please?

[Yoyo Zhou, 2012-07-31 22:57:59]

On 2012/07/31 22:39:39, D Cronin wrote:
> On 2012/07/31 21:26:56, Yoyo Zhou wrote:
> > On 2012/07/31 17:51:35, D Cronin wrote:
> > > On 2012/07/31 17:44:38, I haz the power (commit-bot) wrote:
> > > > Presubmit check for 10815079-14006 failed and returned exit status 1.
> > > > 
> > > > Running presubmit commit checks ...
> > > > 
> > > > ** Presubmit ERRORS **
> > > > Missing LGTM from an OWNER for files in these directories:
> > > >     chrome
> > > > 
> > > > Presubmit checks took 1.2s to calculate.
> > > 
> > > Hmm...
> > > Here's my understanding: c/b/performance_monitor has no OWNERS file, so
even
> > > though I'm not modifying stuff in chrome/ directly (unless it counts
> > > c/test/data/), it is defaulting to that for an OWNER. I know we can TBR
for
> > > simple gypi changes, but this doesn't really fall into that category...
> Should
> > > we add an OWNERS to c/b/performance_monitor?
> > 
> > Yes, we should. I hear there are some new committers who could go in that
> file.
> > =)
> 
> Haha thanks - I'm hoping I'm in that group :) OWNERS file rubberstamp, please?

LGTM but you should get a real OWNER to approve the OWNERS file.

[Aaron Boodman, 2012-08-01 09:09:41]

Yeah, I think it is good for Devlin to be an OWNER for this directory. You still
need to get an owner of the next level up to LGTM this last change.

[Devlin, 2012-08-01 15:29:35]

+jam for OWNERS file addition.

[jam, 2012-08-01 18:11:28]

On 2012/08/01 15:29:35, D Cronin wrote:
> +jam for OWNERS file addition.

1: why isn't this cc'd to chromium-reviews@chromium.org?
2: i dont know any of this code. can you find another reviewer in chrome/owners
who isn't as bogged down as me?

[Devlin, 2012-08-01 18:27:32]

-jam, +brettw for OWNERS file approval

[Devlin, 2012-08-02 16:10:06]

-brettw
OWNERS file addition moved to its own CL here:
https://chromiumcodereview.appspot.com/10825156 (stamped by thakis)

[commit-bot: I haz the power, 2012-08-02 18:08:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/10815079/2...

[commit-bot: I haz the power, 2012-08-02 21:15:09]

Change committed as 149697