Fixed browser crash when using Debug build:

Fixed browser crash when Telemetry uses Debug build.

Factored out chrome.benchmarking methods which don't depend on stats table.


BUG=168640
TEST=tools/telemetry/run_tests --browser=debug PageRunnerTests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182754

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=183611

[Danh Nguyen, 2013-01-07 21:20:17]

Hi Dave, Nat,

This attempts to fix the crash on debug build which I've separated it out from
previous CL.  Please take a look.

I'm not sure whether I need to add unit test for this change.

Thanks,
Danh

[nduca, 2013-01-07 23:58:45]

The right fix for this is to fix crbug.com/163534 --- we need a specific
function from benchmarkng_extension.cc --- take that function and move it a
network_benchmarking_extension.cc --- then remove the actual
--enable-benchmark-mode code from telemetry.

[Danh Nguyen, 2013-01-23 22:51:08]

Hi Nat,

This is my attempt to fix this issue per your suggestion.  I'm pretty ignorant
on the subject so the solution might be naive or incorrect.  Please take a look
and help me out.  I'd appreciate that very much.

Thanks,
Danh

[nduca, 2013-01-24 01:49:52]

Can you see if @tonyg can do a first pass review on this?

[Danh Nguyen, 2013-01-24 14:47:37]

Hi Tony,

I'm attempting to fix the telemetry bug which crashes the debug build following
Nat's tips.  Nat suggested that I should have you as the first pass reviewer for
this change.  Could you help to review it?

Thanks so much,
Danh

[tonyg, 2013-01-24 22:00:29]

Perhaps Nat and I should sync up on this before you go changing the patch
around. But I'd opt for a simpler approach: just pass --enable-stats-table for
debug builds and leave everything else as-is.

https://codereview.chromium.org/11801024/diff/7001/chrome/renderer/benchmarki...
File chrome/renderer/benchmarking_extension.cc (right):

https://codereview.chromium.org/11801024/diff/7001/chrome/renderer/benchmarki...
chrome/renderer/benchmarking_extension.cc:32: "chrome.benchmarking.clearCache =
function() {"
There are other useful methods in the benchmarking extension like clearCache
that I suspect we may want to use in telemetry in the future.

https://codereview.chromium.org/11801024/diff/7001/chrome/renderer/chrome_con...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/11801024/diff/7001/chrome/renderer/chrome_con...
chrome/renderer/chrome_content_renderer_client.cc:216:
thread->RegisterExtension(extensions_v8::NetBenchmarkingExtension::Get());
This exposes closeConnection() to the web platform by default. I don't think we
want to do that.

https://codereview.chromium.org/11801024/diff/7001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/desktop_browser_backend.py (left):

https://codereview.chromium.org/11801024/diff/7001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/desktop_browser_backend.py:51:
args.append('--enable-benchmarking')
We run telemetry on older builds called reference builds. I believe this will
still be necessary for running on those.

[Danh Nguyen, 2013-01-24 22:16:30]

On 2013/01/24 22:00:29, tonyg wrote:
> Perhaps Nat and I should sync up on this before you go changing the patch
> around. But I'd opt for a simpler approach: just pass --enable-stats-table for
> debug builds and leave everything else as-is.
>

Thanks, Tony.  Please let me know when you and Nat come to the decision of how
to solve this.

Danh

[nduca, 2013-02-06 07:40:06]

this approach looks promising. @dtu for final review. Will probably want to see
how trybots feel about this.

https://codereview.chromium.org/11801024/diff/7001/chrome/renderer/benchmarki...
File chrome/renderer/benchmarking_extension.cc (left):

https://codereview.chromium.org/11801024/diff/7001/chrome/renderer/benchmarki...
chrome/renderer/benchmarking_extension.cc:37: "  native function
ClearHostResolverCache();"
we should probably move all network related stuff to this new place.

[Danh Nguyen, 2013-02-07 16:56:32]

Hi All,

Per Tony's suggestions, I've moved all network-related stuff to the new
extension and also added a flag to optionally enable it.  Could you please take
another look?

Thanks,
Danh

https://codereview.chromium.org/11801024/diff/7001/chrome/renderer/benchmarki...
File chrome/renderer/benchmarking_extension.cc (left):

https://codereview.chromium.org/11801024/diff/7001/chrome/renderer/benchmarki...
chrome/renderer/benchmarking_extension.cc:37: "  native function
ClearHostResolverCache();"
On 2013/02/06 07:40:06, nduca wrote:
> we should probably move all network related stuff to this new place.

Done.

https://codereview.chromium.org/11801024/diff/7001/chrome/renderer/benchmarki...
File chrome/renderer/benchmarking_extension.cc (right):

https://codereview.chromium.org/11801024/diff/7001/chrome/renderer/benchmarki...
chrome/renderer/benchmarking_extension.cc:32: "chrome.benchmarking.clearCache =
function() {"
On 2013/01/24 22:00:29, tonyg wrote:
> There are other useful methods in the benchmarking extension like clearCache
> that I suspect we may want to use in telemetry in the future.

Done.

https://codereview.chromium.org/11801024/diff/7001/chrome/renderer/chrome_con...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/11801024/diff/7001/chrome/renderer/chrome_con...
chrome/renderer/chrome_content_renderer_client.cc:216:
thread->RegisterExtension(extensions_v8::NetBenchmarkingExtension::Get());
On 2013/01/24 22:00:29, tonyg wrote:
> This exposes closeConnection() to the web platform by default. I don't think
we
> want to do that.

Fixed.  I've added --enable-net-benchmarking for that purpose.

https://codereview.chromium.org/11801024/diff/7001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/desktop_browser_backend.py (left):

https://codereview.chromium.org/11801024/diff/7001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/desktop_browser_backend.py:51:
args.append('--enable-benchmarking')
On 2013/01/24 22:00:29, tonyg wrote:
> We run telemetry on older builds called reference builds. I believe this will
> still be necessary for running on those.

Done. Added code to detect chrome branch number so that we could add relevant
flag.

[nduca, 2013-02-12 01:28:49]

lgtm

https://codereview.chromium.org/11801024/diff/15001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/desktop_browser_backend.py (right):

https://codereview.chromium.org/11801024/diff/15001/tools/telemetry/telemetry...
tools/telemetry/telemetry/desktop_browser_backend.py:52: if
self._chrome_branch_number >= 1404:
should this be conditional on content shell, since this is a browser-only
extension?

[Danh Nguyen, 2013-02-12 15:57:26]

Thanks Nat for your approval.

Tony, does the change look OK for you?  Could you take another look when you
have a chance?

Thanks,
Danh

https://codereview.chromium.org/11801024/diff/15001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/desktop_browser_backend.py (right):

https://codereview.chromium.org/11801024/diff/15001/tools/telemetry/telemetry...
tools/telemetry/telemetry/desktop_browser_backend.py:52: if
self._chrome_branch_number >= 1404:
On 2013/02/12 01:28:49, nduca wrote:
> should this be conditional on content shell, since this is a browser-only
> extension?

Done.  Made all chrome switches conditional on not being content shell.

[Danh Nguyen, 2013-02-13 15:01:14]

Add @brettw for owner review.

[brettw, 2013-02-15 00:50:20]

LGTM rubberstamp

[Danh Nguyen, 2013-02-15 14:37:27]

On 2013/02/15 00:50:20, brettw wrote:
> LGTM rubberstamp

Thanks, Brett.  I appreciate that very much.

Danh

[commit-bot: I haz the power, 2013-02-15 14:38:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danhn@chromium.org/11801024/20001

[commit-bot: I haz the power, 2013-02-15 17:57:23]

Change committed as 182754

[enne (OOO), 2013-02-15 21:05:32]

On 2013/02/15 17:57:23, I haz the power (commit-bot) wrote:
> Change committed as 182754

This crashes on Win7 perf bots due to the changes to desktop_browser_backend:
http://build.chromium.org/p/chromium.webkit/builders/Win7%20Perf/builds/4654/...

[Danh Nguyen, 2013-02-20 03:00:21]

Just uploaded a patch for desktop_browser_backend.py to fix the failure on
windows perf bot.

[nduca, 2013-02-20 03:02:40]

lgtm nicely done

[commit-bot: I haz the power, 2013-02-20 18:42:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danhn@chromium.org/11801024/42001

[commit-bot: I haz the power, 2013-02-20 20:41:42]

Change committed as 183611

[nduca, 2013-02-22 19:36:20]

lgtm