[tracing] Add a memory dump provider for DOM storage

[tracing] Add a memory dump provider for DOM storage

This CL adds a memory dump provider for DOM storage in browser. It only
accounts for the memory usage of the buffer map and session storage
database, not the local backing databases. This will be done in next CL.
The areas can be accessed only on DOMStorageTaskRunner's
PRIMARY_SEQUENCE. Since memory infra supports collection of dumps from
SequencedTaskRunner, a SequencedTaskRunner is extracted from
DOMStorageTaskRunner's worker pool to pass to memory-infra.

BUG=605785
Committed: https://crrev.com/d0fd26c479bfa0b52f3764ee8925a1c82bf305ca
Cr-Commit-Position: refs/heads/master@{#391455}

[ssid, 2016-04-21 23:36:25]

Patchset #1 (id:1) has been deleted

[ssid, 2016-04-21 23:40:52]

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

[ssid, 2016-04-21 23:40:53]

ptal, thanks.

[ssid, 2016-04-21 23:52:49]

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

[ssid, 2016-04-21 23:52:49]

+michaeln This CL is to capture dom storage memory usage in tracing. ptal,
thanks.

[Primiano Tucci (use gerrit), 2016-04-22 09:56:40]

LGTM from the memory-infra viewpoint.

https://codereview.chromium.org/1906243002/diff/40001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_area.cc (right):

https://codereview.chromium.org/1906243002/diff/40001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:346: std::string name =
StringPrintf("dom_storage/%s/%p", url.c_str(), this);
I'd probably %s_%p instead of %s/%p just for the sake of avoiding one extra
level with one leaf only in the UI

https://codereview.chromium.org/1906243002/diff/40001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_namespace.cc (right):

https://codereview.chromium.org/1906243002/diff/40001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_namespace.cc:191: for
(AreaMap::const_iterator it = areas_.begin(); it != areas_.end(); ++it) {
or if you prefer just
for (const auto& area : areas_)

[ssid, 2016-04-22 23:19:37]

Description was changed from

==========
[tracing] Add a memory dump provider for DOM storage area

This CL adds a memory dump provider for DOM storage in browser. It only
accounts for the memory usage of the buffer map and not the backing
databases. This will be done in next CL.
The areas can be accessed only on DOMStorageTaskRunner's
PRIMARY_SEQUENCE. Since memory infra supports collection of dumps from
SequencedTaskRunner, a SequencedTaskRunner is extracted from
DOMStorageTaskRunner's worker pool to pass to memory-infra.

BUG=605785
==========

to

==========
[tracing] Add a memory dump provider for DOM storage namespaces

This CL adds a memory dump provider for DOM storage in browser. It only
accounts for the memory usage of the buffer map and not the backing
databases. This will be done in next CL.
The areas can be accessed only on DOMStorageTaskRunner's
PRIMARY_SEQUENCE. Since memory infra supports collection of dumps from
SequencedTaskRunner, a SequencedTaskRunner is extracted from
DOMStorageTaskRunner's worker pool to pass to memory-infra.

BUG=605785
==========

[ssid, 2016-04-22 23:28:16]

done thanks.

https://codereview.chromium.org/1906243002/diff/40001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_area.cc (right):

https://codereview.chromium.org/1906243002/diff/40001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:346: std::string name =
StringPrintf("dom_storage/%s/%p", url.c_str(), this);
On 2016/04/22 09:56:39, Primiano Tucci wrote:
> I'd probably %s_%p instead of %s/%p just for the sake of avoiding one extra
> level with one leaf only in the UI

No, I intended to make this %s/%p since it groups the usage by sites. it looks
ugly if I write %s_%p. 
There would be multiple 
google_com_0x23
google_com_0x23
google_com_0x45.

https://codereview.chromium.org/1906243002/diff/40001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_namespace.cc (right):

https://codereview.chromium.org/1906243002/diff/40001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_namespace.cc:191: for
(AreaMap::const_iterator it = areas_.begin(); it != areas_.end(); ++it) {
On 2016/04/22 09:56:39, Primiano Tucci wrote:
> or if you prefer just
> for (const auto& area : areas_)

Done.

[michaeln, 2016-04-22 23:39:42]

lgtm 2

https://codereview.chromium.org/1906243002/diff/60001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_area.cc (right):

https://codereview.chromium.org/1906243002/diff/60001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:344: url[index] = '_';
From a privacy perspective, is it ok to capture URLs in these traces? Just
checking.

[ssid, 2016-04-22 23:54:49]

Patchset #4 (id:80001) has been deleted

[ssid, 2016-04-23 00:19:15]

Thanks for the review.

https://codereview.chromium.org/1906243002/diff/60001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_area.cc (right):

https://codereview.chromium.org/1906243002/diff/60001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:344: url[index] = '_';
On 2016/04/22 23:39:41, michaeln wrote:
> From a privacy perspective, is it ok to capture URLs in these traces? Just
> checking.

Actually traces contain lot of sensitive information, like memory maps.
We have made sure that the tracing infra is not accessible without the visual
consent of the user. So, no script can start tracing and collect traces.

More over there is already a predecessor for adding urls:
https://chromium.googlesource.com/chromium/src/+blame/master/third_party/WebK...

[ssid, 2016-04-23 00:19:24]

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

[commit-bot: I haz the power, 2016-04-23 00:19:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1906243002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1906243002/100001

[michaeln, 2016-04-23 00:28:38]

https://codereview.chromium.org/1906243002/diff/100001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_namespace.cc (right):

https://codereview.chromium.org/1906243002/diff/100001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_namespace.cc:44:
base::trace_event::MemoryDumpManager::GetInstance()->UnregisterDumpProvider(
is the thread-safety-ness of this dependent on the unregister call happening
running on the sequenced task runner?

[ssid, 2016-04-23 00:33:14]

https://codereview.chromium.org/1906243002/diff/100001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_namespace.cc (right):

https://codereview.chromium.org/1906243002/diff/100001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_namespace.cc:44:
base::trace_event::MemoryDumpManager::GetInstance()->UnregisterDumpProvider(
On 2016/04/23 00:28:38, michaeln wrote:
> is the thread-safety-ness of this dependent on the unregister call happening
> running on the sequenced task runner?

Yes. Doesn't it?

[commit-bot: I haz the power, 2016-04-23 02:31:01]

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

[commit-bot: I haz the power, 2016-04-23 02:31:02]

Dry run: This issue passed the CQ dry run.

[ssid, 2016-04-25 21:24:28]

On 2016/04/23 02:31:02, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

I see only one method that destructs namespace object.
DOMStorageContextImpl::DeleteSessionNamespace

This is called the task runner with primary sequence. Am I missing something
here?

[michaeln, 2016-04-25 21:44:20]

This cl isn't threadsafe and i have haven't looked closely enough at the other
to know yet.

Maybe register the DOMStorageContextImpl as a dump provider and have it be
responsible for aggregating all memory usage info for the domstorage system,
including info from the each namespace/area and databases, this
DOMStorageContextImpl class owns the SessionStorageDatabase class your trying to
intrument in the other cl.

DOMStorageContextImpl::Shutdown() is guaranteed to be called on the primary task
runner sequence. There is no corresponding Initialize() method that is
guaranteed to be called on the primary sequence, but there could be. You can
easily add something like this at the end of
DOMStorageContextWrapper::DOMStorageContextWrappe().

  context_->task_runner()->PostShutdownBlockingTask(
      FROM_HERE,
      DOMStorageTaskRunner::PRIMARY_SEQUENCE,
      base::Bind(&DOMStorageContextImpl::Initialize, context_));

The Initialize method can register the class as a dumpprovider, the shutdown
method can unregister. No new locks needed, no new intermediary objects needed.

https://codereview.chromium.org/1906243002/diff/100001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_namespace.cc (right):

https://codereview.chromium.org/1906243002/diff/100001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_namespace.cc:44:
base::trace_event::MemoryDumpManager::GetInstance()->UnregisterDumpProvider(
On 2016/04/23 00:33:13, ssid wrote:
> On 2016/04/23 00:28:38, michaeln wrote:
> > is the thread-safety-ness of this dependent on the unregister call happening
> > running on the sequenced task runner?
> 
> Yes. Doesn't it?


no, this is not guaranteed to be called task runner

deja vu, i wish the interface betweeen dump mgr and providers were async. the
chosen approach seems error prone for all parties involved (the mgr as well as
providers).

[ssid, 2016-04-26 00:41:34]

Description was changed from

==========
[tracing] Add a memory dump provider for DOM storage namespaces

This CL adds a memory dump provider for DOM storage in browser. It only
accounts for the memory usage of the buffer map and not the backing
databases. This will be done in next CL.
The areas can be accessed only on DOMStorageTaskRunner's
PRIMARY_SEQUENCE. Since memory infra supports collection of dumps from
SequencedTaskRunner, a SequencedTaskRunner is extracted from
DOMStorageTaskRunner's worker pool to pass to memory-infra.

BUG=605785
==========

to

==========
[tracing] Add a memory dump provider for DOM storage 

This CL adds a memory dump provider for DOM storage in browser. It only
accounts for the memory usage of the buffer map and session storage
database, not the local backing databases. This will be done in next CL.
The areas can be accessed only on DOMStorageTaskRunner's
PRIMARY_SEQUENCE. Since memory infra supports collection of dumps from
SequencedTaskRunner, a SequencedTaskRunner is extracted from
DOMStorageTaskRunner's worker pool to pass to memory-infra.

BUG=605785
==========

[ssid, 2016-04-26 00:43:49]

Patchset #5 (id:120001) has been deleted

[ssid, 2016-04-26 00:51:55]

I merged both the CLs. added a single dump provider on the primary sequenced
task runner.
Call Session storage dump from context_impl.
The only missing piece is the local storage, for which i will write a different
CL.
ptal thanks!

On 2016/04/25 21:44:20, michaeln wrote:
> This cl isn't threadsafe and i have haven't looked closely enough at the other
> to know yet. 
>
> Maybe register the DOMStorageContextImpl as a dump provider and have it be
> responsible for aggregating all memory usage info for the domstorage system,
> including info from the each namespace/area and databases, this
> DOMStorageContextImpl class owns the SessionStorageDatabase class your trying
to
> intrument in the other cl.

Thanks. I couldn't find who owns SessionStorageDatabase. That is why I wrote 2
CL. Also reduces number of files changed if i made namespace as dump provider.
 
> DOMStorageContextImpl::Shutdown() is guaranteed to be called on the primary
task
> runner sequence. There is no corresponding Initialize() method that is
> guaranteed to be called on the primary sequence, but there could be. You can
> easily add something like this at the end of
> DOMStorageContextWrapper::DOMStorageContextWrappe().
>
>   context_->task_runner()->PostShutdownBlockingTask(
>       FROM_HERE,
>       DOMStorageTaskRunner::PRIMARY_SEQUENCE,
>       base::Bind(&DOMStorageContextImpl::Initialize, context_));
> 
> The Initialize method can register the class as a dumpprovider, the shutdown
> method can unregister. No new locks needed, no new intermediary objects
needed.

Yes, also this new method wouldn't be required. registering of mdp is thread
safe. dump manager holds lock to register providers and can be done from any
thread. So, registering is the constructor and unregistering in Shutdown.
I am hoping that shutdown will always get called before destruction.

https://codereview.chromium.org/1906243002/diff/100001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_namespace.cc (right):

https://codereview.chromium.org/1906243002/diff/100001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_namespace.cc:44:
base::trace_event::MemoryDumpManager::GetInstance()->UnregisterDumpProvider(
On 2016/04/25 21:44:19, michaeln wrote:
> On 2016/04/23 00:33:13, ssid wrote:
> > On 2016/04/23 00:28:38, michaeln wrote:
> > > is the thread-safety-ness of this dependent on the unregister call
happening
> > > running on the sequenced task runner?
> > 
> > Yes. Doesn't it?
> 
> 
> no, this is not guaranteed to be called task runner
> 
> deja vu, i wish the interface betweeen dump mgr and providers were async. the
> chosen approach seems error prone for all parties involved (the mgr as well as
> providers).
> 

Thanks, I was thinking DOMStorageContextImpl::DeleteSessionNamespace was the
only place that deletes namespace objects.

[michaeln, 2016-04-26 22:56:25]

https://codereview.chromium.org/1906243002/diff/160001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_area.cc (right):

https://codereview.chromium.org/1906243002/diff/160001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_area.cc:337: void
DOMStorageArea::OnMemoryDump(base::trace_event::ProcessMemoryDump* pmd) {
do give people warm fuzzies, can u add
DCHECK(task_runner_->IsRunningOnPrimarySequence());
here

https://codereview.chromium.org/1906243002/diff/160001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_impl.cc (right):

https://codereview.chromium.org/1906243002/diff/160001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_impl.cc:64:
DOMStorageContextImpl::~DOMStorageContextImpl() {
can you add a DCHECK(is_shutdown_) here to test our desirement about that having
happened and run some tryjobs, i bet there are cases where that is not true :(

[michaeln, 2016-04-27 00:07:44]

> i bet there are cases where that is not true :(

bah, redness... here's a suggestion to add the behavior u want without impacting
existing shutdown/delete logic

The DOMStorageContextWrapper is what owns the DOMStorageContextImpl, lets make
it responsible for keeping the context alive long enough to reg/unreg the thing
as a dump provider.

Move the RegisterDumpProviderWithSequencedTaskRunner call to
DOMStorageContextWrapper's constructor and add a code along these lines to the
wrapper's destructor...

DOMStorageContextWrapper::~DOMStorageContextWrapper() {
  context_->task_runner()->PostTask(
      FROM_HERE,
      DOMStorageTaskRunner::PRIMARY_SEQUENCE,
      base::Bind(&UnregisterDumpProviderHelper, context_));
}

UnregisterDumpProviderHelper(scoped_refptr<DOMStorageContextImpl> context) {
 
base::trace_event::MemoryDumpManager::GetInstance()->UnregisterDumpProvider(context);
}

[ssid, 2016-04-28 05:39:08]

Sorry for late reply, I was travelling.

On 2016/04/27 00:07:44, michaeln wrote:
> > i bet there are cases where that is not true :(
> 
> bah, redness... here's a suggestion to add the behavior u want without
impacting
> existing shutdown/delete logic

I think it is only the tests that do not shutdown the context properly. The code
should shut it down before destroying.
So, I fixed the tests.
What do you think?

> The DOMStorageContextWrapper is what owns the DOMStorageContextImpl, lets make
> it responsible for keeping the context alive long enough to reg/unreg the
thing
> as a dump provider.

I'd prefer to fix the tests rather than change the life time of the context just
for tracing. It sounds like making the code more complicated than required.

> Move the RegisterDumpProviderWithSequencedTaskRunner call to
> DOMStorageContextWrapper's constructor and add a code along these lines to the
> wrapper's destructor...
> 
> DOMStorageContextWrapper::~DOMStorageContextWrapper() {
>   context_->task_runner()->PostTask(
>       FROM_HERE,
>       DOMStorageTaskRunner::PRIMARY_SEQUENCE,
>       base::Bind(&UnregisterDumpProviderHelper, context_));
> }
> 
> UnregisterDumpProviderHelper(scoped_refptr<DOMStorageContextImpl> context) {
>  
>
base::trace_event::MemoryDumpManager::GetInstance()->UnregisterDumpProvider(context);
> }

[michaeln, 2016-05-02 22:47:12]

lgtm

https://codereview.chromium.org/1906243002/diff/200001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_impl.cc (right):

https://codereview.chromium.org/1906243002/diff/200001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_impl.cc:189: void
DOMStorageContextImpl::Shutdown() {
DCHECK(!task_runner_ || task_runner_->IsRunningOnPrimarySequence());

Can you add that dcheck since we're relying on that now. It would also catch the
bug if somebody were to add a call to Shutdown() from within the dtor as a "fix"
for seeing DCHECK(is_shutdown_) tripping.

https://codereview.chromium.org/1906243002/diff/200001/content/browser/dom_st...
File content/browser/dom_storage/session_storage_database.h (right):

https://codereview.chromium.org/1906243002/diff/200001/content/browser/dom_st...
content/browser/dom_storage/session_storage_database.h:85: bool
OnMemoryDump(base::trace_event::ProcessMemoryDump* pmd);
maybe remove the return value since it's always |true| and not used at the
callsite anyway

[ssid, 2016-05-04 05:02:07]

Patchset #9 (id:220001) has been deleted

[ssid, 2016-05-04 05:02:41]

Patchset #9 (id:240001) has been deleted

[ssid, 2016-05-04 06:06:14]

Thanks. fixed.

https://codereview.chromium.org/1906243002/diff/160001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_area.cc (right):

https://codereview.chromium.org/1906243002/diff/160001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_area.cc:337: void
DOMStorageArea::OnMemoryDump(base::trace_event::ProcessMemoryDump* pmd) {
On 2016/04/26 22:56:25, michaeln wrote:
> do give people warm fuzzies, can u add
> DCHECK(task_runner_->IsRunningOnPrimarySequence());
> here

Done.

https://codereview.chromium.org/1906243002/diff/200001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_impl.cc (right):

https://codereview.chromium.org/1906243002/diff/200001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_impl.cc:189: void
DOMStorageContextImpl::Shutdown() {
On 2016/05/02 22:47:12, michaeln wrote:
> DCHECK(!task_runner_ || task_runner_->IsRunningOnPrimarySequence());
> 
> Can you add that dcheck since we're relying on that now. It would also catch
the
> bug if somebody were to add a call to Shutdown() from within the dtor as a
"fix"
> for seeing DCHECK(is_shutdown_) tripping.

Done.

https://codereview.chromium.org/1906243002/diff/200001/content/browser/dom_st...
File content/browser/dom_storage/session_storage_database.h (right):

https://codereview.chromium.org/1906243002/diff/200001/content/browser/dom_st...
content/browser/dom_storage/session_storage_database.h:85: bool
OnMemoryDump(base::trace_event::ProcessMemoryDump* pmd);
On 2016/05/02 22:47:12, michaeln wrote:
> maybe remove the return value since it's always |true| and not used at the
> callsite anyway

Done.

[ssid, 2016-05-04 06:06:26]

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

[commit-bot: I haz the power, 2016-05-04 06:07:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1906243002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1906243002/260001

[commit-bot: I haz the power, 2016-05-04 06:47:38]

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

[commit-bot: I haz the power, 2016-05-04 06:47:39]

Dry run: This issue passed the CQ dry run.

[ssid, 2016-05-04 07:24:28]

The CQ bit was checked by ssid@chromium.org

[ssid, 2016-05-04 07:24:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, michaeln@chromium.org
Link to the patchset: https://codereview.chromium.org/1906243002/#ps260001
(title: "Fix dcheck and return value.")

[commit-bot: I haz the power, 2016-05-04 07:24:37]

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

[commit-bot: I haz the power, 2016-05-04 07:28:56]

Description was changed from

==========
[tracing] Add a memory dump provider for DOM storage 

This CL adds a memory dump provider for DOM storage in browser. It only
accounts for the memory usage of the buffer map and session storage
database, not the local backing databases. This will be done in next CL.
The areas can be accessed only on DOMStorageTaskRunner's
PRIMARY_SEQUENCE. Since memory infra supports collection of dumps from
SequencedTaskRunner, a SequencedTaskRunner is extracted from
DOMStorageTaskRunner's worker pool to pass to memory-infra.

BUG=605785
==========

to

==========
[tracing] Add a memory dump provider for DOM storage 

This CL adds a memory dump provider for DOM storage in browser. It only
accounts for the memory usage of the buffer map and session storage
database, not the local backing databases. This will be done in next CL.
The areas can be accessed only on DOMStorageTaskRunner's
PRIMARY_SEQUENCE. Since memory infra supports collection of dumps from
SequencedTaskRunner, a SequencedTaskRunner is extracted from
DOMStorageTaskRunner's worker pool to pass to memory-infra.

BUG=605785
==========

[commit-bot: I haz the power, 2016-05-04 07:28:57]

Committed patchset #9 (id:260001)

[commit-bot: I haz the power, 2016-05-04 07:30:01]

Description was changed from

==========
[tracing] Add a memory dump provider for DOM storage 

This CL adds a memory dump provider for DOM storage in browser. It only
accounts for the memory usage of the buffer map and session storage
database, not the local backing databases. This will be done in next CL.
The areas can be accessed only on DOMStorageTaskRunner's
PRIMARY_SEQUENCE. Since memory infra supports collection of dumps from
SequencedTaskRunner, a SequencedTaskRunner is extracted from
DOMStorageTaskRunner's worker pool to pass to memory-infra.

BUG=605785
==========

to

==========
[tracing] Add a memory dump provider for DOM storage

This CL adds a memory dump provider for DOM storage in browser. It only
accounts for the memory usage of the buffer map and session storage
database, not the local backing databases. This will be done in next CL.
The areas can be accessed only on DOMStorageTaskRunner's
PRIMARY_SEQUENCE. Since memory infra supports collection of dumps from
SequencedTaskRunner, a SequencedTaskRunner is extracted from
DOMStorageTaskRunner's worker pool to pass to memory-infra.

BUG=605785

Committed: https://crrev.com/d0fd26c479bfa0b52f3764ee8925a1c82bf305ca
Cr-Commit-Position: refs/heads/master@{#391455}
==========

[commit-bot: I haz the power, 2016-05-04 07:30:02]

Patchset 9 (id:??) landed as
https://crrev.com/d0fd26c479bfa0b52f3764ee8925a1c82bf305ca
Cr-Commit-Position: refs/heads/master@{#391455}