Change PepperFlashBrowserHost to use CookieSettings to get the LSO settings

Change PepperFlashBrowserHost to use CookieSettings to get the LSO settings

This changes the PepperFlashBrowserHost to use CookieSettings to get the LSO settings as opposed to using the resource context and going through the ChromeContentBrowserClient. The CookieSettings are thread-safe refcounted (as opposed to the ResourceContext) so they are safe to access even after the profile is destroyed which should fix the associated bug.

This also removes the now unused functions in ChromeContentBrowserClient.

TBR=brettw for content/public/browser/*
BUG=167353
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=174781

[raymes, 2012-12-29 01:23:58]

PTAL. It's complicated but unfortunately I think it's necessary complexity. I
looked hard for a better way to handle this but could not find one.

Note that the crash reports from the associated bug still do not completely make
sense to me because the UI thread in those report does not appear to have posted
a task to delete the resource context yet. However, I was able to locally
reproduce a situation where the resource context is accessed after it has been
deleted so this CL fixes a real bug and I'm reasonably confident that it will
fix the associated crashes.

[yzshen1, 2012-12-29 02:04:05]

https://codereview.chromium.org/11705003/diff/2001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/pepper/pepper_flash_browser_host.cc (right):

https://codereview.chromium.org/11705003/diff/2001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/pepper_flash_browser_host.cc:100: void
PepperFlashBrowserHost::OnGetLocalDataRestrictions(
Nit, optional: maybe OnLocalDataRestrictionsRetrieved() or something like that
would be better.

https://codereview.chromium.org/11705003/diff/2001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/pepper/pepper_flash_browser_host.h (right):

https://codereview.chromium.org/11705003/diff/2001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/pepper_flash_browser_host.h:20: class
FlashLSOSettingsHelper;
nit: you could merge the namespace with the one below.

https://codereview.chromium.org/11705003/diff/2001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/pepper_flash_browser_host.h:52:
ppapi::host::ReplyMessageContext reply_context,
Any reason to pass a value instead of a ref?

https://codereview.chromium.org/11705003/diff/9003/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/pepper/flash_lso_settings_helper.cc (right):

https://codereview.chromium.org/11705003/diff/9003/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/flash_lso_settings_helper.cc:26:
PP_FLASHLSORESTRICTIONS_NONE;
This value is returned on failure. It indicates "no restrictions". Is that
intended?

https://codereview.chromium.org/11705003/diff/9003/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/flash_lso_settings_helper.cc:47: // pointer
from the UI thread and then call |GetLocalDataRestrictions| with
GetLocalDataRestrictions -> GetLocalDataRestrictionsHelper.

https://codereview.chromium.org/11705003/diff/9003/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/flash_lso_settings_helper.cc:114: // Hold a
ref to ourselves until the profile is destroyed so that we always
Why we want this to be alive for so long?
This helper object is not shared by multiple flash_browser_host objects.

How about posting a task to unregister when the host is dead? (Bind() will
retain a ref, and after un-registration it is likely that ref count will reach
0.)

https://codereview.chromium.org/11705003/diff/9003/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/flash_lso_settings_helper.cc:130:
base::Bind(&FlashLSOSettingsHelper::ClearResourceContext, this));
[I haven't checked. Just want to make sure you have considered that.] so the
profile will post a task to the IO thread to destroy the resource context
*after* this, right?
Otherwise we still have a race.

https://codereview.chromium.org/11705003/diff/9003/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/pepper/flash_lso_settings_helper.h (right):

https://codereview.chromium.org/11705003/diff/9003/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/flash_lso_settings_helper.h:32: // Get the
Flash local data restrictions for the given document/plugin URLs.
Get->Gets (and some other places in this file).

https://codereview.chromium.org/11705003/diff/9003/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/flash_lso_settings_helper.h:39:
base::Callback<void(PP_FlashLSORestrictions)> callback);
Usually we pass const & for callback, right?

https://codereview.chromium.org/11705003/diff/9003/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/flash_lso_settings_helper.h:50:
base::Callback<void(PP_FlashLSORestrictions)> callback,
ditto.

[raymes, 2012-12-29 03:21:56]

https://codereview.chromium.org/11705003/diff/9003/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/pepper/flash_lso_settings_helper.cc (right):

https://codereview.chromium.org/11705003/diff/9003/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/flash_lso_settings_helper.cc:26:
PP_FLASHLSORESTRICTIONS_NONE;
Yes - I actually checked this with bauerb. His response (summarized):
I think defaulting to allow is fine here (in the worst case, we behave like
NPAPI Flash), but I'm looping chrome-privacy in, in case anybody wants to chime
in.

(and nobody chimed in)

https://codereview.chromium.org/11705003/diff/9003/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/flash_lso_settings_helper.cc:47: // pointer
from the UI thread and then call |GetLocalDataRestrictions| with
On 2012/12/29 02:04:05, yzshen1 wrote:
> GetLocalDataRestrictions -> GetLocalDataRestrictionsHelper.

Done.

https://codereview.chromium.org/11705003/diff/9003/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/flash_lso_settings_helper.cc:114: // Hold a
ref to ourselves until the profile is destroyed so that we always
Good idea. Let me know if my change was along the lines of what you were
thinking.

https://codereview.chromium.org/11705003/diff/9003/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/flash_lso_settings_helper.cc:130:
base::Bind(&FlashLSOSettingsHelper::ClearResourceContext, this));
Yes - I made sure this is the case.

https://codereview.chromium.org/11705003/diff/9003/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/pepper/flash_lso_settings_helper.h (right):

https://codereview.chromium.org/11705003/diff/9003/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/flash_lso_settings_helper.h:32: // Get the
Flash local data restrictions for the given document/plugin URLs.
On 2012/12/29 02:04:05, yzshen1 wrote:
> Get->Gets (and some other places in this file).

Done.

https://codereview.chromium.org/11705003/diff/9003/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/flash_lso_settings_helper.h:39:
base::Callback<void(PP_FlashLSORestrictions)> callback);
On 2012/12/29 02:04:05, yzshen1 wrote:
> Usually we pass const & for callback, right?

Done.

https://codereview.chromium.org/11705003/diff/9003/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/flash_lso_settings_helper.h:50:
base::Callback<void(PP_FlashLSORestrictions)> callback,
On 2012/12/29 02:04:05, yzshen1 wrote:
> ditto.

Done.

[yzshen1, 2012-12-29 19:13:40]

LGTM with a few nits.

(I noticed that there are a few comments on patchset 2 you didn't reply. But
those are nits and optional.)

https://codereview.chromium.org/11705003/diff/13002/chrome/browser/renderer_h...
File chrome/browser/renderer_host/pepper/pepper_flash_browser_host.cc (right):

https://codereview.chromium.org/11705003/diff/13002/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_flash_browser_host.cc:44:
content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
Please comment on why this is necessary.
This will avoid the situation that lso_settings_helper_ is destroyed on the IO
thread while the notification arrives at the UI thread.

[raymes, 2012-12-29 21:15:40]

Sorry I missed the comments on the earlier patchset. Addressed them now.

https://codereview.chromium.org/11705003/diff/2001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/pepper/pepper_flash_browser_host.cc (right):

https://codereview.chromium.org/11705003/diff/2001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/pepper_flash_browser_host.cc:100: void
PepperFlashBrowserHost::OnGetLocalDataRestrictions(
On 2012/12/29 02:04:05, yzshen1 wrote:
> Nit, optional: maybe OnLocalDataRestrictionsRetrieved() or something like that
> would be better.

Done.

https://codereview.chromium.org/11705003/diff/2001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/pepper/pepper_flash_browser_host.h (right):

https://codereview.chromium.org/11705003/diff/2001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/pepper_flash_browser_host.h:20: class
FlashLSOSettingsHelper;
On 2012/12/29 02:04:05, yzshen1 wrote:
> nit: you could merge the namespace with the one below.

Done.

https://codereview.chromium.org/11705003/diff/2001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/pepper_flash_browser_host.h:52:
ppapi::host::ReplyMessageContext reply_context,
On 2012/12/29 02:04:05, yzshen1 wrote:
> Any reason to pass a value instead of a ref?

Done.

https://codereview.chromium.org/11705003/diff/13002/chrome/browser/renderer_h...
File chrome/browser/renderer_host/pepper/pepper_flash_browser_host.cc (right):

https://codereview.chromium.org/11705003/diff/13002/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_flash_browser_host.cc:44:
content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
On 2012/12/29 19:13:40, yzshen1 wrote:
> Please comment on why this is necessary.
> This will avoid the situation that lso_settings_helper_ is destroyed on the IO
> thread while the notification arrives at the UI thread.

Done.

https://codereview.chromium.org/11705003/diff/13002/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_flash_browser_host.cc:44:
content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
On 2012/12/29 19:13:40, yzshen1 wrote:
> Please comment on why this is necessary.
> This will avoid the situation that lso_settings_helper_ is destroyed on the IO
> thread while the notification arrives at the UI thread.

Done.

[raymes, 2012-12-29 21:16:50]

TBR=brettw for chrome_browser.gypi

[commit-bot: I haz the power, 2012-12-29 21:17:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/11705003/20003

[Bernhard Bauer, 2012-12-30 00:32:57]

Drive-by review!

I think this can be done a lot easier. You live in chrome/, so you don't need to
use the ContentBrowserClient interface (in fact, you can remove these methods
now and inline their implementations in ChromeContentBrowserClient), and you
don't need to use a ResourceContext, but you can instead directly use the
CookieSettings, which can be accessed even after the profile has been shut down
(by keeping a reference to it).

[raymes, 2012-12-30 02:01:04]

Ah it makes sense now! Thanks!!

I updated the patchset. PTAL.

[Bernhard Bauer, 2012-12-30 02:19:00]

LGTM w/ one additional DCHECK:

https://codereview.chromium.org/11705003/diff/26004/chrome/browser/renderer_h...
File chrome/browser/renderer_host/pepper/pepper_flash_browser_host.cc (right):

https://codereview.chromium.org/11705003/diff/26004/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_flash_browser_host.cc:143:
cookie_settings_ = cookie_settings;
Could you DCHECK that these variables are equal in the other case?

[raymes, 2012-12-30 05:39:19]

https://codereview.chromium.org/11705003/diff/26004/chrome/browser/renderer_h...
File chrome/browser/renderer_host/pepper/pepper_flash_browser_host.cc (right):

https://codereview.chromium.org/11705003/diff/26004/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_flash_browser_host.cc:143:
cookie_settings_ = cookie_settings;
On 2012/12/30 02:19:00, Bernhard Bauer wrote:
> Could you DCHECK that these variables are equal in the other case?

Done.

[commit-bot: I haz the power, 2012-12-30 21:42:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/11705003/35001

[commit-bot: I haz the power, 2012-12-30 21:42:32]

Presubmit check for 11705003-35001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/public/browser

Presubmit checks took 1.3s to calculate.

[commit-bot: I haz the power, 2012-12-30 21:42:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/11705003/35001

[commit-bot: I haz the power, 2012-12-30 21:43:02]

Presubmit check for 11705003-35001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/public/browser

Presubmit checks took 1.1s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[commit-bot: I haz the power, 2012-12-30 21:46:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/11705003/35001