Listen to Off The Record profiles in ContentSettingsHandler.

Listen to Off The Record profiles in ContentSettingsHandler.

ContentSettingsHandler is now made aware of Off The Record profiles
and will observe them as well, correcting bugs where changes from the
settings webUI would function incorrectly.

This is a rework of a previously landed fix [fix] which was reverted due to
a crash bug [crash] when OTR profiles already existed before settings
were opened.

[fix] https://codereview.chromium.org/585953003
[crash] https://code.google.com/p/chromium/issues/detail?id=417597

BUG=425079, 418931
Committed: https://crrev.com/c9ba380c606442a025a38eb67f4d35c65b1a293c
Cr-Commit-Position: refs/heads/master@{#301273}

Committed: https://crrev.com/cc12a6ab824a40524b5d046b8cd80253d19ff50f
Cr-Commit-Position: refs/heads/master@{#301461}

[scheib, 2014-10-24 19:26:01]

Patchset #1 (id:1) has been deleted

[scheib, 2014-10-24 19:30:14]

scheib@chromium.org changed reviewers:
+ thestig@chromium.org

[Lei Zhang, 2014-10-24 19:57:29]

https://codereview.chromium.org/676083003/diff/40001/base/scoped_observer.h
File base/scoped_observer.h (right):

https://codereview.chromium.org/676083003/diff/40001/base/scoped_observer.h#n...
base/scoped_observer.h:35: DCHECK(it != sources_.end());
Looking at base/observer_list_unittest.cc, it should be ok to call
RemoveObserver() for something that's not being observed.

https://codereview.chromium.org/676083003/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/676083003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/content_settings_handler.cc:542:
observer_.Remove(profile->GetHostContentSettingsMap());
Can't we potentially failed the newly added DCHECK in ScopedObserver if we:

1) Start with Profile A and Profile B, both profiles have OTR profiles.

2) At some point ContentSettingsHandler::InitializeHandler() runs with Profile A
as the Profile::FromWebUI() result.

- |observers_| now contains Profile A and Profile A/OTR.

3) Profile B/OTR gets destroyed. This notification observer gets called.

- Attempts to remove Profile B/OTR, which isn't in |observers_|.

[scheib, 2014-10-24 21:46:19]

https://codereview.chromium.org/676083003/diff/40001/base/scoped_observer.h
File base/scoped_observer.h (right):

https://codereview.chromium.org/676083003/diff/40001/base/scoped_observer.h#n...
base/scoped_observer.h:35: DCHECK(it != sources_.end());
On 2014/10/24 19:57:29, Lei Zhang wrote:
> Looking at base/observer_list_unittest.cc, it should be ok to call
> RemoveObserver() for something that's not being observed.

True, but I think errors such as the previous one in ContentSettingsHandler
would not be detected unless a DCHECK is made.

https://codereview.chromium.org/676083003/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/676083003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/content_settings_handler.cc:542:
observer_.Remove(profile->GetHostContentSettingsMap());
On 2014/10/24 19:57:29, Lei Zhang wrote:
> Can't we potentially failed the newly added DCHECK in ScopedObserver if we:
> 
> 1) Start with Profile A and Profile B, both profiles have OTR profiles.
> 
> 2) At some point ContentSettingsHandler::InitializeHandler() runs with Profile
A
> as the Profile::FromWebUI() result.
> 
> - |observers_| now contains Profile A and Profile A/OTR.
> 
> 3) Profile B/OTR gets destroyed. This notification observer gets called.
> 
> - Attempts to remove Profile B/OTR, which isn't in |observers_|.

Good catch. I've added a check.

[Lei Zhang, 2014-10-24 23:27:46]

lgtm

[scheib, 2014-10-25 02:00:57]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2014-10-25 02:01:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/676083003/60001

[commit-bot: I haz the power, 2014-10-25 03:03:48]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2014-10-25 03:04:31]

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

[Avi (use Gerrit), 2014-10-25 19:15:17]

A revert of this CL (patchset #3 id:60001) has been created in
https://codereview.chromium.org/652183006/ by avi@chromium.org.

The reason for reverting is:
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...

is dying consistently in GuestModeOptionsBrowserTest.LoadOptionsByURL:

[16175:16175:1024/205028:FATAL:observer_list.h(159)] Check failed: false.
Observers can only be added once!
#0 0x7fca41ce4b8e base::debug::StackTrace::StackTrace()

This looks like a very related CL..

[scheib, 2014-10-27 20:49:18]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2014-10-27 20:50:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/676083003/100001

[Lei Zhang, 2014-10-27 21:11:38]

Fix in patch set 5 lgtm

[commit-bot: I haz the power, 2014-10-27 22:17:43]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2014-10-27 22:18:22]

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