Telemetry: fixes testGotTrace on old browsers.

Telemetry: fixes testGotTrace on old browsers.

Browsers built prior to r176905 don't support the tracing mechanism used by telemetry.
Prevent the test from failing, since this is an expected scenario.

BUG=170284
TEST=tools/telemetry/run_tests --browser=android-content-shell testGotTrace -vv


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

[bulach, 2013-01-16 13:52:47]

ptal

[Sami, 2013-01-16 14:47:52]

lgtm. I was wondering if we need to close down the http server but looks like
the framework handles that for us.

https://codereview.chromium.org/11931015/diff/1/tools/telemetry/telemetry/tra...
File tools/telemetry/telemetry/tracing_backend_unittest.py (right):

https://codereview.chromium.org/11931015/diff/1/tools/telemetry/telemetry/tra...
tools/telemetry/telemetry/tracing_backend_unittest.py:5: import logging
Alpha order.

[bulach, 2013-01-16 15:05:49]

thanks sami! alpha-sorted..

https://codereview.chromium.org/11931015/diff/1/tools/telemetry/telemetry/tra...
File tools/telemetry/telemetry/tracing_backend_unittest.py (right):

https://codereview.chromium.org/11931015/diff/1/tools/telemetry/telemetry/tra...
tools/telemetry/telemetry/tracing_backend_unittest.py:5: import logging
On 2013/01/16 14:47:52, Sami wrote:
> Alpha order.

Done.

[nduca, 2013-01-16 20:21:23]

https://codereview.chromium.org/11931015/diff/2002/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/tracing_backend_unittest.py (right):

https://codereview.chromium.org/11931015/diff/2002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/tracing_backend_unittest.py:28: try:
better to check self._browser.supports_tracing instead of a try/catch ---
suppose StartTracing had an error, this'd mask it?

[bulach, 2013-01-17 09:28:50]

hmm... this exception is only ever thrown when trying to StartTracing against an
unsupported browser (TracingUnsupportedException).. 
other exceptions won't be be masked.
from the discussion on the previous patch, this seemed to be the best option?

if we want to have a "supports_tracing", we'd need to use version number, which
seemed an overkill... wdyt?

[nduca, 2013-01-17 09:31:59]

We do have a supports_tracing actually. And, your change should have made it
onto canary by now, so just make browser_backend check _chrome_branch_number
against 1385 and you're set. :)

[nduca, 2013-01-17 09:33:11]

But your point is also well made, I read this as a blanket catch, I totally
(idiotically) misread that you were checking for a specific exception. I"m sorry
about that. LGTm whatever you want to do.

[bulach, 2013-01-17 11:28:25]

gargh! :) my stupid memory, didn't recall we had "supports_tracing" already.. I
can add an assertion that 

so looking into, it seems that the version detection is actually broken in
android, so let me look dig into it.

(end goal being here is to assertFalse(supports_tracing) when this exception is
thrown..)

[bulach, 2013-01-17 12:12:36]

On 2013/01/17 11:28:25, bulach wrote:
> gargh! :) my stupid memory, didn't recall we had "supports_tracing" already..
I
> can add an assertion that 
> 
> so looking into, it seems that the version detection is actually broken in
> android, so let me look dig into it.
> 
> (end goal being here is to assertFalse(supports_tracing) when this exception
is
> thrown..)

"I'm feeling lucky" :)
now on android is crashing when trying to do IO on the UI thread when ending the
trace.... tracking this down:
I/DEBUG   (24882):     #02  base/debug/debugger_posix.cc:245
(base::debug::BreakDebugger())
I/DEBUG   (24882):     #03  base/logging.cc:664 (~LogMessage)
I/DEBUG   (24882):     #04  base/threading/thread_restrictions.cc:38
(base::ThreadRestrictions::AssertIOAllowed())
I/DEBUG   (24882):     #05  base/file_util_posix.cc:658
(file_util::OpenFile(FilePath const&, char const*))
I/DEBUG   (24882):     #06  base/file_util.cc:157
(file_util::ReadFileToString(FilePath const&, std::string*))
I/DEBUG   (24882):     #07  base/debug/trace_event_android.cc:99
(base::debug::TraceLog::AddClockSyncMetadataEvents())
I/DEBUG   (24882):     #08  base/debug/trace_event_impl.cc:576
(base::debug::TraceLog::SetDisabled())
I/DEBUG   (24882):     #09  content/browser/trace_controller_impl.cc:314
(content::TraceControllerImpl::OnEndTracingAck(std::vector<std::string,
std::allocator<std::string> > const&))
I/DEBUG   (24882):     #10  base/bind_internal.h:190
(base::internal::RunnableAdapter<void
(content::TraceControllerImpl::*)(std::vector<std::string,
std::allocator<std::string> > const&)>::Run(content::TraceControllerImpl*,
std::vector<std::string, std::allocator<std::string> > const&))
I/DEBUG   (24882):     #11  base/callback.h:396 (base::Callback<void ()>::Run()
const)
I/DEBUG   (24882):     #12  base/message_loop.cc:485
(MessageLoop::DeferOrRunPendingTask(base::PendingTask const&))
I/DEBUG   (24882):     #13  base/message_loop.cc:668 (MessageLoop::DoWork())
I/DEBUG   (24882):     #14  base/message_pump_android.cc:41 (DoRunLoopOnce)

[bulach, 2013-01-17 14:21:31]

nduca, sami: I'm not sure how this ever worked :-/ but here's the deal:

TraceControllerImpl::OnEndTracingAck happens on the UI thread.
it hits base::TraceLog::SetDisabled(), which in turn calls:
#if defined(OS_ANDROID)
  AddClockSyncMetadataEvents();
#endif

this function then reads from /sys/kernel/debug/tracing/trace_[clock|marker],
which then blows with base::ThreadRestrictions::AssertIOAllowed()...


afaict, we have a few options:
1. ThreadRestrictions::ScopedAllowIO, which the presubmit tells not to..
2. somehow change the API so that we can pass a FILE_THREAD from content all the
way down into base and run this function...
3. expose this function under ifdef android and require the (only) caller to
SetDisabled() to call it on its FILE_THREAD...

I can't really decide which one is less ugly, so would you have any better
suggestion?

[Sami, 2013-01-17 15:28:49]

Could we write the clock sync event from TraceLog::SendToATrace when the first
trace event is sent, or does it need to happen at the end of a trace?

Also, AddClockSyncMetadataEvents() talks about sched_setaffinity not being
supported. It's there on bionic but just not part of the NDK. Would it help if
we could use it?

[bulach, 2013-01-17 17:30:50]

+wangxianzhu, hopefully he can answer sami's questions from #11 above :)

[nduca, 2013-01-17 19:36:31]

Now that clank outputs into atrace, I think we can disable the writing of clock
sync metadata entirely at least for android. Put up a standalone patch that
disables, cc whoever did the trace_event->atrace redirect [git blame on
trace_event_impl] that and Ill happily lgtm it.

[bulach, 2013-01-25 22:04:28]

I think this blackhole is done:
android trace fixed by 
https://codereview.chromium.org/12025014/
and version detection by:
https://codereview.chromium.org/12042015/

so this finally can use "supports_tracing" as per nat's suggestion.

https://codereview.chromium.org/11931015/diff/2002/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/tracing_backend_unittest.py (right):

https://codereview.chromium.org/11931015/diff/2002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/tracing_backend_unittest.py:28: try:
On 2013/01/16 20:21:24, nduca wrote:
> better to check self._browser.supports_tracing instead of a try/catch ---
> suppose StartTracing had an error, this'd mask it?

good point, done!
(depends on https://codereview.chromium.org/12042015/ landing..)

[Sami, 2013-01-25 22:30:03]

lgtm

[nduca, 2013-01-26 00:25:22]

lgtm

[nduca, 2013-02-13 09:22:22]

is this dead?

[bulach, 2013-02-13 16:20:22]

argh! sorry, went under the radar... this is still valid, I'm CQing it..

however, the test mentioned in the description is failing for unrelated reasons
(looks like the trace parser / importer is not working), I'll look into
separately.

[commit-bot: I haz the power, 2013-02-13 16:22:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/11931015/12001

[commit-bot: I haz the power, 2013-02-13 16:22:05]

Failed to apply patch for tools/telemetry/telemetry/tracing_backend_unittest.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file tools/telemetry/telemetry/tracing_backend_unittest.py
  Hunk #1 succeeded at 4 with fuzz 2 (offset 1 line).
  Hunk #2 FAILED at 23.
  1 out of 2 hunks FAILED -- saving rejects to file
tools/telemetry/telemetry/tracing_backend_unittest.py.rej

Patch:       tools/telemetry/telemetry/tracing_backend_unittest.py
Index: tools/telemetry/telemetry/tracing_backend_unittest.py
diff --git a/tools/telemetry/telemetry/tracing_backend_unittest.py
b/tools/telemetry/telemetry/tracing_backend_unittest.py
index
030474701e57ccc6e0ad3ac4641143ae782742fd..225340051d2613bc39597f26601519d398a47c55
100644
--- a/tools/telemetry/telemetry/tracing_backend_unittest.py
+++ b/tools/telemetry/telemetry/tracing_backend_unittest.py
@@ -3,6 +3,7 @@
 # found in the LICENSE file.
 
 import json
+import logging
 import os
 
 from telemetry import tab_test_case
@@ -22,10 +23,12 @@ class TracingBackendTest(tab_test_case.TabTestCase):
     util.WaitFor(_IsDone, 5)
 
   def testGotTrace(self):
+    if not self._browser.supports_tracing:
+      logging.warning('Browser does not support tracing, skipping test.')
+      return
     self._StartServer()
     self._browser.StartTracing()
     self._browser.http_server.UrlOf('image.png')
-    self.assertTrue(self._browser.supports_tracing)
     self._browser.StopTracing()
     trace = self._browser.GetTrace()
     json_trace = json.loads(trace)

[commit-bot: I haz the power, 2013-02-13 18:19:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/11931015/21001

[commit-bot: I haz the power, 2013-02-13 19:28:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/11931015/21001

[commit-bot: I haz the power, 2013-02-13 22:10:11]

Change committed as 182317