ExtensionStorageMonitor: use smaller, self-registering StorageObservers

ExtensionStorageMonitor: use smaller, self-registering StorageObservers

Before this change, StorageEventObserver was a single observer that tried to
install a single observation per origin. However, this was problematic:

 - Two hosted apps can have the same storage origin.
   ExtensionsLoadMultipleTest.Test, for example, sets up a case where this
   occurs. The second registration would overwrite the state for the first, and
   it's possible that a dangling pointer would result (if the first registration
   had should_uma = true, and the second registration had should_uma = false).
   Also, disabling the second extension would leave the first extension
   unobserved.
 - The deregistration code, RemoveStorageObserverForFilter, didn't correctly
   uninstall the kStorageTypeTemporary filter, causing bug 716426.

To fix this StorageEventObserver is broken up into two classes:
 - SingleExtensionStorageObserver, which monitors the storage usage of
   one extension. This class registers/deregisters itself as an observer
   on creation/destruction (RAII pattern).
 - ExtensionStorageMonitorIOHelper, which keeps a container of
   SingleExtensionStorageObservers, one per extension.

Using RAII allows us to rip out the RemoveStorageObserverForFilter codepath
entirely from StorageMonitor and QuotaManager.

BUG=716426
TEST=ExtensionStorageMonitorTest

Review-Url: https://codereview.chromium.org/2923663002
Cr-Commit-Position: refs/heads/master@{#479546}
Committed: https://chromium.googlesource.com/chromium/src/+/e3873dfc2212de0b9b0784315b30b38ce0f46fbc

[ncarter (slow), 2017-06-05 20:23:29]

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

[commit-bot: I haz the power, 2017-06-05 20:24:10]

Dry run: CQ is trying da patch.

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

[ncarter (slow), 2017-06-05 20:37:14]

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

[commit-bot: I haz the power, 2017-06-05 20:37:39]

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-06-05 20:49:29]

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

[commit-bot: I haz the power, 2017-06-05 20:49:30]

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...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ncarter (slow), 2017-06-05 22:21:19]

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

[commit-bot: I haz the power, 2017-06-05 22:21:55]

Dry run: CQ is trying da patch.

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

[ncarter (slow), 2017-06-05 22:37:02]

Description was changed from

==========
ExtensionStorageMonitor: use smaller, self-registering StorageObservers

Before this change, StorageEventObserver was a single observer that tried to
install a single observation per origin. However, this was problematic:

 - Two hosted apps can have the same storage origin.
   ExtensionsLoadMultipleTest.Test, for example, sets up a case where this
   occurs. The second registration would overwrite the state for the first, and
   it's possible that a dangling pointer would result (if the first registration
   had should_uma = true, and the second registration had should_uma = false).
   Also, disabling the second extension would leave the first extension
   unobserved.
 - The deregistration code, RemoveStorageObserverForFilter, didn't correctly
   uninstall the kStorageTypeTemporary filter, causing bug 716426.

To fix this StorageEventObserver is broken up into two classes:
 - SingleExtensionStorageObserver, which monitors the storage usage of
   one extension. This class registers/deregisters itself as an observer
   on creation/destruction (RAII pattern).
 - ExtensionStorageMonitorIOHelper, which keeps a container of
   SingleExtensionStorageObservers, one per extension.

Using RAII allows us to rip out the RemoveStorageObserverForFilter codepath
entirely from StorageMonitor.

BUG=716426
==========

to

==========
ExtensionStorageMonitor: use smaller, self-registering StorageObservers

Before this change, StorageEventObserver was a single observer that tried to
install a single observation per origin. However, this was problematic:

 - Two hosted apps can have the same storage origin.
   ExtensionsLoadMultipleTest.Test, for example, sets up a case where this
   occurs. The second registration would overwrite the state for the first, and
   it's possible that a dangling pointer would result (if the first registration
   had should_uma = true, and the second registration had should_uma = false).
   Also, disabling the second extension would leave the first extension
   unobserved.
 - The deregistration code, RemoveStorageObserverForFilter, didn't correctly
   uninstall the kStorageTypeTemporary filter, causing bug 716426.

To fix this StorageEventObserver is broken up into two classes:
 - SingleExtensionStorageObserver, which monitors the storage usage of
   one extension. This class registers/deregisters itself as an observer
   on creation/destruction (RAII pattern).
 - ExtensionStorageMonitorIOHelper, which keeps a container of
   SingleExtensionStorageObservers, one per extension.

Using RAII allows us to rip out the RemoveStorageObserverForFilter codepath
entirely from StorageMonitor and QuotaManager.

BUG=716426
==========

[commit-bot: I haz the power, 2017-06-05 22:44:32]

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

[commit-bot: I haz the power, 2017-06-05 22:44:33]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ncarter (slow), 2017-06-05 23:02:49]

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

[commit-bot: I haz the power, 2017-06-05 23:03:35]

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-06-05 23:17:52]

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

[commit-bot: I haz the power, 2017-06-05 23:17:52]

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

[ncarter (slow), 2017-06-05 23:28:01]

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

[commit-bot: I haz the power, 2017-06-05 23:28:57]

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-06-06 00:17:08]

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

[commit-bot: I haz the power, 2017-06-06 00:17:09]

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

[ncarter (slow), 2017-06-09 21:00:57]

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

[commit-bot: I haz the power, 2017-06-09 21:01:26]

Dry run: CQ is trying da patch.

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

[ncarter (slow), 2017-06-09 21:05:24]

Description was changed from

==========
ExtensionStorageMonitor: use smaller, self-registering StorageObservers

Before this change, StorageEventObserver was a single observer that tried to
install a single observation per origin. However, this was problematic:

 - Two hosted apps can have the same storage origin.
   ExtensionsLoadMultipleTest.Test, for example, sets up a case where this
   occurs. The second registration would overwrite the state for the first, and
   it's possible that a dangling pointer would result (if the first registration
   had should_uma = true, and the second registration had should_uma = false).
   Also, disabling the second extension would leave the first extension
   unobserved.
 - The deregistration code, RemoveStorageObserverForFilter, didn't correctly
   uninstall the kStorageTypeTemporary filter, causing bug 716426.

To fix this StorageEventObserver is broken up into two classes:
 - SingleExtensionStorageObserver, which monitors the storage usage of
   one extension. This class registers/deregisters itself as an observer
   on creation/destruction (RAII pattern).
 - ExtensionStorageMonitorIOHelper, which keeps a container of
   SingleExtensionStorageObservers, one per extension.

Using RAII allows us to rip out the RemoveStorageObserverForFilter codepath
entirely from StorageMonitor and QuotaManager.

BUG=716426
==========

to

==========
ExtensionStorageMonitor: use smaller, self-registering StorageObservers

Before this change, StorageEventObserver was a single observer that tried to
install a single observation per origin. However, this was problematic:

 - Two hosted apps can have the same storage origin.
   ExtensionsLoadMultipleTest.Test, for example, sets up a case where this
   occurs. The second registration would overwrite the state for the first, and
   it's possible that a dangling pointer would result (if the first registration
   had should_uma = true, and the second registration had should_uma = false).
   Also, disabling the second extension would leave the first extension
   unobserved.
 - The deregistration code, RemoveStorageObserverForFilter, didn't correctly
   uninstall the kStorageTypeTemporary filter, causing bug 716426.

To fix this StorageEventObserver is broken up into two classes:
 - SingleExtensionStorageObserver, which monitors the storage usage of
   one extension. This class registers/deregisters itself as an observer
   on creation/destruction (RAII pattern).
 - ExtensionStorageMonitorIOHelper, which keeps a container of
   SingleExtensionStorageObservers, one per extension.

Using RAII allows us to rip out the RemoveStorageObserverForFilter codepath
entirely from StorageMonitor and QuotaManager.

BUG=716426
TEST=ExtensionStorageMonitorTest
==========

[commit-bot: I haz the power, 2017-06-09 21:57:23]

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

[commit-bot: I haz the power, 2017-06-09 21:57:24]

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

[ncarter (slow), 2017-06-09 23:32:34]

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

[commit-bot: I haz the power, 2017-06-09 23:33:11]

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-06-10 01:01:00]

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

[commit-bot: I haz the power, 2017-06-10 01:01:01]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2017-06-12 18:30:42]

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

[ncarter (slow), 2017-06-12 18:31:28]

nick@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org, tzik@chromium.org

[ncarter (slow), 2017-06-12 18:31:29]

devlin: For extension_storage_monitor.cc and tests
tzik: For code deletion under storage/

https://codereview.chromium.org/2923663002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/extension_storage_monitor.cc (left):

https://codereview.chromium.org/2923663002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/extension_storage_monitor.cc:176: filter);
|filter| here should be |temporary_filter|, and if we want to merge back this
fix, that is the 1-line version.

However, testing that fix revealed other lifetime management problems,
especially with hosted apps that share an origin -- StartObservingForExtension
would potentially overwrite the old registration in that case. Hence this more
comprehensive rewrite.

[commit-bot: I haz the power, 2017-06-12 18:31:34]

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-06-12 19:20:45]

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

[commit-bot: I haz the power, 2017-06-12 19:20:46]

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

[Devlin, 2017-06-12 20:15:05]

https://codereview.chromium.org/2923663002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/extension_storage_monitor.cc (right):

https://codereview.chromium.org/2923663002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/extension_storage_monitor.cc:48: const base::TimeDelta
kStorageEventRate = base::TimeDelta::FromSeconds(30);
is this allowed by our sizing/POD rules?

https://codereview.chromium.org/2923663002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/extension_storage_monitor.cc:147:
ExtensionStorageMonitorIOHelper* const io_helper_;
This seems to only be used to get a weak ptr to the ExtensionStorageMonitor;
could we instead just pass in that weak ptr directly?

https://codereview.chromium.org/2923663002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/extension_storage_monitor.cc:210: // some extensions
may be exempt from monitoring.
Maybe worth duplicating this comment on line 200?

https://codereview.chromium.org/2923663002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/extension_storage_monitor.cc:244: {
this scope seems unnecessary?

https://codereview.chromium.org/2923663002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/extension_storage_monitor.cc:344: if
(io_helper_.get()) {
nit: I don't think we need the .get() here, right?

https://codereview.chromium.org/2923663002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/extension_storage_monitor_browsertest.cc (right):

https://codereview.chromium.org/2923663002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/extension_storage_monitor_browsertest.cc:125:
std::unique_ptr<TestExtensionDir> dir(new TestExtensionDir);
prefer auto dir = base::MakeUnique<TestExtensionDir>();

https://codereview.chromium.org/2923663002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/extension_storage_monitor_browsertest.cc:217:
filesystem.c_str(), num_bytes),
... this formatting's a bit odd.  Was this git cl format?  If so, should we file
a bug, or is this desired?

https://codereview.chromium.org/2923663002/diff/180001/chrome/test/data/exten...
File chrome/test/data/extensions/storage_monitor/hosted_apps/common.js (right):

https://codereview.chromium.org/2923663002/diff/180001/chrome/test/data/exten...
chrome/test/data/extensions/storage_monitor/hosted_apps/common.js:5: var
kHundredChars = '';
do we care what these chars are?  If not, maybe
var kHundredChars = 'a'.repeat(100);
either way, we could also do:
var kHundredChars = '0123456789'.repeat(10);

edit: or moot with below

https://codereview.chromium.org/2923663002/diff/180001/chrome/test/data/exten...
chrome/test/data/extensions/storage_monitor/hosted_apps/common.js:30: return
((fileEntry) => {
Hmm... where's the first paren closed?  Am I missing it?

https://codereview.chromium.org/2923663002/diff/180001/chrome/test/data/exten...
chrome/test/data/extensions/storage_monitor/hosted_apps/common.js:40: // Avoid
many loops for large data packets.
or, here just:
str = 'a'.repeat(numChars);

[ncarter (slow), 2017-06-12 22:17:57]

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

[commit-bot: I haz the power, 2017-06-12 22:18:53]

Dry run: CQ is trying da patch.

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

[ncarter (slow), 2017-06-12 22:24:15]

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

[commit-bot: I haz the power, 2017-06-12 22:24:30]

Dry run: CQ is trying da patch.

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

[ncarter (slow), 2017-06-12 22:52:03]

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

[ncarter (slow), 2017-06-12 22:52:08]

PTAL

https://codereview.chromium.org/2923663002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/extension_storage_monitor.cc (right):

https://codereview.chromium.org/2923663002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/extension_storage_monitor.cc:48: const base::TimeDelta
kStorageEventRate = base::TimeDelta::FromSeconds(30);
On 2017/06/12 20:15:05, Devlin (sheriff Jun 9 - 12) wrote:
> is this allowed by our sizing/POD rules?

Good catch. TimeDelta has a constexpr ctor, but it looks like I'd forgotten to
change kStorageEventRate to be constexpr too. Fixed.

https://codereview.chromium.org/2923663002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/extension_storage_monitor.cc:147:
ExtensionStorageMonitorIOHelper* const io_helper_;
On 2017/06/12 20:15:05, Devlin (sheriff Jun 9 - 12) wrote:
> This seems to only be used to get a weak ptr to the ExtensionStorageMonitor;
> could we instead just pass in that weak ptr directly?

We could, but we'd still have to retain the WeakPtr<> data member in the
IOHelper, because we need to pass it to the ctor of
SingleExtensionStorageObserver. I did it this way (with the bare ptr back to the
helper) because a bare ptr is smaller than a WeakPtr, and copying a WeakPtr
involves bumping a refcount.

https://codereview.chromium.org/2923663002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/extension_storage_monitor.cc:210: // some extensions
may be exempt from monitoring.
On 2017/06/12 20:15:05, Devlin (sheriff Jun 9 - 12) wrote:
> Maybe worth duplicating this comment on line 200?

Done.

https://codereview.chromium.org/2923663002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/extension_storage_monitor.cc:244: {
On 2017/06/12 20:15:05, Devlin (sheriff Jun 9 - 12) wrote:
> this scope seems unnecessary?

Removed. I added this to get the diffs to line up for your first pass :)

https://codereview.chromium.org/2923663002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/extension_storage_monitor.cc:344: if
(io_helper_.get()) {
On 2017/06/12 20:15:05, Devlin (sheriff Jun 9 - 12) wrote:
> nit: I don't think we need the .get() here, right?

Done.

https://codereview.chromium.org/2923663002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/extension_storage_monitor_browsertest.cc (right):

https://codereview.chromium.org/2923663002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/extension_storage_monitor_browsertest.cc:125:
std::unique_ptr<TestExtensionDir> dir(new TestExtensionDir);
On 2017/06/12 20:15:05, Devlin (sheriff Jun 9 - 12) wrote:
> prefer auto dir = base::MakeUnique<TestExtensionDir>();

Done.

https://codereview.chromium.org/2923663002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/extension_storage_monitor_browsertest.cc:217:
filesystem.c_str(), num_bytes),
On 2017/06/12 20:15:05, Devlin (sheriff Jun 9 - 12) wrote:
> ... this formatting's a bit odd.  Was this git cl format?  If so, should we
file
> a bug, or is this desired?

Fixed by extracting the literal to a local variable. I'm still in search of a
great way to use raw strings w/ ExecuteScript, so let me know your ideas here. 

clang-format isn't allowed to mess with the whitespace inside the literal, and
it seems kind of odd to ask clang-format to align to whitespace _inside_ of a
string literal. So it's not clear to me what what bug we'd file.

https://codereview.chromium.org/2923663002/diff/180001/chrome/test/data/exten...
File chrome/test/data/extensions/storage_monitor/hosted_apps/common.js (right):

https://codereview.chromium.org/2923663002/diff/180001/chrome/test/data/exten...
chrome/test/data/extensions/storage_monitor/hosted_apps/common.js:5: var
kHundredChars = '';
On 2017/06/12 20:15:05, Devlin (sheriff Jun 9 - 12) wrote:
> do we care what these chars are?  If not, maybe
> var kHundredChars = 'a'.repeat(100);
> either way, we could also do:
> var kHundredChars = '0123456789'.repeat(10);
> 
> edit: or moot with below

Removed -- this was all copypasta from ../write_data/index.js.

https://codereview.chromium.org/2923663002/diff/180001/chrome/test/data/exten...
chrome/test/data/extensions/storage_monitor/hosted_apps/common.js:30: return
((fileEntry) => {
On 2017/06/12 20:15:05, Devlin (sheriff Jun 9 - 12) wrote:
> Hmm... where's the first paren closed?  Am I missing it?

Whoops! Serves me right for trying to rejigger this right before sending out the
CL. I DID test locally -- I swear -- but it looks like I screwed up the
--gtest_filter=

https://codereview.chromium.org/2923663002/diff/180001/chrome/test/data/exten...
chrome/test/data/extensions/storage_monitor/hosted_apps/common.js:40: // Avoid
many loops for large data packets.
On 2017/06/12 20:15:05, Devlin (sheriff Jun 9 - 12) wrote:
> or, here just:
> str = 'a'.repeat(numChars);

Much simpler! Done.

[commit-bot: I haz the power, 2017-06-12 22:52:30]

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-06-13 00:31:28]

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

[commit-bot: I haz the power, 2017-06-13 00:31:29]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-06-13 15:22:47]

extensions lgtm

[tzik, 2017-06-14 08:46:36]

storage lgtm

https://codereview.chromium.org/2923663002/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/extension_storage_monitor.cc (right):

https://codereview.chromium.org/2923663002/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/extension_storage_monitor.cc:239: // Keys are
extension IDs.
Could you update this comment? Or, just remove it?

[ncarter (slow), 2017-06-14 21:40:35]

https://codereview.chromium.org/2923663002/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/extension_storage_monitor.cc (right):

https://codereview.chromium.org/2923663002/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/extension_storage_monitor.cc:239: // Keys are
extension IDs.
On 2017/06/14 08:46:36, tzik wrote:
> Could you update this comment? Or, just remove it?

Done.

[ncarter (slow), 2017-06-14 21:40:42]

The CQ bit was checked by nick@chromium.org

[ncarter (slow), 2017-06-14 21:40:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, tzik@chromium.org
Link to the patchset: https://codereview.chromium.org/2923663002/#ps260001
(title: "Remove lame comment.")

[commit-bot: I haz the power, 2017-06-14 21:41:04]

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-06-14 23:43:48]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1497476442255390,
"parent_rev": "08df676af75230e261ae5e8713d4fc7481af938a", "commit_rev":
"e3873dfc2212de0b9b0784315b30b38ce0f46fbc"}

[commit-bot: I haz the power, 2017-06-14 23:44:03]

Description was changed from

==========
ExtensionStorageMonitor: use smaller, self-registering StorageObservers

Before this change, StorageEventObserver was a single observer that tried to
install a single observation per origin. However, this was problematic:

 - Two hosted apps can have the same storage origin.
   ExtensionsLoadMultipleTest.Test, for example, sets up a case where this
   occurs. The second registration would overwrite the state for the first, and
   it's possible that a dangling pointer would result (if the first registration
   had should_uma = true, and the second registration had should_uma = false).
   Also, disabling the second extension would leave the first extension
   unobserved.
 - The deregistration code, RemoveStorageObserverForFilter, didn't correctly
   uninstall the kStorageTypeTemporary filter, causing bug 716426.

To fix this StorageEventObserver is broken up into two classes:
 - SingleExtensionStorageObserver, which monitors the storage usage of
   one extension. This class registers/deregisters itself as an observer
   on creation/destruction (RAII pattern).
 - ExtensionStorageMonitorIOHelper, which keeps a container of
   SingleExtensionStorageObservers, one per extension.

Using RAII allows us to rip out the RemoveStorageObserverForFilter codepath
entirely from StorageMonitor and QuotaManager.

BUG=716426
TEST=ExtensionStorageMonitorTest
==========

to

==========
ExtensionStorageMonitor: use smaller, self-registering StorageObservers

Before this change, StorageEventObserver was a single observer that tried to
install a single observation per origin. However, this was problematic:

 - Two hosted apps can have the same storage origin.
   ExtensionsLoadMultipleTest.Test, for example, sets up a case where this
   occurs. The second registration would overwrite the state for the first, and
   it's possible that a dangling pointer would result (if the first registration
   had should_uma = true, and the second registration had should_uma = false).
   Also, disabling the second extension would leave the first extension
   unobserved.
 - The deregistration code, RemoveStorageObserverForFilter, didn't correctly
   uninstall the kStorageTypeTemporary filter, causing bug 716426.

To fix this StorageEventObserver is broken up into two classes:
 - SingleExtensionStorageObserver, which monitors the storage usage of
   one extension. This class registers/deregisters itself as an observer
   on creation/destruction (RAII pattern).
 - ExtensionStorageMonitorIOHelper, which keeps a container of
   SingleExtensionStorageObservers, one per extension.

Using RAII allows us to rip out the RemoveStorageObserverForFilter codepath
entirely from StorageMonitor and QuotaManager.

BUG=716426
TEST=ExtensionStorageMonitorTest

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

[commit-bot: I haz the power, 2017-06-14 23:44:05]

Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/e3873dfc2212de0b9b0784315b30...