Add service registration for apps APIs implemented as mojo services.

Add service registration for apps APIs implemented as mojo services.

This adds a ServiceRegistrationManager that acts as a global repository
of extensions API services to be added to RenderFrameHosts along with
the extensions API permissions that gate access to them.

BUG=389016
Committed: https://crrev.com/d92097b170dd8bf44cbef31fa5fae5d523d1c651
Cr-Commit-Position: refs/heads/master@{#301310}

[Sam McNally, 2014-10-13 20:49:18]

sammc@chromium.org changed reviewers:
+ raymes@chromium.org

[raymes, 2014-10-15 20:22:50]

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/exte...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/exte...
extensions/browser/extension_web_contents_observer.cc:33: if
(known_render_frame_hosts_.count(render_frame_host))
It seems like a bug that this might be called twice. We should look at why.

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/serv...
File extensions/browser/service_registration_manager.cc (right):

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/serv...
extensions/browser/service_registration_manager.cc:68: extension,
render_frame_host->GetProcess()->GetID());
I don't really understand the details of this. Is there something we have to be
worried about here or will this always get the right context type?

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/serv...
extensions/browser/service_registration_manager.cc:71: // pass |site_url| as the
URL for permission checking. This is sufficient
What other URL would we pass? I'm not quite sure I understand what the general
case would look like.

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/serv...
extensions/browser/service_registration_manager.cc:76:
->IsAvailable(factory.first, extension, context_type, site_url)
Is it possible for the security context to of a RenderFrame to change? Could it
be navigated to a different URL and we decide we no longer want to grant
permissions there? My guess is that the answer is no, but just wanted to check
:)

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/serv...
File extensions/browser/service_registration_manager.h (right):

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/serv...
extensions/browser/service_registration_manager.h:24: class ServiceFactoryBase {
Maybe add a comment explaining what this is for and why you use this pattern.

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/serv...
extensions/browser/service_registration_manager.h:82: void
AddToRenderFrame(content::RenderFrameHost* render_frame_host);
nit: AddServicesToRenderFrame might be more descriptive here.

[raymes, 2014-10-15 20:23:06]

raymes@chromium.org changed reviewers:
+ rockot@chromium.org

[Ken Rockot(use gerrit already), 2014-10-15 21:45:27]

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/exte...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/exte...
extensions/browser/extension_web_contents_observer.cc:33: if
(known_render_frame_hosts_.count(render_frame_host))
On 2014/10/15 20:22:49, raymes wrote:
> It seems like a bug that this might be called twice. We should look at why.

Are we sure this really happens? It seems impossible unless main frame creation
elicits two calls (contrary to the comments in WebContentsImpl), but I think
that would have to break lots of other things.

[Sam McNally, 2014-10-15 22:52:56]

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/exte...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/exte...
extensions/browser/extension_web_contents_observer.cc:33: if
(known_render_frame_hosts_.count(render_frame_host))
On 2014/10/15 21:45:27, Ken Rockot wrote:
> On 2014/10/15 20:22:49, raymes wrote:
> > It seems like a bug that this might be called twice. We should look at why.
> 
> Are we sure this really happens? It seems impossible unless main frame
creation
> elicits two calls (contrary to the comments in WebContentsImpl), but I think
> that would have to break lots of other things.

I put in a check and logging of stack traces and RenderFrameHost* and here's
what I got:
[6948:1287:1015/153255:INFO:extension_web_contents_observer.cc(34)] 0x7bcebf00
0   libbase.dylib                       0x0cc747ff
base::debug::StackTrace::StackTrace() + 63
1   libbase.dylib                       0x0cc7485b
base::debug::StackTrace::StackTrace() + 43
2   libchrome_main_dll.dylib            0x046bb8f5
extensions::ExtensionWebContentsObserver::RenderFrameCreated(content::RenderFrameHost*)
+ 581
3   libcontent.dylib                    0x1c6eb929
content::WebContentsImpl::RenderViewCreated(content::RenderViewHost*) + 761
4   libcontent.dylib                    0x1c6eb9df non-virtual thunk to
content::WebContentsImpl::RenderViewCreated(content::RenderViewHost*) + 63
5   libcontent.dylib                    0x1c3bbf72
content::RenderViewHostImpl::CreateRenderView(std::basic_string<unsigned short,
base::string16_char_traits, std::allocator<unsigned short> > const&, int, int,
int, bool) + 2722
6   libcontent.dylib                    0x1c6ef130
content::WebContentsImpl::CreateRenderViewForRenderManager(content::RenderViewHost*,
int, int, bool) + 880
7   libcontent.dylib                    0x1c6ef230 non-virtual thunk to
content::WebContentsImpl::CreateRenderViewForRenderManager(content::RenderViewHost*,
int, int, bool) + 112
8   libcontent.dylib                    0x1be572e3
content::RenderFrameHostManager::InitRenderView(content::RenderViewHostImpl*,
int, int, bool) + 643
9   libcontent.dylib                    0x1be55dd4
content::RenderFrameHostManager::Navigate(content::NavigationEntryImpl const&) +
996
10  libcontent.dylib                    0x1be18f99
content::NavigatorImpl::NavigateToEntry(content::RenderFrameHostImpl*,
content::NavigationEntryImpl const&, content::NavigationController::ReloadType)
+ 1209
11  libcontent.dylib                    0x1be1a50a
content::NavigatorImpl::NavigateToPendingEntry(content::RenderFrameHostImpl*,
content::NavigationController::ReloadType) + 106
12  libcontent.dylib                    0x1c6e2af3
content::WebContentsImpl::NavigateToPendingEntry(content::NavigationController::ReloadType)
+ 563
13  libcontent.dylib                    0x1c6e2b6f non-virtual thunk to
content::WebContentsImpl::NavigateToPendingEntry(content::NavigationController::ReloadType)
+ 63
14  libcontent.dylib                    0x1bdf96fb
content::NavigationControllerImpl::NavigateToPendingEntry(content::NavigationController::ReloadType)
+ 859
15  libcontent.dylib                    0x1bdf9d14
content::NavigationControllerImpl::LoadEntry(content::NavigationEntryImpl*) + 84
16  libcontent.dylib                    0x1bdfc150
content::NavigationControllerImpl::LoadURLWithParams(content::NavigationController::LoadURLParams
const&) + 2720
17  libcontent.dylib                    0x1bdfb671
content::NavigationControllerImpl::LoadURL(GURL const&, content::Referrer
const&, ui::PageTransition, std::string const&) + 241
18  libchrome_main_dll.dylib            0x046640e0
extensions::ExtensionHost::LoadInitialURL() + 160
19  libchrome_main_dll.dylib            0x04663e72
extensions::ExtensionHost::CreateRenderViewNow() + 66
20  libchrome_main_dll.dylib            0x04667c79
extensions::ExtensionHost::ProcessCreationQueue::ProcessOneHost() + 89
21  libchrome_main_dll.dylib            0x04668558
base::internal::RunnableAdapter<void
(extensions::ExtensionHost::ProcessCreationQueue::*)()>::Run(extensions::ExtensionHost::ProcessCreationQueue*)
+ 120
22  libchrome_main_dll.dylib            0x046683fc
base::internal::InvokeHelper<true, void, base::internal::RunnableAdapter<void
(extensions::ExtensionHost::ProcessCreationQueue::*)()>, void
(base::WeakPtr<extensions::ExtensionHost::ProcessCreationQueue>
const&)>::MakeItSo(base::internal::RunnableAdapter<void
(extensions::ExtensionHost::ProcessCreationQueue::*)()>,
base::WeakPtr<extensions::ExtensionHost::ProcessCreationQueue> const&) + 92
23  libchrome_main_dll.dylib            0x0466833c base::internal::Invoker<1,
base::internal::BindState<base::internal::RunnableAdapter<void
(extensions::ExtensionHost::ProcessCreationQueue::*)()>, void
(extensions::ExtensionHost::ProcessCreationQueue*), void
(base::WeakPtr<extensions::ExtensionHost::ProcessCreationQueue>)>, void
(extensions::ExtensionHost::ProcessCreationQueue*)>::Run(base::internal::BindStateBase*)
+ 108
24  libbase.dylib                       0x0cc5da8f base::Callback<void
()>::Run() const + 63
25  libbase.dylib                       0x0cc76cfc
base::debug::TaskAnnotator::RunTask(char const*, char const*, base::PendingTask
const&) + 1084
26  libbase.dylib                       0x0cd65919
base::MessageLoop::RunTask(base::PendingTask const&) + 793
27  libbase.dylib                       0x0cd65ac2
base::MessageLoop::DeferOrRunPendingTask(base::PendingTask const&) + 98
28  libbase.dylib                       0x0cd65d91 base::MessageLoop::DoWork() +
321
29  libbase.dylib                       0x0cc35a55
base::MessagePumpCFRunLoopBase::RunWork() + 101
30  libbase.dylib                       0x0cc34e71
base::MessagePumpCFRunLoopBase::RunWorkSource(void*) + 49
31  CoreFoundation                      0x9b38db5f
__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 15
32  CoreFoundation                      0x9b37e95b __CFRunLoopDoSources0 + 235
33  CoreFoundation                      0x9b37e05e __CFRunLoopRun + 1022
34  CoreFoundation                      0x9b37d9ea CFRunLoopRunSpecific + 394
35  CoreFoundation                      0x9b37d84b CFRunLoopRunInMode + 123
36  HIToolbox                           0x90033b5d RunCurrentEventLoopInMode +
259
37  HIToolbox                           0x900338e2 ReceiveNextEventCommon + 526
38  HIToolbox                           0x900336bd
_BlockUntilNextEventMatchingListInModeWithFilter + 92
39  AppKit                              0x913e2349 _DPSNextEvent + 1602
40  AppKit                              0x913e1870 -[NSApplication
nextEventMatchingMask:untilDate:inMode:dequeue:] + 119
41  AppKit                              0x913d415c -[NSApplication run] + 727
42  libbase.dylib                       0x0cc36960
base::MessagePumpNSApplication::DoRun(base::MessagePump::Delegate*) + 368
43  libbase.dylib                       0x0cc35668
base::MessagePumpCFRunLoopBase::Run(base::MessagePump::Delegate*) + 104
44  libbase.dylib                       0x0cd6528c
base::MessageLoop::RunHandler() + 300
45  libbase.dylib                       0x0cdd6c63 base::RunLoop::Run() + 83
46  libchrome_main_dll.dylib            0x012f49ba (anonymous
namespace)::SessionRestoreImpl::Restore() + 682
47  libchrome_main_dll.dylib            0x012f45ea
SessionRestore::RestoreSession(Profile*, Browser*, chrome::HostDesktopType,
unsigned int, std::vector<GURL, std::allocator<GURL> > const&) + 794
48  libchrome_main_dll.dylib            0x03250c3c
StartupBrowserCreatorImpl::ProcessStartupURLs(std::vector<GURL,
std::allocator<GURL> > const&, chrome::HostDesktopType) + 2060
49  libchrome_main_dll.dylib            0x0324fba2
StartupBrowserCreatorImpl::ProcessLaunchURLs(bool, std::vector<GURL,
std::allocator<GURL> > const&, chrome::HostDesktopType) + 466
50  libchrome_main_dll.dylib            0x0324f0af
StartupBrowserCreatorImpl::Launch(Profile*, std::vector<GURL,
std::allocator<GURL> > const&, bool, chrome::HostDesktopType) + 1375
51  libchrome_main_dll.dylib            0x03244cb0
StartupBrowserCreator::LaunchBrowser(base::CommandLine const&, Profile*,
base::FilePath const&, chrome::startup::IsProcessStartup,
chrome::startup::IsFirstRun, int*) + 1216
52  libchrome_main_dll.dylib            0x03246e60
StartupBrowserCreator::ProcessCmdLineImpl(base::CommandLine const&,
base::FilePath const&, bool, Profile*, std::vector<Profile*,
std::allocator<Profile*> > const&, int*, StartupBrowserCreator*) + 4976
53  libchrome_main_dll.dylib            0x0039b9d8
StartupBrowserCreator::Start(base::CommandLine const&, base::FilePath const&,
Profile*, std::vector<Profile*, std::allocator<Profile*> > const&, int*) + 136
54  libchrome_main_dll.dylib            0x00397768
ChromeBrowserMainParts::PreMainMessageLoopRunImpl() + 4664
55  libchrome_main_dll.dylib            0x0039649e
ChromeBrowserMainParts::PreMainMessageLoopRun() + 334
56  libcontent.dylib                    0x1bb72af9
content::BrowserMainLoop::PreMainMessageLoopRun() + 377
57  libcontent.dylib                    0x1bb7c378
base::internal::RunnableAdapter<int
(content::BrowserMainLoop::*)()>::Run(content::BrowserMainLoop*) + 120
58  libcontent.dylib                    0x1bb7c294
base::internal::InvokeHelper<false, int, base::internal::RunnableAdapter<int
(content::BrowserMainLoop::*)()>, void
(content::BrowserMainLoop*)>::MakeItSo(base::internal::RunnableAdapter<int
(content::BrowserMainLoop::*)()>, content::BrowserMainLoop*) + 68
59  libcontent.dylib                    0x1bb7c1d4 base::internal::Invoker<1,
base::internal::BindState<base::internal::RunnableAdapter<int
(content::BrowserMainLoop::*)()>, int (content::BrowserMainLoop*), void
(base::internal::UnretainedWrapper<content::BrowserMainLoop>)>, int
(content::BrowserMainLoop*)>::Run(base::internal::BindStateBase*) + 116
60  libcontent.dylib                    0x1c1eb76f base::Callback<int ()>::Run()
const + 63
61  libcontent.dylib                    0x1c66de28
content::StartupTaskRunner::RunAllTasksNow() + 120
[6948:1287:1015/153256:INFO:extension_web_contents_observer.cc(34)] 0x7bdc62e0
0   libbase.dylib                       0x0cc747ff
base::debug::StackTrace::StackTrace() + 63
1   libbase.dylib                       0x0cc7485b
base::debug::StackTrace::StackTrace() + 43
2   libchrome_main_dll.dylib            0x046bb8f5
extensions::ExtensionWebContentsObserver::RenderFrameCreated(content::RenderFrameHost*)
+ 581
3   libcontent.dylib                    0x1c6eb929
content::WebContentsImpl::RenderViewCreated(content::RenderViewHost*) + 761
4   libcontent.dylib                    0x1c6eb9df non-virtual thunk to
content::WebContentsImpl::RenderViewCreated(content::RenderViewHost*) + 63
5   libcontent.dylib                    0x1c3bbf72
content::RenderViewHostImpl::CreateRenderView(std::basic_string<unsigned short,
base::string16_char_traits, std::allocator<unsigned short> > const&, int, int,
int, bool) + 2722
6   libcontent.dylib                    0x1c6ef130
content::WebContentsImpl::CreateRenderViewForRenderManager(content::RenderViewHost*,
int, int, bool) + 880
7   libcontent.dylib                    0x1c6ef230 non-virtual thunk to
content::WebContentsImpl::CreateRenderViewForRenderManager(content::RenderViewHost*,
int, int, bool) + 112
8   libcontent.dylib                    0x1be572e3
content::RenderFrameHostManager::InitRenderView(content::RenderViewHostImpl*,
int, int, bool) + 643
9   libcontent.dylib                    0x1be5c8f0
content::RenderFrameHostManager::CreateRenderFrame(content::SiteInstance*, int,
bool, bool, bool) + 2144
10  libcontent.dylib                    0x1be5c03d
content::RenderFrameHostManager::CreateRenderFrameHostForNewSiteInstance(content::SiteInstance*,
content::SiteInstance*, bool) + 365
11  libcontent.dylib                    0x1be5652b
content::RenderFrameHostManager::UpdateStateForNavigate(GURL const&,
content::SiteInstance*, ui::PageTransition, bool, bool, content::GlobalRequestID
const&, int) + 1115
12  libcontent.dylib                    0x1be55c52
content::RenderFrameHostManager::Navigate(content::NavigationEntryImpl const&) +
610
13  libcontent.dylib                    0x1be18f99
content::NavigatorImpl::NavigateToEntry(content::RenderFrameHostImpl*,
content::NavigationEntryImpl const&, content::NavigationController::ReloadType)
+ 1209
14  libcontent.dylib                    0x1be1a50a
content::NavigatorImpl::NavigateToPendingEntry(content::RenderFrameHostImpl*,
content::NavigationController::ReloadType) + 106
15  libcontent.dylib                    0x1c6e2af3
content::WebContentsImpl::NavigateToPendingEntry(content::NavigationController::ReloadType)
+ 563
16  libcontent.dylib                    0x1c6e2b6f non-virtual thunk to
content::WebContentsImpl::NavigateToPendingEntry(content::NavigationController::ReloadType)
+ 63
17  libcontent.dylib                    0x1bdf96fb
content::NavigationControllerImpl::NavigateToPendingEntry(content::NavigationController::ReloadType)
+ 859
18  libcontent.dylib                    0x1bdf9d14
content::NavigationControllerImpl::LoadEntry(content::NavigationEntryImpl*) + 84
19  libcontent.dylib                    0x1bdfc150
content::NavigationControllerImpl::LoadURLWithParams(content::NavigationController::LoadURLParams
const&) + 2720
20  libchrome_main_dll.dylib            0x02c93906 (anonymous
namespace)::LoadURLInContents(content::WebContents*, GURL const&,
chrome::NavigateParams*) + 550
21  libchrome_main_dll.dylib            0x02c922af
chrome::Navigate(chrome::NavigateParams*) + 2479
22  libchrome_main_dll.dylib            0x012f8b6e (anonymous
namespace)::SessionRestoreImpl::AppendURLsToBrowser(Browser*, std::vector<GURL,
std::allocator<GURL> > const&) + 286
23  libchrome_main_dll.dylib            0x012f8812 (anonymous
namespace)::SessionRestoreImpl::FinishedTabCreation(bool, bool) + 466
24  libchrome_main_dll.dylib            0x012fdee7 (anonymous
namespace)::SessionRestoreImpl::ProcessSessionWindows(std::vector<SessionWindow*,
std::allocator<SessionWindow*> >*, int) + 951
25  libchrome_main_dll.dylib            0x012f4a49 (anonymous
namespace)::SessionRestoreImpl::Restore() + 825
26  libchrome_main_dll.dylib            0x012f45ea
SessionRestore::RestoreSession(Profile*, Browser*, chrome::HostDesktopType,
unsigned int, std::vector<GURL, std::allocator<GURL> > const&) + 794
27  libchrome_main_dll.dylib            0x03250c3c
StartupBrowserCreatorImpl::ProcessStartupURLs(std::vector<GURL,
std::allocator<GURL> > const&, chrome::HostDesktopType) + 2060
28  libchrome_main_dll.dylib            0x0324fba2
StartupBrowserCreatorImpl::ProcessLaunchURLs(bool, std::vector<GURL,
std::allocator<GURL> > const&, chrome::HostDesktopType) + 466
29  libchrome_main_dll.dylib            0x0324f0af
StartupBrowserCreatorImpl::Launch(Profile*, std::vector<GURL,
std::allocator<GURL> > const&, bool, chrome::HostDesktopType) + 1375
30  libchrome_main_dll.dylib            0x03244cb0
StartupBrowserCreator::LaunchBrowser(base::CommandLine const&, Profile*,
base::FilePath const&, chrome::startup::IsProcessStartup,
chrome::startup::IsFirstRun, int*) + 1216
31  libchrome_main_dll.dylib            0x03246e60
StartupBrowserCreator::ProcessCmdLineImpl(base::CommandLine const&,
base::FilePath const&, bool, Profile*, std::vector<Profile*,
std::allocator<Profile*> > const&, int*, StartupBrowserCreator*) + 4976
32  libchrome_main_dll.dylib            0x0039b9d8
StartupBrowserCreator::Start(base::CommandLine const&, base::FilePath const&,
Profile*, std::vector<Profile*, std::allocator<Profile*> > const&, int*) + 136
33  libchrome_main_dll.dylib            0x00397768
ChromeBrowserMainParts::PreMainMessageLoopRunImpl() + 4664
34  libchrome_main_dll.dylib            0x0039649e
ChromeBrowserMainParts::PreMainMessageLoopRun() + 334
35  libcontent.dylib                    0x1bb72af9
content::BrowserMainLoop::PreMainMessageLoopRun() + 377
36  libcontent.dylib                    0x1bb7c378
base::internal::RunnableAdapter<int
(content::BrowserMainLoop::*)()>::Run(content::BrowserMainLoop*) + 120
37  libcontent.dylib                    0x1bb7c294
base::internal::InvokeHelper<false, int, base::internal::RunnableAdapter<int
(content::BrowserMainLoop::*)()>, void
(content::BrowserMainLoop*)>::MakeItSo(base::internal::RunnableAdapter<int
(content::BrowserMainLoop::*)()>, content::BrowserMainLoop*) + 68
38  libcontent.dylib                    0x1bb7c1d4 base::internal::Invoker<1,
base::internal::BindState<base::internal::RunnableAdapter<int
(content::BrowserMainLoop::*)()>, int (content::BrowserMainLoop*), void
(base::internal::UnretainedWrapper<content::BrowserMainLoop>)>, int
(content::BrowserMainLoop*)>::Run(base::internal::BindStateBase*) + 116
39  libcontent.dylib                    0x1c1eb76f base::Callback<int ()>::Run()
const + 63
40  libcontent.dylib                    0x1c66de28
content::StartupTaskRunner::RunAllTasksNow() + 120
41  libcontent.dylib                    0x1bb7064f
content::BrowserMainLoop::CreateStartupTasks() + 1279
42  libcontent.dylib                    0x1bb84dc6
content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&) +
822
43  libcontent.dylib                    0x1bb6c7ce
content::BrowserMain(content::MainFunctionParams const&) + 254
44  libcontent.dylib                    0x1b9bac5c
content::RunNamedProcessTypeMain(std::string const&, content::MainFunctionParams
const&, content::ContentMainDelegate*) + 268
45  libcontent.dylib                    0x1b9bc34b
content::ContentMainRunnerImpl::Run() + 779
46  libcontent.dylib                    0x1b9ba0f5
content::ContentMain(content::ContentMainParams const&) + 133
47  libchrome_main_dll.dylib            0x000adc5c ChromeMain + 92
48  Chromium                            0x0009aacb main + 43
49  Chromium                            0x0009aa95 start + 53
[6948:1287:1015/153256:INFO:extension_web_contents_observer.cc(34)] 0x7bdc62e0
[6948:1287:1015/153256:FATAL:extension_web_contents_observer.cc(36)] Check
failed: inserted.
0   libbase.dylib                       0x0cc747ff
base::debug::StackTrace::StackTrace() + 63
1   libbase.dylib                       0x0cc7485b
base::debug::StackTrace::StackTrace() + 43
2   libbase.dylib                       0x0cd22932
logging::LogMessage::~LogMessage() + 82
3   libbase.dylib                       0x0cd216ab
logging::LogMessage::~LogMessage() + 43
4   libchrome_main_dll.dylib            0x046bb8e7
extensions::ExtensionWebContentsObserver::RenderFrameCreated(content::RenderFrameHost*)
+ 567
5   libcontent.dylib                    0x1c6ea1af
content::WebContentsImpl::RenderFrameCreated(content::RenderFrameHost*) + 143
6   libcontent.dylib                    0x1c6ea23f non-virtual thunk to
content::WebContentsImpl::RenderFrameCreated(content::RenderFrameHost*) + 63
7   libcontent.dylib                    0x1be5cbbc
content::RenderFrameHostManager::CreateRenderFrame(content::SiteInstance*, int,
bool, bool, bool) + 2860
8   libcontent.dylib                    0x1be5c03d
content::RenderFrameHostManager::CreateRenderFrameHostForNewSiteInstance(content::SiteInstance*,
content::SiteInstance*, bool) + 365
9   libcontent.dylib                    0x1be5652b
content::RenderFrameHostManager::UpdateStateForNavigate(GURL const&,
content::SiteInstance*, ui::PageTransition, bool, bool, content::GlobalRequestID
const&, int) + 1115
10  libcontent.dylib                    0x1be55c52
content::RenderFrameHostManager::Navigate(content::NavigationEntryImpl const&) +
610
11  libcontent.dylib                    0x1be18f99
content::NavigatorImpl::NavigateToEntry(content::RenderFrameHostImpl*,
content::NavigationEntryImpl const&, content::NavigationController::ReloadType)
+ 1209
12  libcontent.dylib                    0x1be1a50a
content::NavigatorImpl::NavigateToPendingEntry(content::RenderFrameHostImpl*,
content::NavigationController::ReloadType) + 106
13  libcontent.dylib                    0x1c6e2af3
content::WebContentsImpl::NavigateToPendingEntry(content::NavigationController::ReloadType)
+ 563
14  libcontent.dylib                    0x1c6e2b6f non-virtual thunk to
content::WebContentsImpl::NavigateToPendingEntry(content::NavigationController::ReloadType)
+ 63
15  libcontent.dylib                    0x1bdf96fb
content::NavigationControllerImpl::NavigateToPendingEntry(content::NavigationController::ReloadType)
+ 859
16  libcontent.dylib                    0x1bdf9d14
content::NavigationControllerImpl::LoadEntry(content::NavigationEntryImpl*) + 84
17  libcontent.dylib                    0x1bdfc150
content::NavigationControllerImpl::LoadURLWithParams(content::NavigationController::LoadURLParams
const&) + 2720
18  libchrome_main_dll.dylib            0x02c93906 (anonymous
namespace)::LoadURLInContents(content::WebContents*, GURL const&,
chrome::NavigateParams*) + 550
19  libchrome_main_dll.dylib            0x02c922af
chrome::Navigate(chrome::NavigateParams*) + 2479
20  libchrome_main_dll.dylib            0x012f8b6e (anonymous
namespace)::SessionRestoreImpl::AppendURLsToBrowser(Browser*, std::vector<GURL,
std::allocator<GURL> > const&) + 286
21  libchrome_main_dll.dylib            0x012f8812 (anonymous
namespace)::SessionRestoreImpl::FinishedTabCreation(bool, bool) + 466
22  libchrome_main_dll.dylib            0x012fdee7 (anonymous
namespace)::SessionRestoreImpl::ProcessSessionWindows(std::vector<SessionWindow*,
std::allocator<SessionWindow*> >*, int) + 951
23  libchrome_main_dll.dylib            0x012f4a49 (anonymous
namespace)::SessionRestoreImpl::Restore() + 825
24  libchrome_main_dll.dylib            0x012f45ea
SessionRestore::RestoreSession(Profile*, Browser*, chrome::HostDesktopType,
unsigned int, std::vector<GURL, std::allocator<GURL> > const&) + 794
25  libchrome_main_dll.dylib            0x03250c3c
StartupBrowserCreatorImpl::ProcessStartupURLs(std::vector<GURL,
std::allocator<GURL> > const&, chrome::HostDesktopType) + 2060
26  libchrome_main_dll.dylib            0x0324fba2
StartupBrowserCreatorImpl::ProcessLaunchURLs(bool, std::vector<GURL,
std::allocator<GURL> > const&, chrome::HostDesktopType) + 466
27  libchrome_main_dll.dylib            0x0324f0af
StartupBrowserCreatorImpl::Launch(Profile*, std::vector<GURL,
std::allocator<GURL> > const&, bool, chrome::HostDesktopType) + 1375
28  libchrome_main_dll.dylib            0x03244cb0
StartupBrowserCreator::LaunchBrowser(base::CommandLine const&, Profile*,
base::FilePath const&, chrome::startup::IsProcessStartup,
chrome::startup::IsFirstRun, int*) + 1216
29  libchrome_main_dll.dylib            0x03246e60
StartupBrowserCreator::ProcessCmdLineImpl(base::CommandLine const&,
base::FilePath const&, bool, Profile*, std::vector<Profile*,
std::allocator<Profile*> > const&, int*, StartupBrowserCreator*) + 4976
30  libchrome_main_dll.dylib            0x0039b9d8
StartupBrowserCreator::Start(base::CommandLine const&, base::FilePath const&,
Profile*, std::vector<Profile*, std::allocator<Profile*> > const&, int*) + 136
31  libchrome_main_dll.dylib            0x00397768
ChromeBrowserMainParts::PreMainMessageLoopRunImpl() + 4664
32  libchrome_main_dll.dylib            0x0039649e
ChromeBrowserMainParts::PreMainMessageLoopRun() + 334
33  libcontent.dylib                    0x1bb72af9
content::BrowserMainLoop::PreMainMessageLoopRun() + 377
34  libcontent.dylib                    0x1bb7c378
base::internal::RunnableAdapter<int
(content::BrowserMainLoop::*)()>::Run(content::BrowserMainLoop*) + 120
35  libcontent.dylib                    0x1bb7c294
base::internal::InvokeHelper<false, int, base::internal::RunnableAdapter<int
(content::BrowserMainLoop::*)()>, void
(content::BrowserMainLoop*)>::MakeItSo(base::internal::RunnableAdapter<int
(content::BrowserMainLoop::*)()>, content::BrowserMainLoop*) + 68
36  libcontent.dylib                    0x1bb7c1d4 base::internal::Invoker<1,
base::internal::BindState<base::internal::RunnableAdapter<int
(content::BrowserMainLoop::*)()>, int (content::BrowserMainLoop*), void
(base::internal::UnretainedWrapper<content::BrowserMainLoop>)>, int
(content::BrowserMainLoop*)>::Run(base::internal::BindStateBase*) + 116
37  libcontent.dylib                    0x1c1eb76f base::Callback<int ()>::Run()
const + 63
38  libcontent.dylib                    0x1c66de28
content::StartupTaskRunner::RunAllTasksNow() + 120
39  libcontent.dylib                    0x1bb7064f
content::BrowserMainLoop::CreateStartupTasks() + 1279
40  libcontent.dylib                    0x1bb84dc6
content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&) +
822
41  libcontent.dylib                    0x1bb6c7ce
content::BrowserMain(content::MainFunctionParams const&) + 254
42  libcontent.dylib                    0x1b9bac5c
content::RunNamedProcessTypeMain(std::string const&, content::MainFunctionParams
const&, content::ContentMainDelegate*) + 268
43  libcontent.dylib                    0x1b9bc34b
content::ContentMainRunnerImpl::Run() + 779
44  libcontent.dylib                    0x1b9ba0f5
content::ContentMain(content::ContentMainParams const&) + 133
45  libchrome_main_dll.dylib            0x000adc5c ChromeMain + 92
46  Chromium                            0x0009aacb main + 43
47  Chromium                            0x0009aa95 start + 53

It looks like RenderFrameHostManager is triggering a second call. I'll take a
closer look into this when I get back to Sydney.

[Sam McNally, 2014-10-20 00:22:06]

sammc@chromium.org changed reviewers:
+ kalman@chromium.org

[Sam McNally, 2014-10-20 00:22:07]

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/serv...
File extensions/browser/service_registration_manager.cc (right):

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/serv...
extensions/browser/service_registration_manager.cc:68: extension,
render_frame_host->GetProcess()->GetID());
On 2014/10/15 20:22:49, raymes wrote:
> I don't really understand the details of this. Is there something we have to
be
> worried about here or will this always get the right context type?

I don't think this will make anything worse than it currently is.

+kalman for real answers.

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/serv...
extensions/browser/service_registration_manager.cc:71: // pass |site_url| as the
URL for permission checking. This is sufficient
On 2014/10/15 20:22:50, raymes wrote:
> What other URL would we pass? I'm not quite sure I understand what the general
> case would look like.

The actual URL loaded by the frame. This might matter for
https://developer.chrome.com/apps/manifest/externally_connectable for example.

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/serv...
extensions/browser/service_registration_manager.cc:76:
->IsAvailable(factory.first, extension, context_type, site_url)
On 2014/10/15 20:22:50, raymes wrote:
> Is it possible for the security context to of a RenderFrame to change? Could
it
> be navigated to a different URL and we decide we no longer want to grant
> permissions there? My guess is that the answer is no, but just wanted to check
> :)

As far as I can tell from the documentation the answer is no.

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/serv...
File extensions/browser/service_registration_manager.h (right):

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/serv...
extensions/browser/service_registration_manager.h:24: class ServiceFactoryBase {
On 2014/10/15 20:22:50, raymes wrote:
> Maybe add a comment explaining what this is for and why you use this pattern.

Done.

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/serv...
extensions/browser/service_registration_manager.h:82: void
AddToRenderFrame(content::RenderFrameHost* render_frame_host);
On 2014/10/15 20:22:50, raymes wrote:
> nit: AddServicesToRenderFrame might be more descriptive here.

Done.

[raymes, 2014-10-20 02:12:55]

Thanks, looks close to me.

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/exte...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/exte...
extensions/browser/extension_web_contents_observer.cc:33: if
(known_render_frame_hosts_.count(render_frame_host))
Given that it's not urgent to land this CL I think we should try to fix this bug
or at least find an owner willing to look at it before landing (otherwise other
people might start adding the same hack as this and we'll be in a worse mess!).

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/serv...
File extensions/browser/service_registration_manager.cc (right):

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/serv...
extensions/browser/service_registration_manager.cc:68: extension,
render_frame_host->GetProcess()->GetID());
Thanks. kalman@ it would be great if you could review the changes in this file,
I'm less familiar with this stuff.

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/serv...
extensions/browser/service_registration_manager.cc:71: // pass |site_url| as the
URL for permission checking. This is sufficient
Maybe we can name "site_url"->"extension_url" for now and possibly remove this
comment. Later when we revisit the general case it will be clearer that we might
need to use a different URL.

[Sam McNally, 2014-10-20 05:40:42]

Patchset #5 (id:220001) has been deleted

[Sam McNally, 2014-10-20 07:28:20]

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/serv...
File extensions/browser/service_registration_manager.cc (right):

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/serv...
extensions/browser/service_registration_manager.cc:71: // pass |site_url| as the
URL for permission checking. This is sufficient
On 2014/10/20 02:12:55, raymes wrote:
> Maybe we can name "site_url"->"extension_url" for now and possibly remove this
> comment. Later when we revisit the general case it will be clearer that we
might
> need to use a different URL.

Done.

[not at google - send to devlin, 2014-10-20 18:28:31]

(I presume you don't want me to look at anything besides that 1 line)

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/serv...
File extensions/browser/service_registration_manager.cc (right):

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/serv...
extensions/browser/service_registration_manager.cc:68: extension,
render_frame_host->GetProcess()->GetID());
On 2014/10/20 02:12:55, raymes wrote:
> Thanks. kalman@ it would be great if you could review the changes in this
file,
> I'm less familiar with this stuff.

Short answer: yes.

Long answer: it's complicated and the method has a long explanation of why:

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

[Sam McNally, 2014-10-22 02:04:29]

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/exte...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/652793002/diff/120001/extensions/browser/exte...
extensions/browser/extension_web_contents_observer.cc:33: if
(known_render_frame_hosts_.count(render_frame_host))
On 2014/10/20 02:12:55, raymes wrote:
> Given that it's not urgent to land this CL I think we should try to fix this
bug
> or at least find an owner willing to look at it before landing (otherwise
other
> people might start adding the same hack as this and we'll be in a worse
mess!).

Filed https://code.google.com/p/chromium/issues/detail?id=425397 and switched to
using RenderViewCreated for now. This should be enough for serial and we can add
support for non-main frames later.

[raymes, 2014-10-23 22:12:56]

lgtm

https://codereview.chromium.org/652793002/diff/340001/extensions/browser/mojo...
File extensions/browser/mojo/service_registration_manager.cc (right):

https://codereview.chromium.org/652793002/diff/340001/extensions/browser/mojo...
extensions/browser/mojo/service_registration_manager.cc:75: .is_available()) {
nit: can we pull this out of the if-statement to make the indentation nicer?

[Sam McNally, 2014-10-23 23:27:17]

Patchset #11 (id:360001) has been deleted

[Sam McNally, 2014-10-23 23:31:17]

Ken: can you review for OWNERS

https://codereview.chromium.org/652793002/diff/340001/extensions/browser/mojo...
File extensions/browser/mojo/service_registration_manager.cc (right):

https://codereview.chromium.org/652793002/diff/340001/extensions/browser/mojo...
extensions/browser/mojo/service_registration_manager.cc:75: .is_available()) {
On 2014/10/23 22:12:56, raymes wrote:
> nit: can we pull this out of the if-statement to make the indentation nicer?

Done.

[raymes, 2014-10-23 23:59:51]

https://codereview.chromium.org/652793002/diff/340001/extensions/browser/mojo...
File extensions/browser/mojo/service_registration_manager.h (right):

https://codereview.chromium.org/652793002/diff/340001/extensions/browser/mojo...
extensions/browser/mojo/service_registration_manager.h:37: const
base::Callback<void(mojo::InterfaceRequest<Interface>)>& factory)
I'd still like to revisit this some more (for followup CLs :). I still think it
is quite hard to understand and verbose when passing base::Callbacks to
InterfaceRequests around. I feel that it would be more self-documenting and more
flexible to have some kind of InterfaceRequestHandler<Interface> class. Let's
think some more about it as we continue working on this stuff :)

[Ken Rockot(use gerrit already), 2014-10-24 16:36:14]

lgtm

https://codereview.chromium.org/652793002/diff/380001/extensions/browser/mojo...
File extensions/browser/mojo/service_registration_manager.h (right):

https://codereview.chromium.org/652793002/diff/380001/extensions/browser/mojo...
extensions/browser/mojo/service_registration_manager.h:37: const
base::Callback<void(mojo::InterfaceRequest<Interface>)>& factory)
I agree with Raymes on the confusing nature of this type, but I'm not convinced
it can be made less confusing with any sort of type alias. Any alternative I can
think of will still be a relatively complex type expression
(FooSomething<Interface>::SomeCallbackType) and will only serve to mask the
underlying equally-complex type expression.

Fortunately I think the example of actually registering the serial service
clarifies the usage for anyone trying to understand this code, so I'm not sure
anything needs to change here.

https://codereview.chromium.org/652793002/diff/380001/extensions/browser/mojo...
extensions/browser/mojo/service_registration_manager.h:72: const std::string&
permission_name,
nit: Maybe we could have a ServiceFilter type (which for now only supports
API-permission-name-based filtering) instead of just a string? Optional, not
sure it's worth doing right now, but maybe an alternative to taking extension
ID, BrowserContext, etc, directly.

[Sam McNally, 2014-10-26 23:17:30]

sammc@chromium.org changed reviewers:
+ benwells@chromium.org

[Sam McNally, 2014-10-26 23:17:31]

+benwells for extensions/browser/DEPS.

https://codereview.chromium.org/652793002/diff/380001/extensions/browser/mojo...
File extensions/browser/mojo/service_registration_manager.h (right):

https://codereview.chromium.org/652793002/diff/380001/extensions/browser/mojo...
extensions/browser/mojo/service_registration_manager.h:72: const std::string&
permission_name,
On 2014/10/24 16:36:14, Ken Rockot wrote:
> nit: Maybe we could have a ServiceFilter type (which for now only supports
> API-permission-name-based filtering) instead of just a string? Optional, not
> sure it's worth doing right now, but maybe an alternative to taking extension
> ID, BrowserContext, etc, directly.

The TODO is about currying the Extension and/or BrowserContext into the service
factory so the service can do its own permission management.

That said, I think we will want to support more flexible coarse-grained
"permissions". Added a TODO for that too.

[benwells, 2014-10-27 02:03:17]

lgtm

[Sam McNally, 2014-10-27 02:11:03]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-10-27 02:12:19]

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

[commit-bot: I haz the power, 2014-10-27 02:54:47]

Committed patchset #13 (id:420001)

[commit-bot: I haz the power, 2014-10-27 02:55:41]

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