Remove unused lock, add debug-only verification of intended usage.

Remove unused lock, add verification of intended usage.

There used to be locking in this class, but many small changes had contributed to removing all use of the lock. The lock does not appear to actually be needed, as long as the class continues to be used as it currently is used, with the deferred initialization via RegisterExtensionService always happening before the providers map owned by the object is iterated over.

This change removes the |lock_| member and adds debug-only code that verifies that this is how the class continues to be used.

BUG=none

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

[jochen (gone - plz use gerrit), 2013-02-08 13:57:07]

lgtm

[commit-bot: I haz the power, 2013-02-08 13:57:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12223052/1

[commit-bot: I haz the power, 2013-02-08 15:32:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12223052/1

[commit-bot: I haz the power, 2013-02-08 16:25:39]

Retried try job too often on linux_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[Jói, 2013-02-11 17:32:00]

Hi again Jochen,

This wasn't quite as simple as I thought.

HostContentSettingsMap has a few places where it enumerates the map the lock is
supposed to protect, and makes outcalls, that may result in e.g. notification
calls being made that come back to HostContentSettingsMap.  So we need to avoid
making outcalls while holding the lock, and we also need to avoid the
possibility of any of these outcalls modifying the map while we're iterating
over it.  Right now that doesn't look like it would happen, but that's not
something we can guarantee within the class.

Something like the new patchset would work (NB: doesn't pass tests yet, so
review only for the idea), but that made me think perhaps an array of fixed size
would be more suitable than a std::map.  What do you think?

I'd be happy to fix it that way if you agree that would be better.  Note that
this is related to (although it does not directly fix) a bug tagged M26 so I'd
try to get this prepared ASAP.

Cheers,
Jói

[jochen (gone - plz use gerrit), 2013-02-11 18:29:49]

so reading through the code again, the provider map shouldn't really every
change. Even the setting of the extension provider should happen before anything
else is used in the HCSM (except for during tests, when it's sometimes not set
at all)

Historically, the HCSM also managed the settings which were protected by the
lock_.

I think it should be possible to add a bool fully_initialized_ which you set to
true when the extension provider was set, and then check that it's set before
accessing the map.

tl;dr I think wrt the bug, I think this is a red herring.

Otherwise, using a std::map seems a bit of an overkill, given that we only have
a known number of providers.

Maybe it helps to have a quick chat about this tomorrow?

[Jói, 2013-02-11 19:34:15]

Hi Jochen,

I think you're right it's not the cause for the bug.  I never thought
it was, but I did think it was a bug; now I'm starting to think it may
not really be a bug (i.e. perhaps the lock_ member can just go away
completely, as I think you're suggesting).

I'll look into this some more tomorrow morning, and maybe we can chat.

Cheers,
Jói



On Mon, Feb 11, 2013 at 6:29 PM,  <jochen@chromium.org> wrote:
> so reading through the code again, the provider map shouldn't really every
> change. Even the setting of the extension provider should happen before
> anything
> else is used in the HCSM (except for during tests, when it's sometimes not
> set
> at all)
>
> Historically, the HCSM also managed the settings which were protected by the
> lock_.
>
> I think it should be possible to add a bool fully_initialized_ which you set
> to
> true when the extension provider was set, and then check that it's set
> before
> accessing the map.
>
> tl;dr I think wrt the bug, I think this is a red herring.
>
> Otherwise, using a std::map seems a bit of an overkill, given that we only
> have
> a known number of providers.
>
> Maybe it helps to have a quick chat about this tomorrow?
>
> https://codereview.chromium.org/12223052/

[Jói, 2013-02-12 12:10:09]

Hi Jochen,

Please take a look at the new approach. I changed the change description and
subject to reflect the change in approach.

Cheers,
Jói

[jochen (gone - plz use gerrit), 2013-02-12 12:15:18]

lgtm!

[commit-bot: I haz the power, 2013-02-12 12:16:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12223052/10002

[commit-bot: I haz the power, 2013-02-12 12:33:03]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-02-12 13:09:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12223052/1009

[commit-bot: I haz the power, 2013-02-12 14:14:14]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-02-12 14:20:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12223052/1009

[Jói, 2013-02-12 14:58:59]

Committed manually as r181930 as the CQ was having trouble with
consistently-failing interactive_ui_tests on mac_rel.