The command line flag is now set during the test startup.

The command line flag is now set during the test startup.

These two tests replaced the global command line object, which might
cause heap-use-after-free of the temporary object at the end of the
test. For instance, ChromeExtensionsClient::GetWebstoreUpdateURL()
tries to read the command line flags, and does so asynchronously, not
in the main thread of the test.

The proposed patch eliminates use of the ScopedExperimentalCommandLine
at the test scope level, thus minimizing the risk of improper access to
memory.

BUG=
R=finnur@chromium.org

Committed: https://crrev.com/6ede907a3a8dfc095039b9fcd56478604bd2ae0a
Cr-Commit-Position: refs/heads/master@{#380096}

[Finnur, 2016-03-02 11:51:31]

Description was changed from

==========
The command line flag is now set during the test startup.

These two tests replaced the global command line object, which might cause
heap-use-after-free of the temporary object at the end of the test. For
instance, ChromeExtensionsClient::GetWebstoreUpdateURL() tries to read the
command line flags, and does so asynchronously, not in the main thread of the
test.

The proposed patch eliminates use of the ScopedExperimentalCommandLine at the
test scope level, thus minimizing the risk of improper access to memory.

BUG=
R=finnur@chromium.org
==========

to

==========
The command line flag is now set during the test startup.

These two tests replaced the global command line object, which might cause
heap-use-after-free of the temporary object at the end of the test. For
instance, ChromeExtensionsClient::GetWebstoreUpdateURL() tries to read the
command line flags, and does so asynchronously, not in the main thread of the
test.

The proposed patch eliminates use of the ScopedExperimentalCommandLine at the
test scope level, thus minimizing the risk of improper access to memory.

BUG=
R=finnur@chromium.org
==========

[Finnur, 2016-03-02 11:51:31]

finnur@chromium.org changed reviewers:
+ asargent@chromium.org
- finnur@chromium.org

[Finnur, 2016-03-02 11:51:54]

asargent@ is probably more appropriate for this.

[Finnur, 2016-03-02 11:52:09]

Also: Missing BUG= from CL description.

[Alexander Dunaev, 2016-03-02 12:12:08]

On 2016/03/02 11:52:09, Finnur wrote:
> Also: Missing BUG= from CL description.

I haven't found anything appropriate, should I file one?

[Finnur, 2016-03-02 13:13:52]

If there's a use-after-free issue, I presume there's an ASAN both report filed
or something, no?

In general, we probably won't bother with a bug if the issue is minor, but in
that case just a BUG=None is warranted.

[Finnur, 2016-03-02 13:14:13]

> If there's a use-after-free issue, I presume there's an ASAN both report filed

ASAN report, not ASAN both report.

[asargent_no_longer_on_chrome, 2016-03-02 18:18:10]

asargent@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[asargent_no_longer_on_chrome, 2016-03-02 18:18:10]

Hmm, in looking this over and thinking about it a little, I wonder if
ScopedExperimentalCommandLine is a dangerous construct that we should just get
rid of. I bet the problematic part is how it restores the previous command line
in its destructor - usually in tests we just append command lines early on and
never bother removing them again. 

Devlin, what do you think?

[Devlin, 2016-03-02 18:25:35]

On 2016/03/02 18:18:10, Antony Sargent wrote:
> Hmm, in looking this over and thinking about it a little, I wonder if
> ScopedExperimentalCommandLine is a dangerous construct that we should just get
> rid of. I bet the problematic part is how it restores the previous command
line
> in its destructor - usually in tests we just append command lines early on and
> never bother removing them again. 
> 
> Devlin, what do you think?

+1.  AFAIK, we long ago gave up on trying to have tests preserve the
commandline, and now we just reset it between tests.  I'm all for removing
ScopedExperimentalCommandLine in its entirety. :)

[asargent_no_longer_on_chrome, 2016-03-02 18:41:18]

Ok, sounds like we're agreed. 

voodoo@ - do you mind reworking this CL to just remove all instances of
ScopedExperimentalCommandLine?

Also, a small nit - can you reformat the CL description so that it wraps at 80
chars/line (actually 72 is even better). This makes it easier to read in 'git
log' output and emails.

[Alexander Dunaev, 2016-03-03 05:53:01]

On 2016/03/02 18:41:18, Antony Sargent wrote:
> Ok, sounds like we're agreed. 
> 
> voodoo@ - do you mind reworking this CL to just remove all instances of
> ScopedExperimentalCommandLine?
> 
> Also, a small nit - can you reformat the CL description so that it wraps at 80
> chars/line (actually 72 is even better). This makes it easier to read in 'git
> log' output and emails.

I don't mind reworking the CL, will provide an update soon.

Will reformat the description, too.

[Alexander Dunaev, 2016-03-03 05:58:33]

Description was changed from

==========
The command line flag is now set during the test startup.

These two tests replaced the global command line object, which might cause
heap-use-after-free of the temporary object at the end of the test. For
instance, ChromeExtensionsClient::GetWebstoreUpdateURL() tries to read the
command line flags, and does so asynchronously, not in the main thread of the
test.

The proposed patch eliminates use of the ScopedExperimentalCommandLine at the
test scope level, thus minimizing the risk of improper access to memory.

BUG=
R=finnur@chromium.org
==========

to

==========
The command line flag is now set during the test startup.

These two tests replaced the global command line object, which might
cause heap-use-after-free of the temporary object at the end of the
test. For instance, ChromeExtensionsClient::GetWebstoreUpdateURL()
tries to read the command line flags, and does so asynchronously, not
in the main thread of the test.

The proposed patch eliminates use of the ScopedExperimentalCommandLine
at the test scope level, thus minimizing the risk of improper access to
memory.

BUG=
R=finnur@chromium.org
==========

[Alexander Dunaev, 2016-03-03 08:19:49]

The patch is updated, the ScopedExperimentalCommandLine is removed.

Is there any way to make the server notifying the reviewers on CL update
automatically? Will git cl upload --send-mail do that?

[Alexander Dunaev, 2016-03-04 08:34:49]

asargent@, could you please take a look?

[asargent_no_longer_on_chrome, 2016-03-04 21:56:17]

lgtm

Regarding your question "Is there any way to make the server notifying the
reviewers on CL update automatically?", the upload flag you mention may work
that way, I'm not sure. If it does, I also don't know if that will do the
equivalent of "Publish+Mail Comments", which in practice almost always is needed
with any but the very first upload (there usually wouldn't be a need to upload a
new version and send mail about it if there weren't comments you were responding
to).

[Alexander Dunaev, 2016-03-09 03:55:36]

The CQ bit was checked by voodoo@yandex-team.ru

[commit-bot: I haz the power, 2016-03-09 03:55:49]

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

[commit-bot: I haz the power, 2016-03-09 05:51:31]

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

[commit-bot: I haz the power, 2016-03-09 05:51:32]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Alexander Dunaev, 2016-03-09 06:25:58]

The CQ bit was unchecked by voodoo@yandex-team.ru

[Alexander Dunaev, 2016-03-09 06:25:58]

The CQ bit was checked by voodoo@yandex-team.ru

[commit-bot: I haz the power, 2016-03-09 06:26:13]

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

[Alexander Dunaev, 2016-03-09 06:26:14]

The CQ bit was unchecked by voodoo@yandex-team.ru

[Alexander Dunaev, 2016-03-09 06:26:22]

The CQ bit was checked by voodoo@yandex-team.ru

[commit-bot: I haz the power, 2016-03-09 06:26:38]

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

[commit-bot: I haz the power, 2016-03-09 07:48:00]

Description was changed from

==========
The command line flag is now set during the test startup.

These two tests replaced the global command line object, which might
cause heap-use-after-free of the temporary object at the end of the
test. For instance, ChromeExtensionsClient::GetWebstoreUpdateURL()
tries to read the command line flags, and does so asynchronously, not
in the main thread of the test.

The proposed patch eliminates use of the ScopedExperimentalCommandLine
at the test scope level, thus minimizing the risk of improper access to
memory.

BUG=
R=finnur@chromium.org
==========

to

==========
The command line flag is now set during the test startup.

These two tests replaced the global command line object, which might
cause heap-use-after-free of the temporary object at the end of the
test. For instance, ChromeExtensionsClient::GetWebstoreUpdateURL()
tries to read the command line flags, and does so asynchronously, not
in the main thread of the test.

The proposed patch eliminates use of the ScopedExperimentalCommandLine
at the test scope level, thus minimizing the risk of improper access to
memory.

BUG=
R=finnur@chromium.org
==========

[commit-bot: I haz the power, 2016-03-09 07:48:01]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-09 07:48:59]

Description was changed from

==========
The command line flag is now set during the test startup.

These two tests replaced the global command line object, which might
cause heap-use-after-free of the temporary object at the end of the
test. For instance, ChromeExtensionsClient::GetWebstoreUpdateURL()
tries to read the command line flags, and does so asynchronously, not
in the main thread of the test.

The proposed patch eliminates use of the ScopedExperimentalCommandLine
at the test scope level, thus minimizing the risk of improper access to
memory.

BUG=
R=finnur@chromium.org
==========

to

==========
The command line flag is now set during the test startup.

These two tests replaced the global command line object, which might
cause heap-use-after-free of the temporary object at the end of the
test. For instance, ChromeExtensionsClient::GetWebstoreUpdateURL()
tries to read the command line flags, and does so asynchronously, not
in the main thread of the test.

The proposed patch eliminates use of the ScopedExperimentalCommandLine
at the test scope level, thus minimizing the risk of improper access to
memory.

BUG=
R=finnur@chromium.org

Committed: https://crrev.com/6ede907a3a8dfc095039b9fcd56478604bd2ae0a
Cr-Commit-Position: refs/heads/master@{#380096}
==========

[commit-bot: I haz the power, 2016-03-09 07:49:00]

Patchset 2 (id:??) landed as
https://crrev.com/6ede907a3a8dfc095039b9fcd56478604bd2ae0a
Cr-Commit-Position: refs/heads/master@{#380096}