[ChromeDriver] Subscribe PerformanceLogger to CommandListener interface

[ChromeDriver] Subscribe PerformanceLogger to CommandListener interface

BUG=

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

[johnmoore, 2014-06-26 22:58:52]

Hi all,

This is a preview of the second of four proposed CLs for ChromeDriver tracing
support. It contains a merge of the first CL since that one hasn't been
submitted yet. This should provide a better idea of where we're headed with the
CommandListener interface.

[stgao, 2014-06-27 16:48:25]

Hi John,

Overall, the CL looks quite good.
Let's meet and discuss pending issues if there are some.

Thanks,
Shuotao Gao

https://codereview.chromium.org/353063005/diff/1/chrome/test/chromedriver/com...
File chrome/test/chromedriver/command_listener_proxy.cc (right):

https://codereview.chromium.org/353063005/diff/1/chrome/test/chromedriver/com...
chrome/test/chromedriver/command_listener_proxy.cc:18: CHECK(command_listener_);
It seems this CHECK could be removed, as we already have one in the constructor.

https://codereview.chromium.org/353063005/diff/1/chrome/test/chromedriver/log...
File chrome/test/chromedriver/logging.cc (right):

https://codereview.chromium.org/353063005/diff/1/chrome/test/chromedriver/log...
chrome/test/chromedriver/logging.cc:262: // session->chrome will own perf_log,
and session will own proxy.
The proxy is a good idea.
Nice work!

https://codereview.chromium.org/353063005/diff/1/chrome/test/chromedriver/log...
chrome/test/chromedriver/logging.cc:263: // session->command_listeners (the
proxy) will be destroyed first.
Why it is destroyed first? What happens if it is not?

https://codereview.chromium.org/353063005/diff/1/chrome/test/chromedriver/ses...
File chrome/test/chromedriver/session.h (right):

https://codereview.chromium.org/353063005/diff/1/chrome/test/chromedriver/ses...
chrome/test/chromedriver/session.h:82: // command_listeners should be declared
after chrome. When the Session is
When referring a variable or a class, usually we wrap it like below:
|command_listerners|.

Please also check other files for such reference.

https://codereview.chromium.org/353063005/diff/1/chrome/test/chromedriver/ses...
File chrome/test/chromedriver/session_commands.cc (right):

https://codereview.chromium.org/353063005/diff/1/chrome/test/chromedriver/ses...
chrome/test/chromedriver/session_commands.cc:130: std::list<CommandListener*>
command_listeners;
Is it possible to use ScopedVector here and assign it to the one in the Session
object instead of adding one by one?

That will make it easy to ensure there is no leak.

https://codereview.chromium.org/353063005/diff/1/chrome/test/chromedriver/uti...
File chrome/test/chromedriver/util.cc (right):

https://codereview.chromium.org/353063005/diff/1/chrome/test/chromedriver/uti...
chrome/test/chromedriver/util.cc:410: session->command_listeners.begin();
indent.

https://codereview.chromium.org/353063005/diff/1/chrome/test/chromedriver/util.h
File chrome/test/chromedriver/util.h (right):

https://codereview.chromium.org/353063005/diff/1/chrome/test/chromedriver/uti...
chrome/test/chromedriver/util.h:10: #include
"chrome/test/chromedriver/session.h"
declare it like below instead of a direct include.

[johnmoore, 2014-06-27 21:58:04]

Hi Shuotao,

Thanks for meeting with me today. I've addressed your comments on this CL, which
includes the first CL for tracing support as well
(https://codereview.chromium.org/331943003). If you have any other concerns
after our discussion today, just shoot me an email. Thanks!

https://codereview.chromium.org/353063005/diff/1/chrome/test/chromedriver/com...
File chrome/test/chromedriver/command_listener_proxy.cc (right):

https://codereview.chromium.org/353063005/diff/1/chrome/test/chromedriver/com...
chrome/test/chromedriver/command_listener_proxy.cc:18: CHECK(command_listener_);
On 2014/06/27 16:48:24, Shuotao wrote:
> It seems this CHECK could be removed, as we already have one in the
constructor.

Done.

https://codereview.chromium.org/353063005/diff/1/chrome/test/chromedriver/log...
File chrome/test/chromedriver/logging.cc (right):

https://codereview.chromium.org/353063005/diff/1/chrome/test/chromedriver/log...
chrome/test/chromedriver/logging.cc:263: // session->command_listeners (the
proxy) will be destroyed first.
On 2014/06/27 16:48:24, Shuotao wrote:
> Why it is destroyed first? What happens if it is not?

In session.h, |command_listeners| is declared after |chrome|. So when a
|Session| is destroyed, |command_listeners| is destroyed first, as per the C++
spec (members are destroyed in the reverse order of their construction). It will
always be destroyed first so long as the order of declaration isn't changed in
session.h. I placed a comment in session.h noting the importance of the order.

https://codereview.chromium.org/353063005/diff/1/chrome/test/chromedriver/ses...
File chrome/test/chromedriver/session.h (right):

https://codereview.chromium.org/353063005/diff/1/chrome/test/chromedriver/ses...
chrome/test/chromedriver/session.h:82: // command_listeners should be declared
after chrome. When the Session is
On 2014/06/27 16:48:25, Shuotao wrote:
> When referring a variable or a class, usually we wrap it like below:
> |command_listerners|.
> 
> Please also check other files for such reference.

Done.

https://codereview.chromium.org/353063005/diff/1/chrome/test/chromedriver/ses...
File chrome/test/chromedriver/session_commands.cc (right):

https://codereview.chromium.org/353063005/diff/1/chrome/test/chromedriver/ses...
chrome/test/chromedriver/session_commands.cc:130: std::list<CommandListener*>
command_listeners;
On 2014/06/27 16:48:25, Shuotao wrote:
> Is it possible to use ScopedVector here and assign it to the one in the
Session
> object instead of adding one by one?
> 
> That will make it easy to ensure there is no leak.

Done.

https://codereview.chromium.org/353063005/diff/1/chrome/test/chromedriver/uti...
File chrome/test/chromedriver/util.cc (right):

https://codereview.chromium.org/353063005/diff/1/chrome/test/chromedriver/uti...
chrome/test/chromedriver/util.cc:410: session->command_listeners.begin();
On 2014/06/27 16:48:25, Shuotao wrote:
> indent.

Done.

https://codereview.chromium.org/353063005/diff/1/chrome/test/chromedriver/util.h
File chrome/test/chromedriver/util.h (right):

https://codereview.chromium.org/353063005/diff/1/chrome/test/chromedriver/uti...
chrome/test/chromedriver/util.h:10: #include
"chrome/test/chromedriver/session.h"
On 2014/06/27 16:48:25, Shuotao wrote:
> declare it like below instead of a direct include.

Done.

[stgao, 2014-07-01 19:02:27]

Hi John,

Thanks for the new patch.
I typed in some more comments on styles and the like.

Thanks,
Shuotao Gao

https://codereview.chromium.org/353063005/diff/40001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/353063005/diff/40001/chrome/chrome_tests.gypi...
chrome/chrome_tests.gypi:622: 'test/chromedriver/command_listener.h',
Seems like command_listener.h should go first as web_view.h

https://codereview.chromium.org/353063005/diff/40001/chrome/chrome_tests.gypi...
chrome/chrome_tests.gypi:640: 'test/chromedriver/performance_logger.cc',
It seems the original order was wrong.
Would you mind correcting it here?

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
File chrome/test/chromedriver/command_listener_proxy.h (right):

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/command_listener_proxy.h:18: // command_listener must
not be null.
command_listener  ->  |command_listener|

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/command_listener_proxy.h:21: // Forwards commands to
command_listener.
command_listener seems unclear to me: command_listener from constructor or
command_listener_?

Maybe just say "CommandListener"?

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/command_listener_proxy.h:22: virtual Status
BeforeCommand(const std::string& command_name);
OVERRIDE

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
File chrome/test/chromedriver/commands_unittest.cc (right):

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/commands_unittest.cc:552: class MockCommandListener :
public CommandListener {
duplicate code, how about extracting it out as a command_listener_stub.cc/h as I
suggested last time?

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/commands_unittest.cc:585: Status ExecuteTestListener(
The name is not that clear for setting session->quit = true.

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/commands_unittest.cc:598: const std::string&
session_id) {
Seems some parameters are not used. Could they be removed?

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/commands_unittest.cc:630: MockCommandListener* listener
= new MockCommandListener();
As we have a reference to this |listener|, we could verify whether it is called
or not here before and after an execution of cmd.

Could the test be simplified a little bit? Or I misunderstand something here?

If a function above is used as a command, please postfix it with "Command" like
"abcCommand".

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
File chrome/test/chromedriver/logging.cc (right):

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/logging.cc:8: #include <list>
Unused.

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
File chrome/test/chromedriver/logging.h (right):

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/logging.h:8: #include <list>
Seems unused now.

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
File chrome/test/chromedriver/session.h (right):

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/session.h:53: void AddListener(CommandListener*
listener);
Seems unused now.
Could be removed.

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
File chrome/test/chromedriver/session_unittest.cc (right):

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/session_unittest.cc:36: class MockCommandListener :
public CommandListener {
Duplicate code again.
Now it seems that it should be in a shared class as I mentioned above.

[johnmoore, 2014-07-02 22:17:45]

Hi Shuotao,

I have made most of the edits you suggested. The remaining issues are in
commands_unittest.cc. Please take a look at my replies to your comments there
and let me know your thoughts when you have a chance. Thanks!

https://codereview.chromium.org/353063005/diff/40001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/353063005/diff/40001/chrome/chrome_tests.gypi...
chrome/chrome_tests.gypi:622: 'test/chromedriver/command_listener.h',
On 2014/07/01 19:02:26, Shuotao wrote:
> Seems like command_listener.h should go first as web_view.h

Done.

https://codereview.chromium.org/353063005/diff/40001/chrome/chrome_tests.gypi...
chrome/chrome_tests.gypi:640: 'test/chromedriver/performance_logger.cc',
On 2014/07/01 19:02:26, Shuotao wrote:
> It seems the original order was wrong.
> Would you mind correcting it here?

Done.

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
File chrome/test/chromedriver/command_listener_proxy.h (right):

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/command_listener_proxy.h:18: // command_listener must
not be null.
On 2014/07/01 19:02:26, Shuotao wrote:
> command_listener  ->  |command_listener|

Done.

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/command_listener_proxy.h:21: // Forwards commands to
command_listener.
On 2014/07/01 19:02:27, Shuotao wrote:
> command_listener seems unclear to me: command_listener from constructor or
> command_listener_?
> 
> Maybe just say "CommandListener"?

Done.

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/command_listener_proxy.h:22: virtual Status
BeforeCommand(const std::string& command_name);
On 2014/07/01 19:02:27, Shuotao wrote:
> OVERRIDE

Done.

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
File chrome/test/chromedriver/commands_unittest.cc (right):

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/commands_unittest.cc:552: class MockCommandListener :
public CommandListener {
On 2014/07/01 19:02:27, Shuotao wrote:
> duplicate code, how about extracting it out as a command_listener_stub.cc/h as
I
> suggested last time?

I have removed session->AddListener, so the duplicate code no longer exists.

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/commands_unittest.cc:585: Status ExecuteTestListener(
On 2014/07/01 19:02:27, Shuotao wrote:
> The name is not that clear for setting session->quit = true.

I have updated the command and callback names to match the style of the rest of
the tests.

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/commands_unittest.cc:598: const std::string&
session_id) {
On 2014/07/01 19:02:27, Shuotao wrote:
> Seems some parameters are not used. Could they be removed?

They could be removed if I re-implemented ExecuteSessionCommand in my test
method, such that it didn't pass the extra values. I don't like that idea since
it kind of defeats the purpose of the test, which is to mimic actual program
execution as closely as possible. Alternatively I could use the extra parameters
(i.e. perform checks on them), but they really aren't part of my test, so if
they somehow cause the test to fail, it won't be as clear why this particular
test is failing. Neither solution seems ideal, but let me know if you'd still
like me to address this issue.

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/commands_unittest.cc:630: MockCommandListener* listener
= new MockCommandListener();
On 2014/07/01 19:02:27, Shuotao wrote:
> As we have a reference to this |listener|, we could verify whether it is
called
> or not here before and after an execution of cmd.
> 
> Could the test be simplified a little bit? Or I misunderstand something here?
> 
> If a function above is used as a command, please postfix it with "Command"
like
> "abcCommand".

I have fixed the command and callback method names.

Regarding your other question:

Before the first command is executed, the listener has not been added to
|session|, and cannot be added to it because we do not have access to |session|
directly from the test. 

So, we must add the listener to |session| within the command itself, which has
access to |session|.

Recall that BeforeCommand is called before a command is executed. So when we
execute the command to add the listener to |session|, |session|'s listeners
(there aren't any of them yet) have already been notified of BeforeCommand.

So now we need to execute a second command so that the listener we just added in
the previous command can be notified of BeforeCommand. This second command is
basically a no-op, and just quits the session as a formality. Then in the
callback for the second command, we make sure the listener was notified.

I could try to re-write the test so that we had access to |session| from within
the test method, but I think that would defeat the purpose of the test.

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
File chrome/test/chromedriver/logging.cc (right):

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/logging.cc:8: #include <list>
On 2014/07/01 19:02:27, Shuotao wrote:
> Unused.

Done.

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
File chrome/test/chromedriver/logging.h (right):

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/logging.h:8: #include <list>
On 2014/07/01 19:02:27, Shuotao wrote:
> Seems unused now.

Done.

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
File chrome/test/chromedriver/session.h (right):

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/session.h:53: void AddListener(CommandListener*
listener);
On 2014/07/01 19:02:27, Shuotao wrote:
> Seems unused now.
> Could be removed.

Done.

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
File chrome/test/chromedriver/session_unittest.cc (right):

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/session_unittest.cc:36: class MockCommandListener :
public CommandListener {
On 2014/07/01 19:02:27, Shuotao wrote:
> Duplicate code again.
> Now it seems that it should be in a shared class as I mentioned above.

I have removed AddListener from session, so this duplicate code is no longer
needed.

[stgao, 2014-07-11 21:36:07]

lgtm with a nit on MockCommandListener.

It could be addressed in a separate CL if you think it makes sense.

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
File chrome/test/chromedriver/commands_unittest.cc (right):

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/commands_unittest.cc:552: class MockCommandListener :
public CommandListener {
On 2014/07/02 22:17:44, johnmoore wrote:
> On 2014/07/01 19:02:27, Shuotao wrote:
> > duplicate code, how about extracting it out as a command_listener_stub.cc/h
as
> I
> > suggested last time?
> 
> I have removed session->AddListener, so the duplicate code no longer exists.

It seems there is another MockCommandListener in
chrome/test/chromedriver/command_listener_proxy_unittest.cc

It would be better if it could be extracted into separate
command_listener_stub.cc/h files.

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/commands_unittest.cc:598: const std::string&
session_id) {
On 2014/07/02 22:17:44, johnmoore wrote:
> On 2014/07/01 19:02:27, Shuotao wrote:
> > Seems some parameters are not used. Could they be removed?
> 
> They could be removed if I re-implemented ExecuteSessionCommand in my test
> method, such that it didn't pass the extra values. I don't like that idea
since
> it kind of defeats the purpose of the test, which is to mimic actual program
> execution as closely as possible. Alternatively I could use the extra
parameters
> (i.e. perform checks on them), but they really aren't part of my test, so if
> they somehow cause the test to fail, it won't be as clear why this particular
> test is failing. Neither solution seems ideal, but let me know if you'd still
> like me to address this issue.

OK. That's fine.

https://codereview.chromium.org/353063005/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/commands_unittest.cc:630: MockCommandListener* listener
= new MockCommandListener();
On 2014/07/02 22:17:44, johnmoore wrote:
> On 2014/07/01 19:02:27, Shuotao wrote:
> > As we have a reference to this |listener|, we could verify whether it is
> called
> > or not here before and after an execution of cmd.
> > 
> > Could the test be simplified a little bit? Or I misunderstand something
here?
> > 
> > If a function above is used as a command, please postfix it with "Command"
> like
> > "abcCommand".
> 
> I have fixed the command and callback method names.
> 
> Regarding your other question:
> 
> Before the first command is executed, the listener has not been added to
> |session|, and cannot be added to it because we do not have access to
|session|
> directly from the test. 
> 
> So, we must add the listener to |session| within the command itself, which has
> access to |session|.
> 
> Recall that BeforeCommand is called before a command is executed. So when we
> execute the command to add the listener to |session|, |session|'s listeners
> (there aren't any of them yet) have already been notified of BeforeCommand.
> 
> So now we need to execute a second command so that the listener we just added
in
> the previous command can be notified of BeforeCommand. This second command is
> basically a no-op, and just quits the session as a formality. Then in the
> callback for the second command, we make sure the listener was notified.
> 
> I could try to re-write the test so that we had access to |session| from
within
> the test method, but I think that would defeat the purpose of the test.

You are right. We can't do something like ErrorFindChildElement here.
Thanks for the detailed explanation.

[johnmoore, 2014-07-14 14:52:08]

The CQ bit was checked by johnmoore@google.com

[commit-bot: I haz the power, 2014-07-14 14:53:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/johnmoore@google.com/353063005/50001

[commit-bot: I haz the power, 2014-07-14 16:04:02]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-07-14 16:10:03]

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

[commit-bot: I haz the power, 2014-07-14 16:10:04]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[johnmoore, 2014-07-14 16:29:22]

The CQ bit was checked by johnmoore@google.com

[commit-bot: I haz the power, 2014-07-14 16:30:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/johnmoore@google.com/353063005/70001

[commit-bot: I haz the power, 2014-07-14 19:57:43]

Change committed as 283003

[johnmoore, 2014-07-15 00:45:41]

An ASAN trybot caught a use-after-free memory error in a unit test after I
committed the CL. I have fixed the error and used ASAN locally to verify the
fix. Hopefully the Chromium ASAN trybots will run overnight--I have manually
initiated them. If all clears, we will need to re-submit this CL since it was
reverted in a separate CL due to the ASAN failure. I'm not sure whether I will
need to upload a new CL entirely in order to submit again.

The error was in the tricky unittest that Shuotao had expressed concern over. I
have taken a close look at the offending test and have simplified it as much as
I believe is possible, in addition to fixing the use-after-free error.

[stgao, 2014-07-16 17:39:42]

This CL could be reused, and you don't have to upload a new CL.

But I would let samuong@ to take another look at the change.

[johnmoore, 2014-07-16 17:57:24]

SGTM. Sam, please note that the CL now passes the linux_asan trybot, and
chromedriver_unittests passes the mac_asan trybot. So the CL should not break
the trybots again. But please have a look at commands_unittest and let me know
if you have any comments.

A summary of what happened and why it now works: setting session->quit to true
deletes the session, which owns the MockCommandListener we are testing with. So
trying to call listener->VerifyCalled for the MockCommandListener in the old
ExecuteQuitSessionCommand callback was a use-after-free error since the session
has been deleted by the time the command callback executes. Now, instead of
giving session the actual MockCommandListener, I give it a CommandListenerProxy.
So the MockCommandListener gets notified via the proxy, the session object gets
freed (along with the proxy), and then we can still call VerifyCalled on the
MockCommandListener. The MockCommandListener is freed by a scoped_ptr at the end
of the test. So there shouldn't be any memory leaks or use-after-free errors. I
have also moved the calls to VerifyCalled and VerifyNotCalled out of the command
callbacks and into the body of the test as Shuotao suggested in a previous
comment. For an explanation of why we need to execute two commands on the
session for this test instead of one, see my previous comment on the CL.

Thanks!

[samuong, 2014-07-16 21:25:29]

On 2014/07/16 17:57:24, johnmoore wrote:
> SGTM. Sam, please note that the CL now passes the linux_asan trybot, and
> chromedriver_unittests passes the mac_asan trybot. So the CL should not break
> the trybots again. But please have a look at commands_unittest and let me know
> if you have any comments.
> 
> A summary of what happened and why it now works: setting session->quit to true
> deletes the session, which owns the MockCommandListener we are testing with.
So
> trying to call listener->VerifyCalled for the MockCommandListener in the old
> ExecuteQuitSessionCommand callback was a use-after-free error since the
session
> has been deleted by the time the command callback executes. Now, instead of
> giving session the actual MockCommandListener, I give it a
CommandListenerProxy.
> So the MockCommandListener gets notified via the proxy, the session object
gets
> freed (along with the proxy), and then we can still call VerifyCalled on the
> MockCommandListener. The MockCommandListener is freed by a scoped_ptr at the
end
> of the test. So there shouldn't be any memory leaks or use-after-free errors.
I
> have also moved the calls to VerifyCalled and VerifyNotCalled out of the
command
> callbacks and into the body of the test as Shuotao suggested in a previous
> comment. For an explanation of why we need to execute two commands on the
> session for this test instead of one, see my previous comment on the CL.
> 
> Thanks!

LGTM for the last set of changes to fix the use-after-free issue in the test.
Since you've already gotten an LGTM from Shuotao for the rest of it, I'm happy
for you to commit this now. I'll take a closer look at the rest of the code
separately and let you know if I see any issues, and we can address these in a
separate CL.

[johnmoore, 2014-07-17 16:27:13]

The CQ bit was checked by johnmoore@google.com

[commit-bot: I haz the power, 2014-07-17 16:28:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/johnmoore@google.com/353063005/110001

[commit-bot: I haz the power, 2014-07-17 19:18:13]

Change committed as 283835