Whitelist net/ MemoryDumpProvider

Whitelist net/ MemoryDumpProvider

(1) Whitelist net/ MemoryDumpProvider.
(2) Run net/ MemoryDumpProvider unittests in background mode.
(3) Use PRIxPTR when printing pointer address in MemoryDumpProvider.
(4) Annotate the "origin" of a URLRequestContext in a string attribute
rather than in the name of the allocator dump. This is to facilitate the
regex matching of the whitelist.

BUG=669108
Review-Url: https://codereview.chromium.org/2699163002
Cr-Commit-Position: refs/heads/master@{#452836}
Committed: https://chromium.googlesource.com/chromium/src/+/d5debfcf471cb88798a6043f28f6631c497ae4de

[xunjieli, 2017-02-17 16:48:25]

Description was changed from

==========
Run net/ MemoryDumpProvider unittests in BACKGROUND and DETAILED mode

BUG=669108
==========

to

==========
Run net/ MemoryDumpProvider unittests in BACKGROUND and DETAILED mode

BUG=669108
==========

[xunjieli, 2017-02-17 16:48:25]

xunjieli@chromium.org changed reviewers:
+ ssid@chromium.org

[xunjieli, 2017-02-17 16:50:12]

https://codereview.chromium.org/2699163002/diff/1/net/base/sdch_manager_unitt...
File net/base/sdch_manager_unittest.cc (right):

https://codereview.chromium.org/2699163002/diff/1/net/base/sdch_manager_unitt...
net/base/sdch_manager_unittest.cc:669: base::trace_event::MemoryDumpArgs
dump_args = {GetParam()};
sid: Do I need to set "ProcessMemoryDump::is_black_hole_non_fatal_for_testing_ =
true"
when in BACKGROUND mode?
Currently these tests will crash in BACKGROUND mode.

I guess I don't understand how this works.. How does it catch the case where
someone adds new dumps?

[ssid, 2017-02-21 18:07:44]

Thanks for working on this. We need to merge this cl with the whitelisting CL.

I saw that you are updating https://codereview.chromium.org/2661333002. I
thought we decided there are no more changes to dump provider.

You also need to update the stringprintf calls in all the dump functions to use
PRIXPTR instead of directly printing the address. That is:
Change the code at
https://chromium.googlesource.com/chromium/src/+/c51531ca429e54478b286186091d...
to something like this:
https://chromium.googlesource.com/chromium/src/+/c51531ca429e54478b286186091d...


This is because we rely on the fact that the pointers are printed using 0xff
format. But windows does not do this and prints the hex directly. We need to
rely on having 0x to regex match the string in background_whitelist.cc.

https://codereview.chromium.org/2699163002/diff/1/net/base/sdch_manager_unitt...
File net/base/sdch_manager_unittest.cc (right):

https://codereview.chromium.org/2699163002/diff/1/net/base/sdch_manager_unitt...
net/base/sdch_manager_unittest.cc:669: base::trace_event::MemoryDumpArgs
dump_args = {GetParam()};
On 2017/02/17 16:50:11, xunjieli wrote:
> sid: Do I need to set "ProcessMemoryDump::is_black_hole_non_fatal_for_testing_
=
> true"
> when in BACKGROUND mode?
No. This shouldn't be set.

> Currently these tests will crash in BACKGROUND mode.
> 
> I guess I don't understand how this works.. How does it catch the case where
> someone adds new dumps?
Sorry I forgot to mention. This has to be done along with this cl:
https://codereview.chromium.org/2667293003/ which adds the dump names to
whitelist.

Is it possible for you to take over that cl and merge it with this one?

[xunjieli, 2017-02-21 21:49:13]

Description was changed from

==========
Run net/ MemoryDumpProvider unittests in BACKGROUND and DETAILED mode

BUG=669108
==========

to

==========
Whitelist net/ MemoryDumpProvider

(1) Whitelist net/ MemoryDumpProvider.
(2) Run net/ MemoryDumpProvider unittests in background mode.
(3) Use PRIxPTR when printing pointer address in MemoryDumpProvider.

BUG=669108
==========

[xunjieli, 2017-02-21 21:49:34]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 21:50:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 21:55:51]

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

[commit-bot: I haz the power, 2017-02-21 21:55:52]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[xunjieli, 2017-02-21 22:10:58]

Description was changed from

==========
Whitelist net/ MemoryDumpProvider

(1) Whitelist net/ MemoryDumpProvider.
(2) Run net/ MemoryDumpProvider unittests in background mode.
(3) Use PRIxPTR when printing pointer address in MemoryDumpProvider.

BUG=669108
==========

to

==========
Whitelist net/ MemoryDumpProvider

(1) Whitelist net/ MemoryDumpProvider.
(2) Run net/ MemoryDumpProvider unittests in background mode.
(3) Use PRIxPTR when printing pointer address in MemoryDumpProvider.
(4) Annotate the "origin" of a URLRequestContext in a string attribute
rather than in the name of the allocator dump. This is to faciliate the
regex matching of whitelist.

BUG=669108
==========

[xunjieli, 2017-02-21 22:11:10]

Description was changed from

==========
Whitelist net/ MemoryDumpProvider

(1) Whitelist net/ MemoryDumpProvider.
(2) Run net/ MemoryDumpProvider unittests in background mode.
(3) Use PRIxPTR when printing pointer address in MemoryDumpProvider.
(4) Annotate the "origin" of a URLRequestContext in a string attribute
rather than in the name of the allocator dump. This is to faciliate the
regex matching of whitelist.

BUG=669108
==========

to

==========
Whitelist net/ MemoryDumpProvider

(1) Whitelist net/ MemoryDumpProvider.
(2) Run net/ MemoryDumpProvider unittests in background mode.
(3) Use PRIxPTR when printing pointer address in MemoryDumpProvider.
(4) Annotate the "origin" of a URLRequestContext in a string attribute
rather than in the name of the allocator dump. This is to facilitate the
regex matching of whitelist.

BUG=669108
==========

[xunjieli, 2017-02-21 22:11:19]

Description was changed from

==========
Whitelist net/ MemoryDumpProvider

(1) Whitelist net/ MemoryDumpProvider.
(2) Run net/ MemoryDumpProvider unittests in background mode.
(3) Use PRIxPTR when printing pointer address in MemoryDumpProvider.
(4) Annotate the "origin" of a URLRequestContext in a string attribute
rather than in the name of the allocator dump. This is to facilitate the
regex matching of whitelist.

BUG=669108
==========

to

==========
Whitelist net/ MemoryDumpProvider

(1) Whitelist net/ MemoryDumpProvider.
(2) Run net/ MemoryDumpProvider unittests in background mode.
(3) Use PRIxPTR when printing pointer address in MemoryDumpProvider.
(4) Annotate the "origin" of a URLRequestContext in a string attribute
rather than in the name of the allocator dump. This is to facilitate the
regex matching of the whitelist.

BUG=669108
==========

[xunjieli, 2017-02-23 18:59:15]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 18:59:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xunjieli, 2017-02-23 19:00:21]

xunjieli@chromium.org changed reviewers:
+ primiano@google.com

[xunjieli, 2017-02-23 19:00:22]

primiano@: PTAL base/trace_event/memory_infra_background_whitelist.cc

ssid@: PTAL all.

https://codereview.chromium.org/2699163002/diff/1/net/base/sdch_manager_unitt...
File net/base/sdch_manager_unittest.cc (right):

https://codereview.chromium.org/2699163002/diff/1/net/base/sdch_manager_unitt...
net/base/sdch_manager_unittest.cc:669: base::trace_event::MemoryDumpArgs
dump_args = {GetParam()};
On 2017/02/21 18:07:43, ssid wrote:
> On 2017/02/17 16:50:11, xunjieli wrote:
> > sid: Do I need to set
"ProcessMemoryDump::is_black_hole_non_fatal_for_testing_
> =
> > true"
> > when in BACKGROUND mode?
> No. This shouldn't be set.
> 
> > Currently these tests will crash in BACKGROUND mode.
> > 
> > I guess I don't understand how this works.. How does it catch the case where
> > someone adds new dumps?
> Sorry I forgot to mention. This has to be done along with this cl:
> https://codereview.chromium.org/2667293003/ which adds the dump names to
> whitelist.
> 
> Is it possible for you to take over that cl and merge it with this one?

Done.

[commit-bot: I haz the power, 2017-02-23 19:32:21]

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

[commit-bot: I haz the power, 2017-02-23 19:32:22]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Primiano Tucci (use gerrit), 2017-02-23 19:32:40]

I fell a bit out of the loop on the performance/coverage discussion. defer the
choice to ssid@
Plz ping me in if there any unresolved controversy. 
RS-LGTM to base/trace_event/memory_infra_background_whitelist.cc .

[xunjieli, 2017-02-23 20:21:46]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 20:22:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ssid, 2017-02-23 20:41:01]

Great, lgtm thanks!

One question: Do you believe we this unittests cover all the instrumentation
done in codebase?

https://codereview.chromium.org/2699163002/diff/120001/base/trace_event/memor...
File base/trace_event/memory_infra_background_whitelist.cc (right):

https://codereview.chromium.org/2699163002/diff/120001/base/trace_event/memor...
base/trace_event/memory_infra_background_whitelist.cc:75:
"net/ssl_session_cache",
nit: fix sorting

https://codereview.chromium.org/2699163002/diff/120001/net/url_request/url_re...
File net/url_request/url_request_context.cc (right):

https://codereview.chromium.org/2699163002/diff/120001/net/url_request/url_re...
net/url_request/url_request_context.cc:146: "net/url_request_context_0x%"
PRIxPTR, reinterpret_cast<uintptr_t>(this));
include inttypes?

[xunjieli, 2017-02-23 20:48:17]

> One question: Do you believe we this unittests cover all the instrumentation
done in codebase?

The unit tests cover all places where we create allocator dumps. But the tests
don't have 100% coverage. For example, in SimpleCache, we might not be following
through all possible EMU()s, but EMU() calls should be fairly safe.

https://codereview.chromium.org/2699163002/diff/120001/base/trace_event/memor...
File base/trace_event/memory_infra_background_whitelist.cc (right):

https://codereview.chromium.org/2699163002/diff/120001/base/trace_event/memor...
base/trace_event/memory_infra_background_whitelist.cc:75:
"net/ssl_session_cache",
On 2017/02/23 20:41:01, ssid wrote:
> nit: fix sorting

Done.

https://codereview.chromium.org/2699163002/diff/120001/net/url_request/url_re...
File net/url_request/url_request_context.cc (right):

https://codereview.chromium.org/2699163002/diff/120001/net/url_request/url_re...
net/url_request/url_request_context.cc:146: "net/url_request_context_0x%"
PRIxPTR, reinterpret_cast<uintptr_t>(this));
On 2017/02/23 20:41:01, ssid wrote:
> include inttypes?

Done.

[xunjieli, 2017-02-23 21:12:35]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 21:13:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xunjieli, 2017-02-23 23:09:32]

The CQ bit was unchecked by xunjieli@chromium.org

[xunjieli, 2017-02-23 23:09:34]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2017-02-23 23:09:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, ssid@chromium.org
Link to the patchset: https://codereview.chromium.org/2699163002/#ps140001
(title: "ssid comments")

[commit-bot: I haz the power, 2017-02-23 23:10:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 00:58:08]

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

[commit-bot: I haz the power, 2017-02-24 00:58:09]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[xunjieli, 2017-02-24 14:55:45]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2017-02-24 14:55:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, ssid@chromium.org
Link to the patchset: https://codereview.chromium.org/2699163002/#ps160001
(title: "fix windows")

[commit-bot: I haz the power, 2017-02-24 14:55:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xunjieli, 2017-02-24 14:55:59]

The CQ bit was unchecked by xunjieli@chromium.org

[xunjieli, 2017-02-24 14:56:06]

Patchset #9 (id:160001) has been deleted

[xunjieli, 2017-02-24 14:57:18]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2017-02-24 14:57:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, ssid@chromium.org
Link to the patchset: https://codereview.chromium.org/2699163002/#ps180001
(title: "fix test on windows")

[commit-bot: I haz the power, 2017-02-24 14:57:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 15:55:53]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1487948238597670,
"parent_rev": "a4a59720181d50024b86da5937cc7ae459f725a6", "commit_rev":
"d5debfcf471cb88798a6043f28f6631c497ae4de"}

[commit-bot: I haz the power, 2017-02-24 15:56:30]

Description was changed from

==========
Whitelist net/ MemoryDumpProvider

(1) Whitelist net/ MemoryDumpProvider.
(2) Run net/ MemoryDumpProvider unittests in background mode.
(3) Use PRIxPTR when printing pointer address in MemoryDumpProvider.
(4) Annotate the "origin" of a URLRequestContext in a string attribute
rather than in the name of the allocator dump. This is to facilitate the
regex matching of the whitelist.

BUG=669108
==========

to

==========
Whitelist net/ MemoryDumpProvider

(1) Whitelist net/ MemoryDumpProvider.
(2) Run net/ MemoryDumpProvider unittests in background mode.
(3) Use PRIxPTR when printing pointer address in MemoryDumpProvider.
(4) Annotate the "origin" of a URLRequestContext in a string attribute
rather than in the name of the allocator dump. This is to facilitate the
regex matching of the whitelist.

BUG=669108

Review-Url: https://codereview.chromium.org/2699163002
Cr-Commit-Position: refs/heads/master@{#452836}
Committed:
https://chromium.googlesource.com/chromium/src/+/d5debfcf471cb88798a6043f28f6...
==========

[commit-bot: I haz the power, 2017-02-24 15:56:31]

Committed patchset #9 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/d5debfcf471cb88798a6043f28f6...