Stop using GetDefaultProfile() in Chrome OS implementation of platform_util::OpenExternal()

Stop using GetDefaultProfile() in Chrome OS implementation of platform_util::OpenExternal()

Add Profile* as an argument of OpenExternal().
Disallow calling OpenExternal() from threads other than UI thread.

Changes for the implementations:
 Chrome OS implementation: Use the argument Profile and stop posting tasks to UI thread.
 Win implementation: Post tasks to FILE thread. (for the reason noted in external_protocol_handler.cc)
 Other implementations: Just add Profile* argument and add thread check.

Changes for user code:
 1. first_run_dialog.cc: Just pass Profile*.
 2. browser_commands.cc: Pass Profile* acquired from Browser.
 3. chrome_shell_window_delegate.cc: Pass Profile* acquired from WebContents.
 4. external_protocol_handler.cc: Pass Profile* acquired with a pair of render_process_host_id and tab_contents_id.

BUG=322682
TEST=git cl try

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

[hashimoto, 2013-12-10 14:08:56]

sky@,
Could you review this change?

[sky, 2013-12-10 17:32:10]

https://codereview.chromium.org/107033003/diff/140001/chrome/browser/external...
File chrome/browser/external_protocol/external_protocol_handler.cc (right):

https://codereview.chromium.org/107033003/diff/140001/chrome/browser/external...
chrome/browser/external_protocol/external_protocol_handler.cc:297:
content::WebContents* web_contents = tab_util::GetWebContentsByID(
Can't this code be passed the Profile?

[hashimoto, 2013-12-11 08:57:50]

https://codereview.chromium.org/107033003/diff/140001/chrome/browser/external...
File chrome/browser/external_protocol/external_protocol_handler.cc (right):

https://codereview.chromium.org/107033003/diff/140001/chrome/browser/external...
chrome/browser/external_protocol/external_protocol_handler.cc:297:
content::WebContents* web_contents = tab_util::GetWebContentsByID(
On 2013/12/10 17:32:11, sky wrote:
> Can't this code be passed the Profile?

I preferred receiving a pair of int to Profile* because:

1. All users of this function are receiving a pair of int.
 This function is called in a callback chain starting from
ChromeResourceDispatcherHostDelegate::HandleExternalProtocol() who posts a task
to run ExternalProtocolHandler::LaunchUrl().
 ExternalProtocolHandler::LaunchUrl() receives a pair of int, and other
external_protocol_dialog related codes are also passing a pair of int around.
 Passing a pair of int as arguments here looks consistent with neighbors. (and
user codes do not need to call tab_util::GetWebContentsByID by themselves)

2. This function is called asynchronously.
 This function is called asynchronously by CreateShellWorker (instantiated in
LaunchUrlWithDelegate() above) or external_protocol_dialog UI code.
 I think calling GetWebContentsByID() and perform NULL check against its result
here is safer than doing it earlier and using the result raw pointer of Profile
later.

I might be too nervous about #2 because it seems ProfileManager does not delete
Profile until it gets destroyed in
ChromeBrowserMainParts::PostMainMessageLoopRun().
If it's OK to stop worrying about use-after-free of Profile*, I will change this
file to call GetWebContentsByID() in
ExternalProtocolHandler::LaunchUrlWithDelegate() and pass a raw pointer of
Profile to this function.

[sky, 2013-12-11 15:19:24]

LGTM

https://codereview.chromium.org/107033003/diff/140001/chrome/browser/external...
File chrome/browser/external_protocol/external_protocol_handler.cc (right):

https://codereview.chromium.org/107033003/diff/140001/chrome/browser/external...
chrome/browser/external_protocol/external_protocol_handler.cc:297:
content::WebContents* web_contents = tab_util::GetWebContentsByID(
On 2013/12/11 08:57:51, hashimoto wrote:
> On 2013/12/10 17:32:11, sky wrote:
> > Can't this code be passed the Profile?
> 
> I preferred receiving a pair of int to Profile* because:
> 
> 1. All users of this function are receiving a pair of int.
>  This function is called in a callback chain starting from
> ChromeResourceDispatcherHostDelegate::HandleExternalProtocol() who posts a
task
> to run ExternalProtocolHandler::LaunchUrl().
>  ExternalProtocolHandler::LaunchUrl() receives a pair of int, and other
> external_protocol_dialog related codes are also passing a pair of int around.
>  Passing a pair of int as arguments here looks consistent with neighbors. (and
> user codes do not need to call tab_util::GetWebContentsByID by themselves)
> 
> 2. This function is called asynchronously.
>  This function is called asynchronously by CreateShellWorker (instantiated in
> LaunchUrlWithDelegate() above) or external_protocol_dialog UI code.
>  I think calling GetWebContentsByID() and perform NULL check against its
result
> here is safer than doing it earlier and using the result raw pointer of
Profile
> later.
> 
> I might be too nervous about #2 because it seems ProfileManager does not
delete
> Profile until it gets destroyed in
> ChromeBrowserMainParts::PostMainMessageLoopRun().
> If it's OK to stop worrying about use-after-free of Profile*, I will change
this
> file to call GetWebContentsByID() in
> ExternalProtocolHandler::LaunchUrlWithDelegate() and pass a raw pointer of
> Profile to this function.

If this goes through different threads, keep what you have here.

https://codereview.chromium.org/107033003/diff/140001/chrome/browser/platform...
File chrome/browser/platform_util_chromeos.cc (right):

https://codereview.chromium.org/107033003/diff/140001/chrome/browser/platform...
chrome/browser/platform_util_chromeos.cc:55: if (url.is_valid())
Under what conditions can we get here with an invalid url? Shouldn't this check
be in the call site?

[hashimoto, 2013-12-12 05:55:57]

https://codereview.chromium.org/107033003/diff/140001/chrome/browser/platform...
File chrome/browser/platform_util_chromeos.cc (right):

https://codereview.chromium.org/107033003/diff/140001/chrome/browser/platform...
chrome/browser/platform_util_chromeos.cc:55: if (url.is_valid())
On 2013/12/11 15:19:25, sky wrote:
> Under what conditions can we get here with an invalid url? Shouldn't this
check
> be in the call site?

Among all 4 users of this function:
1. first_run_dialog.cc: Passing a constant URL.
2. browser_commands.cc: Passing a mailto: URL made by the caller. 
3. chrome_shell_window_delegate.cc: Cannot be sure. Too many call sites.
4. external_protocol_handler.cc: Invalid URL should not be passed because
job_factory->IsHandledURL(url) returns true for invalid URLs in
ResourceDispatcherHostImpl::HandleExternalProtocol()

So chrome_shell_window_delegate.cc is the only user who may pass invalid URLs
for this function.
But from what I see from the CL which introduced this check
(http://crrev.com/209751), it seems safe to remove this check.
It should not be dangerous to pass invalid URLs to chrome::Navigate anyways.

[commit-bot: I haz the power, 2013-12-12 05:56:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/107033003/200001

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

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

[commit-bot: I haz the power, 2013-12-12 13:42:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/107033003/200001

[sky, 2013-12-12 17:14:52]

In that case how about changing it to a DCHECK?

On Wed, Dec 11, 2013 at 9:55 PM,  <hashimoto@chromium.org> wrote:
>
>
https://codereview.chromium.org/107033003/diff/140001/chrome/browser/platform...
> File chrome/browser/platform_util_chromeos.cc (right):
>
>
https://codereview.chromium.org/107033003/diff/140001/chrome/browser/platform...
> chrome/browser/platform_util_chromeos.cc:55: if (url.is_valid())
> On 2013/12/11 15:19:25, sky wrote:
>>
>> Under what conditions can we get here with an invalid url? Shouldn't
>
> this check
>>
>> be in the call site?
>
>
> Among all 4 users of this function:
> 1. first_run_dialog.cc: Passing a constant URL.
> 2. browser_commands.cc: Passing a mailto: URL made by the caller.
> 3. chrome_shell_window_delegate.cc: Cannot be sure. Too many call sites.
> 4. external_protocol_handler.cc: Invalid URL should not be passed
> because job_factory->IsHandledURL(url) returns true for invalid URLs in
> ResourceDispatcherHostImpl::HandleExternalProtocol()
>
> So chrome_shell_window_delegate.cc is the only user who may pass invalid
> URLs for this function.
> But from what I see from the CL which introduced this check
> (http://crrev.com/209751), it seems safe to remove this check.
> It should not be dangerous to pass invalid URLs to chrome::Navigate
> anyways.
>
> https://codereview.chromium.org/107033003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

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

Change committed as 240346