Begin to enable extension APIs in Extension Service Worker.

Begin to enable extension APIs in Extension Service Worker.

This CL adds "tabs" API support in extension SW and adds an end-to-end
test for it.
This CL also puts the feature behind a runtime flag which is disabled for non trunk builds by default.

Some known issues:
1) Lifetime improvement of UIThreadWorkerResponseCallbackWrapper is missing.
I am currently cleaning up on RPH shutdown, but this is not idea, we should
be cleaning up on worker thread shutdown.

2) IPC send<->receive from/to Worker Thread can be made better, see
comment above WorkerThreadDispatcher in patch set #1.

3) Currently if an extension is not "active" in a process, then API calls
from that process is not classified correctly. I'm using
"extension_service_worker" context, which allows running tabs API. In
theory since a worker cannot be spawned from outside of the extension
process (TODO: Verify this), we can <fingers_crossed>safely</fingers_crossed>
classify this as a blessed context.

BUG=602442
Test=Register an extension with "tabs" permission, register a service
worker from the extension and call chrome.tabs.create() from SW code!
Expect it to work

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

[lazyboy, 2016-04-12 03:53:21]

lazyboy@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[lazyboy, 2016-04-12 03:53:22]

Here's the CL, let me know how you'd like to proceed, e.g. meet up in VC to
discuss, start a brief doc about the approach noting the caveats, etc.
Thanks.

[Devlin, 2016-04-13 19:46:32]

first round.  I'm also a little concerned with some of the threading in
dispatcher - it should survive for the duration of the render thread, but
shutdown might be a little tricky.

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
File extensions/browser/extension_function.cc (right):

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
extensions/browser/extension_function.cc:444: if (!render_frame_host)
Might be worth a comment when this can happen.

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
File extensions/browser/extension_function_dispatcher.cc (right):

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
extensions/browser/extension_function_dispatcher.cc:256:
UIThreadWorkerResponseCallbackWrapperMap& map =
This could potentially remove multiple CallbackWrappers, right?  If that's the
case, don't we need to remove ourselves as an observer in the dtor as well, if
we're still observing?

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
extensions/browser/extension_function_dispatcher.cc:267: // Note: we might be
deleted here!
Technically, this object might even be deleted *before* this point, anywhere in
the for-loop.  For that reason, I think I'd prefer to make a
RemoveWorkerCallbacksForProcess() function on the function dispatcher.

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
extensions/browser/extension_function_dispatcher.cc:412: // Extension API from
Service Worker.
Add an analogous comment in the other if clause on line 397.

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
extensions/browser/extension_function_dispatcher.cc:413:
DCHECK(params.embedded_worker_id >= 0);
DCHECK_GE?

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
extensions/browser/extension_function_dispatcher.cc:414: const int& id =
params.embedded_worker_id;
no need to make this a const & - it's no cheaper than copying the int.

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
extensions/browser/extension_function_dispatcher.cc:429:
DispatchWithCallbackInternal(
We can lump these (this and line 408) together if we just have a callback object
that we set in the two different if clauses.

I suspect we can also simplify some of the other duplicate logic here, but it's
a little messy to do so... I'll think on it a bit.

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
File extensions/browser/extension_function_dispatcher.h (right):

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
extensions/browser/extension_function_dispatcher.h:174: typedef std::pair<int,
int> UIThreadWorkerResponseCallbackWrapperMapKey;
usings in new code

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
extensions/browser/extension_function_dispatcher.h:174: typedef std::pair<int,
int> UIThreadWorkerResponseCallbackWrapperMapKey;
comment indicating what the two ints that make up the key are.

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
File extensions/browser/extension_service_worker_message_filter.cc (right):

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
extensions/browser/extension_service_worker_message_filter.cc:44:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
DCHECK_CURRENTLY_ON?

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
File extensions/browser/extension_service_worker_message_filter.h (right):

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
extensions/browser/extension_service_worker_message_filter.h:12: class Profile;
mentioning Profile is a layering violation, since //extensions isn't supposed to
depend on chrome.  Luckily, it doesn't look like it's needed.

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
extensions/browser/extension_service_worker_message_filter.h:28:
content::BrowserContext* profile);
nit s/profile/context

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
extensions/browser/extension_service_worker_message_filter.h:43:
scoped_ptr<ExtensionFunctionDispatcher,
std::unique_ptr in new code.

https://codereview.chromium.org/1880933002/diff/1/extensions/common/extension...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/1880933002/diff/1/extensions/common/extension...
extensions/common/extension_messages.h:111: // id.
Otherwise, this is...?

https://codereview.chromium.org/1880933002/diff/1/extensions/common/extension...
extensions/common/extension_messages.h:115: // worker id.
ditto

https://codereview.chromium.org/1880933002/diff/1/extensions/common/features/...
File extensions/common/features/simple_feature.cc (right):

https://codereview.chromium.org/1880933002/diff/1/extensions/common/features/...
extensions/common/features/simple_feature.cc:266:
contexts["extension_service_worker"] = Feature::SERVICE_WORKER_CONTEXT;
We should have this behind a flag (or just an internal bool we can flip for
testing/experimentation) until we get some of the TODOs ironed out.

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/dispatc...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/dispatc...
extensions/renderer/dispatcher.cc:448: // TODO(lazyboy):
?

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/dispatc...
extensions/renderer/dispatcher.cc:453: scoped_ptr<ModuleSystem> module_system(
unique_ptr

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/dispatc...
extensions/renderer/dispatcher.cc:467: if (context->extension()) {
When could this be false?  We don't initialize any bindings for non-extension sw
contexts.

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/dispatc...
extensions/renderer/dispatcher.cc:1363: void
Dispatcher::UpdateBindingsForContext(ScriptContext* context) {
Might be worth commenting that this can run on multiple threads.  I think most
stuff we do here is safe, but it's still a little dodgy.

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/request...
File extensions/renderer/request_sender.cc (left):

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/request...
extensions/renderer/request_sender.cc:83: content::RenderFrame* render_frame =
context->GetRenderFrame();
I think this early-out was important.  Not least because of things like the
InsertRequest() call on 89, which would result in an orphaned request if we
never actually sent it.

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/request...
File extensions/renderer/request_sender.h (right):

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/request...
extensions/renderer/request_sender.h:15: struct ExtensionHostMsg_Request_Params;
put top-level namespace forward declarations in one place (i.e., move this or
PendingRequest)

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/resourc...
File extensions/renderer/resources/runtime_custom_bindings.js (right):

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/resourc...
extensions/renderer/resources/runtime_custom_bindings.js:14: var WINDOW = {};
Does anything else need to be done here?

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/script_...
File extensions/renderer/script_context.cc (right):

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/script_...
extensions/renderer/script_context.cc:259: GURL url = url_;
ternary?

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/service...
File extensions/renderer/service_worker_data.h (right):

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/service...
extensions/renderer/service_worker_data.h:18: // TODO(lazyboy): Move
WorkerScriptContextSet functionality to this class.
WorkerScriptContextSet?

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/service...
extensions/renderer/service_worker_data.h:22: ~ServiceWorkerData();
\n

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/service...
extensions/renderer/service_worker_data.h:30: scoped_ptr<V8SchemaRegistry>
v8_schema_registry_;
unique_ptr

[lazyboy, 2016-04-14 02:07:54]

round 1 comments addressed in patch set #2

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
File extensions/browser/extension_function.cc (right):

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
extensions/browser/extension_function.cc:444: if (!render_frame_host)
On 2016/04/13 19:46:30, Devlin wrote:
> Might be worth a comment when this can happen.

Done.

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
File extensions/browser/extension_function_dispatcher.cc (right):

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
extensions/browser/extension_function_dispatcher.cc:256:
UIThreadWorkerResponseCallbackWrapperMap& map =
On 2016/04/13 19:46:30, Devlin wrote:
> This could potentially remove multiple CallbackWrappers, right?  If that's the
> case, don't we need to remove ourselves as an observer in the dtor as well, if
> we're still observing?

Ah, RenderProcessHost would still keep a reference to this.
Fixed.

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
extensions/browser/extension_function_dispatcher.cc:267: // Note: we might be
deleted here!
On 2016/04/13 19:46:30, Devlin wrote:
> Technically, this object might even be deleted *before* this point, anywhere
in
> the for-loop.  For that reason, I think I'd prefer to make a
> RemoveWorkerCallbacksForProcess() function on the function dispatcher.

Done.

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
extensions/browser/extension_function_dispatcher.cc:412: // Extension API from
Service Worker.
On 2016/04/13 19:46:31, Devlin wrote:
> Add an analogous comment in the other if clause on line 397.

Done.

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
extensions/browser/extension_function_dispatcher.cc:413:
DCHECK(params.embedded_worker_id >= 0);
On 2016/04/13 19:46:31, Devlin wrote:
> DCHECK_GE?

Done.

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
extensions/browser/extension_function_dispatcher.cc:414: const int& id =
params.embedded_worker_id;
On 2016/04/13 19:46:31, Devlin wrote:
> no need to make this a const & - it's no cheaper than copying the int.

Done.

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
extensions/browser/extension_function_dispatcher.cc:429:
DispatchWithCallbackInternal(
On 2016/04/13 19:46:31, Devlin wrote:
> We can lump these (this and line 408) together if we just have a callback
object
> that we set in the two different if clauses.
I had that in the beginning, but later the code diverged to two unrelated
callback classes instead of one:
UIThreadResponseCallbackWrapper, UIThreadWorkerResponseCallbackWrapper.
> 
> I suspect we can also simplify some of the other duplicate logic here, but
it's
> a little messy to do so... I'll think on it a bit.

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
File extensions/browser/extension_function_dispatcher.h (right):

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
extensions/browser/extension_function_dispatcher.h:174: typedef std::pair<int,
int> UIThreadWorkerResponseCallbackWrapperMapKey;
On 2016/04/13 19:46:31, Devlin wrote:
> comment indicating what the two ints that make up the key are.

Done.

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
extensions/browser/extension_function_dispatcher.h:174: typedef std::pair<int,
int> UIThreadWorkerResponseCallbackWrapperMapKey;
On 2016/04/13 19:46:31, Devlin wrote:
> usings in new code

Done.

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
File extensions/browser/extension_service_worker_message_filter.cc (right):

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
extensions/browser/extension_service_worker_message_filter.cc:44:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
On 2016/04/13 19:46:31, Devlin wrote:
> DCHECK_CURRENTLY_ON?

Done.

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
File extensions/browser/extension_service_worker_message_filter.h (right):

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
extensions/browser/extension_service_worker_message_filter.h:12: class Profile;
On 2016/04/13 19:46:31, Devlin wrote:
> mentioning Profile is a layering violation, since //extensions isn't supposed
to
> depend on chrome.  Luckily, it doesn't look like it's needed.

Right, this somehow creeped in, removed.

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
extensions/browser/extension_service_worker_message_filter.h:28:
content::BrowserContext* profile);
On 2016/04/13 19:46:31, Devlin wrote:
> nit s/profile/context

Done.

https://codereview.chromium.org/1880933002/diff/1/extensions/browser/extensio...
extensions/browser/extension_service_worker_message_filter.h:43:
scoped_ptr<ExtensionFunctionDispatcher,
On 2016/04/13 19:46:31, Devlin wrote:
> std::unique_ptr in new code.

Done.

https://codereview.chromium.org/1880933002/diff/1/extensions/common/extension...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/1880933002/diff/1/extensions/common/extension...
extensions/common/extension_messages.h:111: // id.
On 2016/04/13 19:46:31, Devlin wrote:
> Otherwise, this is...?

Done.

https://codereview.chromium.org/1880933002/diff/1/extensions/common/extension...
extensions/common/extension_messages.h:115: // worker id.
On 2016/04/13 19:46:31, Devlin wrote:
> ditto

Done.

https://codereview.chromium.org/1880933002/diff/1/extensions/common/features/...
File extensions/common/features/simple_feature.cc (right):

https://codereview.chromium.org/1880933002/diff/1/extensions/common/features/...
extensions/common/features/simple_feature.cc:266:
contexts["extension_service_worker"] = Feature::SERVICE_WORKER_CONTEXT;
On 2016/04/13 19:46:31, Devlin wrote:
> We should have this behind a flag (or just an internal bool we can flip for
> testing/experimentation) until we get some of the TODOs ironed out.

I've added a bool in feature_util, used that to guard some key code: the message
filter and the ServiceWorkerData and worker request sender code. However this
very line is not guarded as I need to guard _api_features.json too.

Since we need to turn this on in tests, and we need to sync the bool in
renderer/ process, this also involves IPC :( Let me know if you have a better
suggestion.

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/dispatc...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/dispatc...
extensions/renderer/dispatcher.cc:448: // TODO(lazyboy):
On 2016/04/13 19:46:32, Devlin wrote:
> ?

Removed.

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/dispatc...
extensions/renderer/dispatcher.cc:453: scoped_ptr<ModuleSystem> module_system(
On 2016/04/13 19:46:31, Devlin wrote:
> unique_ptr

Done.

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/dispatc...
extensions/renderer/dispatcher.cc:467: if (context->extension()) {
On 2016/04/13 19:46:32, Devlin wrote:
> When could this be false?  We don't initialize any bindings for non-extension
sw
> contexts.

Copy-paste issue, removed the check.

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/dispatc...
extensions/renderer/dispatcher.cc:1363: void
Dispatcher::UpdateBindingsForContext(ScriptContext* context) {
On 2016/04/13 19:46:32, Devlin wrote:
> Might be worth commenting that this can run on multiple threads.  I think most
> stuff we do here is safe, but it's still a little dodgy.

Done.

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/request...
File extensions/renderer/request_sender.cc (left):

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/request...
extensions/renderer/request_sender.cc:83: content::RenderFrame* render_frame =
context->GetRenderFrame();
On 2016/04/13 19:46:32, Devlin wrote:
> I think this early-out was important.  Not least because of things like the
> InsertRequest() call on 89, which would result in an orphaned request if we
> never actually sent it.

Done.

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/request...
File extensions/renderer/request_sender.h (right):

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/request...
extensions/renderer/request_sender.h:15: struct ExtensionHostMsg_Request_Params;
On 2016/04/13 19:46:32, Devlin wrote:
> put top-level namespace forward declarations in one place (i.e., move this or
> PendingRequest)

Done.

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/resourc...
File extensions/renderer/resources/runtime_custom_bindings.js (right):

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/resourc...
extensions/renderer/resources/runtime_custom_bindings.js:14: var WINDOW = {};
On 2016/04/13 19:46:32, Devlin wrote:
> Does anything else need to be done here?

At this point there isn't much we can do, WINDOW is used for backgroundPage
stuff below, which isn't available in SW context. I've added a TODO.

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/script_...
File extensions/renderer/script_context.cc (right):

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/script_...
extensions/renderer/script_context.cc:259: GURL url = url_;
On 2016/04/13 19:46:32, Devlin wrote:
> ternary?

Done.

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/service...
File extensions/renderer/service_worker_data.h (right):

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/service...
extensions/renderer/service_worker_data.h:18: // TODO(lazyboy): Move
WorkerScriptContextSet functionality to this class.
On 2016/04/13 19:46:32, Devlin wrote:
> WorkerScriptContextSet?

WorkerScriptContextSet manages SW ScriptContexts in worker thread in a TLS
variable.
ServiceWorkerData is also stored in worker TLS, so this class can also store
those ScriptContexts.
I've clarified the comment a bit.

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/service...
extensions/renderer/service_worker_data.h:22: ~ServiceWorkerData();
On 2016/04/13 19:46:32, Devlin wrote:
> \n

Done.

https://codereview.chromium.org/1880933002/diff/1/extensions/renderer/service...
extensions/renderer/service_worker_data.h:30: scoped_ptr<V8SchemaRegistry>
v8_schema_registry_;
On 2016/04/13 19:46:32, Devlin wrote:
> unique_ptr

Done.

[Devlin, 2016-04-14 22:34:52]

mostly nits

https://codereview.chromium.org/1880933002/diff/1/extensions/common/features/...
File extensions/common/features/simple_feature.cc (right):

https://codereview.chromium.org/1880933002/diff/1/extensions/common/features/...
extensions/common/features/simple_feature.cc:266:
contexts["extension_service_worker"] = Feature::SERVICE_WORKER_CONTEXT;
On 2016/04/14 02:07:53, lazyboy wrote:
> On 2016/04/13 19:46:31, Devlin wrote:
> > We should have this behind a flag (or just an internal bool we can flip for
> > testing/experimentation) until we get some of the TODOs ironed out.
> 
> I've added a bool in feature_util, used that to guard some key code: the
message
> filter and the ServiceWorkerData and worker request sender code. However this
> very line is not guarded as I need to guard _api_features.json too.
> 
> Since we need to turn this on in tests, and we need to sync the bool in
> renderer/ process, this also involves IPC :( Let me know if you have a better
> suggestion.

See suggestion in ipc messages.

https://codereview.chromium.org/1880933002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/1880933002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/service_worker_apitest.cc:638: web_contents,
"window.runServiceWorker()", &result));
Shouldn't we check |result|?

https://codereview.chromium.org/1880933002/diff/20001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/service_worker/tabs_create/sw.js
(right):

https://codereview.chromium.org/1880933002/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/service_worker/tabs_create/sw.js:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
2016

https://codereview.chromium.org/1880933002/diff/20001/extensions/browser/exte...
File extensions/browser/extension_function_dispatcher.cc (right):

https://codereview.chromium.org/1880933002/diff/20001/extensions/browser/exte...
extensions/browser/extension_function_dispatcher.cc:261: if (render_process_id
!= render_process_id_)
When could this happen?  We only add ourselves as an observer to RPH with that
id.

https://codereview.chromium.org/1880933002/diff/20001/extensions/common/exten...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/1880933002/diff/20001/extensions/common/exten...
extensions/common/extension_messages.h:586: bool /* enabled */)
Maybe we should just enable this on trunk.  For one thing, then we only need the
SetChannel IPC above.  For another, it means that we get trybot coverage for all
of this - which is good.

https://codereview.chromium.org/1880933002/diff/20001/extensions/renderer/req...
File extensions/renderer/request_sender.cc (right):

https://codereview.chromium.org/1880933002/diff/20001/extensions/renderer/req...
extensions/renderer/request_sender.cc:91: // RenderFrame cannot be found
PendingRequest below.
this comment needs to be reworded

https://codereview.chromium.org/1880933002/diff/20001/extensions/renderer/req...
File extensions/renderer/request_sender.h (right):

https://codereview.chromium.org/1880933002/diff/20001/extensions/renderer/req...
extensions/renderer/request_sender.h:86: virtual void
SendRequest(content::RenderFrame* render_frame,
Function comments

https://codereview.chromium.org/1880933002/diff/20001/extensions/renderer/ser...
File extensions/renderer/service_worker_request_sender.cc (right):

https://codereview.chromium.org/1880933002/diff/20001/extensions/renderer/ser...
extensions/renderer/service_worker_request_sender.cc:28: DCHECK(worker_thread_id
> 0);
DCHECK_GT

[lazyboy, 2016-04-15 00:57:47]

https://codereview.chromium.org/1880933002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/1880933002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/service_worker_apitest.cc:638: web_contents,
"window.runServiceWorker()", &result));
On 2016/04/14 22:34:52, Devlin wrote:
> Shouldn't we check |result|?

Wow, this got deleted somehow.
Fixed.

https://codereview.chromium.org/1880933002/diff/20001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/service_worker/tabs_create/sw.js
(right):

https://codereview.chromium.org/1880933002/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/service_worker/tabs_create/sw.js:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
On 2016/04/14 22:34:52, Devlin wrote:
> 2016

Done.

https://codereview.chromium.org/1880933002/diff/20001/extensions/browser/exte...
File extensions/browser/extension_function_dispatcher.cc (right):

https://codereview.chromium.org/1880933002/diff/20001/extensions/browser/exte...
extensions/browser/extension_function_dispatcher.cc:261: if (render_process_id
!= render_process_id_)
On 2016/04/14 22:34:52, Devlin wrote:
> When could this happen?  We only add ourselves as an observer to RPH with that
> id.

Right, removed.

https://codereview.chromium.org/1880933002/diff/20001/extensions/common/exten...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/1880933002/diff/20001/extensions/common/exten...
extensions/common/extension_messages.h:586: bool /* enabled */)
On 2016/04/14 22:34:52, Devlin wrote:
> Maybe we should just enable this on trunk.  For one thing, then we only need
the
> SetChannel IPC above.  For another, it means that we get trybot coverage for
all
> of this - which is good.

OK, moved the feature thing to ExtensionsClient.

https://codereview.chromium.org/1880933002/diff/20001/extensions/renderer/req...
File extensions/renderer/request_sender.cc (right):

https://codereview.chromium.org/1880933002/diff/20001/extensions/renderer/req...
extensions/renderer/request_sender.cc:91: // RenderFrame cannot be found
PendingRequest below.
On 2016/04/14 22:34:52, Devlin wrote:
> this comment needs to be reworded

Done.

https://codereview.chromium.org/1880933002/diff/20001/extensions/renderer/req...
File extensions/renderer/request_sender.h (right):

https://codereview.chromium.org/1880933002/diff/20001/extensions/renderer/req...
extensions/renderer/request_sender.h:86: virtual void
SendRequest(content::RenderFrame* render_frame,
On 2016/04/14 22:34:52, Devlin wrote:
> Function comments

Done.

https://codereview.chromium.org/1880933002/diff/20001/extensions/renderer/ser...
File extensions/renderer/service_worker_request_sender.cc (right):

https://codereview.chromium.org/1880933002/diff/20001/extensions/renderer/ser...
extensions/renderer/service_worker_request_sender.cc:28: DCHECK(worker_thread_id
> 0);
On 2016/04/14 22:34:52, Devlin wrote:
> DCHECK_GT

Done.

[Devlin, 2016-04-15 21:06:19]

lgtm (though you'll need some blink folks)

https://codereview.chromium.org/1880933002/diff/40001/extensions/browser/exte...
File extensions/browser/extension_function_dispatcher.cc (right):

https://codereview.chromium.org/1880933002/diff/40001/extensions/browser/exte...
extensions/browser/extension_function_dispatcher.cc:236:
rph->RemoveObserver(this);
Hmm... is this right?  If this is the object that first received the
RenderProcessExited (or other) notification, it's already removed itself as an
observer, but, I think, RPH::FromID will still return the RPH (since it's
kinda-sorta still valid).  So we'll try to remove a non-existent observer.

Since CleanUp() results in the synchronous deletion of this object, can we just
move the RemoveObserver call to be solely in the dtor?  And, in that case, maybe
just migrate to a ScopedObserver?

https://codereview.chromium.org/1880933002/diff/40001/extensions/browser/exte...
File extensions/browser/extension_service_worker_message_filter.h (right):

https://codereview.chromium.org/1880933002/diff/40001/extensions/browser/exte...
extensions/browser/extension_service_worker_message_filter.h:28: //
content::BrowserMessageFilter methods:
nit: prefer just
// content::BrowserMessageFilter:

[lazyboy, 2016-04-18 18:42:15]

https://codereview.chromium.org/1880933002/diff/40001/extensions/browser/exte...
File extensions/browser/extension_function_dispatcher.cc (right):

https://codereview.chromium.org/1880933002/diff/40001/extensions/browser/exte...
extensions/browser/extension_function_dispatcher.cc:236:
rph->RemoveObserver(this);
On 2016/04/15 21:06:19, Devlin wrote:
> Hmm... is this right?  If this is the object that first received the
> RenderProcessExited (or other) notification, it's already removed itself as an
> observer, but, I think, RPH::FromID will still return the RPH (since it's
> kinda-sorta still valid).  So we'll try to remove a non-existent observer.
> 
> Since CleanUp() results in the synchronous deletion of this object, can we
just
> move the RemoveObserver call to be solely in the dtor?  And, in that case,
maybe
> just migrate to a ScopedObserver?

Done.

https://codereview.chromium.org/1880933002/diff/40001/extensions/browser/exte...
File extensions/browser/extension_service_worker_message_filter.h (right):

https://codereview.chromium.org/1880933002/diff/40001/extensions/browser/exte...
extensions/browser/extension_service_worker_message_filter.h:28: //
content::BrowserMessageFilter methods:
On 2016/04/15 21:06:19, Devlin wrote:
> nit: prefer just
> // content::BrowserMessageFilter:

Done.

https://codereview.chromium.org/1880933002/diff/60001/extensions/renderer/obj...
File extensions/renderer/object_backed_native_handler.cc (right):

https://codereview.chromium.org/1880933002/diff/60001/extensions/renderer/obj...
extensions/renderer/object_backed_native_handler.cc:171: : nullptr;
FYI, I had to add this b/c accessing ScriptContextSet wouldn't work on worker
threads.

[lazyboy, 2016-04-19 02:20:22]

lazyboy@chromium.org changed reviewers:
+ falken@chromium.org, yhirano@chromium.org

[lazyboy, 2016-04-19 02:20:23]

+falken for content/renderer/service_worker/*

+yhirano for Source/bindings/
bindings/core/v8/WorkerOrWorkletScriptController.h
bindings/core/v8/WorkerOrWorkletScriptController.cpp

[haraken, 2016-04-19 04:24:56]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-04-19 04:24:58]

bindings/ LGTM with a question.

https://codereview.chromium.org/1880933002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp
(right):

https://codereview.chromium.org/1880933002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp:120:
registeredExtensions().append(extension);

Can we add an assert to check that this is a ScriptController for ServiceWorker?
We don't want to allow any extension for normal workers.

https://codereview.chromium.org/1880933002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp:177:
extensionNames.append(extension->name());

Just to confirm: You're unconditionally enabling all extensions here -- this is
intentional, right?

c.f., The main world enables only extensions that meet
associatedFrame->loader().client()->allowScriptExtension().

https://codereview.chromium.org/1880933002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp:215:
V8Extensions& WorkerOrWorkletScriptController::registeredExtensions()

Move this method to just above registerExtensionIfNeeded.

[lazyboy, 2016-04-20 02:26:54]

https://codereview.chromium.org/1880933002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp
(right):

https://codereview.chromium.org/1880933002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp:120:
registeredExtensions().append(extension);
On 2016/04/19 04:24:57, haraken wrote:
> 
> Can we add an assert to check that this is a ScriptController for
ServiceWorker?
> We don't want to allow any extension for normal workers.
> 

How do I do that? Note that this is a static method.

https://codereview.chromium.org/1880933002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp:177:
extensionNames.append(extension->name());
On 2016/04/19 04:24:57, haraken wrote:
> 
> Just to confirm: You're unconditionally enabling all extensions here -- this
is
> intentional, right?
> 
> c.f., The main world enables only extensions that meet
> associatedFrame->loader().client()->allowScriptExtension().

Yes, I had to do this unconditionally. (I also got fooled by it at first
thinking allowScriptExtension() always returns true, except EmptyClients, which
I mistakenly thought were only for tests).

I don't however know what the implication of unconditionally turning this on is.
One way could be to try to ask the embedder the same way as FrameLoaderClient
does.

Also, should we at guard setting |extensionNames| with if
(m_globalScope->isServiceWorkerGlobalScope()) ?

WDYT?

https://codereview.chromium.org/1880933002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp:215:
V8Extensions& WorkerOrWorkletScriptController::registeredExtensions()
On 2016/04/19 04:24:57, haraken wrote:
> 
> Move this method to just above registerExtensionIfNeeded.

Done.

[falken, 2016-04-20 02:31:46]

https://codereview.chromium.org/1880933002/diff/60001/chrome/common/extension...
File chrome/common/extensions/chrome_extensions_client.cc (right):

https://codereview.chromium.org/1880933002/diff/60001/chrome/common/extension...
chrome/common/extensions/chrome_extensions_client.cc:383: return
GetCurrentChannel() <= version_info::Channel::UNKNOWN;
Should this just be == UNKNOWN?

https://codereview.chromium.org/1880933002/diff/60001/content/renderer/servic...
File content/renderer/service_worker/embedded_worker_dispatcher.cc (right):

https://codereview.chromium.org/1880933002/diff/60001/content/renderer/servic...
content/renderer/service_worker/embedded_worker_dispatcher.cc:83:
GetContentClient()->renderer()->InstallV8ExtensionForServiceWorkers();
I don't understand V8 Extensions, but it looks like it's just some JS that gets
injected. In the case of extensions it's the code in
extensions/renderer/safe_builtins.cc.

Do you know how heavy this InstallV8ExtensionForServiceWorkers() function will
be? Does it early return if this service worker is not an extensions service
worker?

[haraken, 2016-04-20 04:24:56]

https://codereview.chromium.org/1880933002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp
(right):

https://codereview.chromium.org/1880933002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp:177:
extensionNames.append(extension->name());
On 2016/04/20 02:26:54, lazyboy wrote:
> On 2016/04/19 04:24:57, haraken wrote:
> > 
> > Just to confirm: You're unconditionally enabling all extensions here -- this
> is
> > intentional, right?
> > 
> > c.f., The main world enables only extensions that meet
> > associatedFrame->loader().client()->allowScriptExtension().
> 
> Yes, I had to do this unconditionally. (I also got fooled by it at first
> thinking allowScriptExtension() always returns true, except EmptyClients,
which
> I mistakenly thought were only for tests).
> 
> I don't however know what the implication of unconditionally turning this on
is.
> One way could be to try to ask the embedder the same way as FrameLoaderClient
> does.
> 
> Also, should we at guard setting |extensionNames| with if
> (m_globalScope->isServiceWorkerGlobalScope()) ?
> 
> WDYT?

Yes, we *must* do this. Otherwise, the extensions will be enable on normal
workers as well.

BTW, is the set of extensions installed on ScriptController different from the
set of extensions installed on WorkerOrWorkletScriptController? In other words,
do you have different sets of extensions for the main page and the service
worker?

I think the answer should be no. If so, we should remove
WorkerOrWorkletScriptController::extensions and instead just use
ScriptController::extensions.

[lazyboy, 2016-04-21 05:52:15]

Sorry for the delay, I was trying to get rid of
WorkerOrWorkletScriptController::registeredExtension() in favor of
ScriptController::registeredExtension(), but didn't succeed.
See patch set #6

https://codereview.chromium.org/1880933002/diff/60001/chrome/common/extension...
File chrome/common/extensions/chrome_extensions_client.cc (right):

https://codereview.chromium.org/1880933002/diff/60001/chrome/common/extension...
chrome/common/extensions/chrome_extensions_client.cc:383: return
GetCurrentChannel() <= version_info::Channel::UNKNOWN;
On 2016/04/20 02:31:45, falken wrote:
> Should this just be == UNKNOWN?

Done.

https://codereview.chromium.org/1880933002/diff/60001/content/renderer/servic...
File content/renderer/service_worker/embedded_worker_dispatcher.cc (right):

https://codereview.chromium.org/1880933002/diff/60001/content/renderer/servic...
content/renderer/service_worker/embedded_worker_dispatcher.cc:83:
GetContentClient()->renderer()->InstallV8ExtensionForServiceWorkers();
On 2016/04/20 02:31:46, falken wrote:
> I don't understand V8 Extensions, but it looks like it's just some JS that
gets
> injected. In the case of extensions it's the code in
> extensions/renderer/safe_builtins.cc.
> 
> Do you know how heavy this InstallV8ExtensionForServiceWorkers() function will
> be? Does it early return if this service worker is not an extensions service
> worker?

I've added params.script_url to the function to bail out if the script url is
not an extension URL.

https://codereview.chromium.org/1880933002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp
(right):

https://codereview.chromium.org/1880933002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp:177:
extensionNames.append(extension->name());
On 2016/04/20 04:24:56, haraken wrote:
> On 2016/04/20 02:26:54, lazyboy wrote:
> > On 2016/04/19 04:24:57, haraken wrote:
> > > 
> > > Just to confirm: You're unconditionally enabling all extensions here --
this
> > is
> > > intentional, right?
> > > 
> > > c.f., The main world enables only extensions that meet
> > > associatedFrame->loader().client()->allowScriptExtension().
> > 
> > Yes, I had to do this unconditionally. (I also got fooled by it at first
> > thinking allowScriptExtension() always returns true, except EmptyClients,
> which
> > I mistakenly thought were only for tests).
> > 
> > I don't however know what the implication of unconditionally turning this on
> is.
> > One way could be to try to ask the embedder the same way as
FrameLoaderClient
> > does.
> > 
> > Also, should we at guard setting |extensionNames| with if
> > (m_globalScope->isServiceWorkerGlobalScope()) ?
> > 
> > WDYT?
> 
> Yes, we *must* do this. Otherwise, the extensions will be enable on normal
> workers as well.
I have guarded the extension installation by checking whether this has a 
ServiceWorker GlobalScope.

> 
> BTW, is the set of extensions installed on ScriptController different from the
> set of extensions installed on WorkerOrWorkletScriptController? In other
words,
> do you have different sets of extensions for the main page and the service
> worker?
> 
> I think the answer should be no. If so, we should remove
> WorkerOrWorkletScriptController::extensions and instead just use
> ScriptController::extensions.


When I run and print ScriptController::registeredExtensions() for debugging, I
get the following 3, where I would have expected only the SafeBuiltins one:
extensions::SafeBuiltins
v8/External
v8/LoadTimes

Since WorkerOrWorkerScriptController::registeredExtensions() is only
accessed/read in worker thread, isn't using
ScriptController::registeredExtensions() non-thread safe as someone can add more
stuff to it, possibly from main renderer?

If I use ScriptController::registeredExtensions() here, extension service
workers work, but regular web service worker renderer crashes. I haven't figured
out the exact details here.

Also, based on falken@'s comment, I've restricted populating
WorkerOrWorkletScriptController::registeredExtensions() to only for chrome
extension service workers, not for web service workers.

[haraken, 2016-04-21 07:50:44]

> When I run and print ScriptController::registeredExtensions() for debugging, I
> get the following 3, where I would have expected only the SafeBuiltins one:
> extensions::SafeBuiltins
> v8/External
> v8/LoadTimes
> 

I don't know the last two either, but it sounds a bit strange that you're going
to enable different set of extensions for service workers and the main thread.

> Since WorkerOrWorkerScriptController::registeredExtensions() is only
> accessed/read in worker thread, isn't using
> ScriptController::registeredExtensions() non-thread safe as someone can add
more
> stuff to it, possibly from main renderer?

We can add a lock. But in the first place, as far as I understand, extensions
can be added to the list only before Blink creates the first window, right?
Otherwise, the extensions won't be enabled on the window.

> 
> If I use ScriptController::registeredExtensions() here, extension service
> workers work, but regular web service worker renderer crashes. I haven't
figured
> out the exact details here.

Hmm. I'm sorry I'm blocking you but I'd like to clarify:

- Whether we should use the same set of extensions for service workers and the
main thread.
- If the answer is yes, I think we should unconditionally use
ScriptController::registeredExtensions().
- If the answer is no, I'm curious why we need different sets of extensions.

https://codereview.chromium.org/1880933002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp
(right):

https://codereview.chromium.org/1880933002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp:190:
if (m_globalScope->isServiceWorkerGlobalScope()) {

Slightly better:

  CHECK(m_globalScope->isServiceWorkerGlobalScope() ||
registeredExtensions().size() == 0);

[lazyboy, 2016-04-22 00:05:14]

Description was changed from

==========
Begin to enable extension APIs in Extension Service Worker.

This CL adds "tabs" API support in extension SW and adds an end-to-end
test for it.

Some known issues:
1) Lifetime improvement of UIThreadWorkerResponseCallbackWrapper is missing.
I am currently cleaning up on RPH shutdown, but this is not idea, we should
be cleaning up on worker thread shutdown.

2) IPC send<->receive from/to Worker Thread can be made better, see
comment above WorkerThreadDispatcher in patch set #1.

3) Currently if an extension is not "active" in a process, then API calls
from that process is not classified correctly. I'm using
"extension_service_worker" context, which allows running tabs API. In
theory since a worker cannot be spawned from outside of the extension
process (TODO: Verify this), we can <fingers_crossed>safely</fingers_crossed>
classify this as a blessed context.

BUG=602442
Test=Register an extension with "tabs" permission, register a service
worker from the extension and call chrome.tabs.create() from SW code!
Expect it to work
==========

to

==========
Begin to enable extension APIs in Extension Service Worker.

This CL adds "tabs" API support in extension SW and adds an end-to-end
test for it.
This CL also puts the feature behind a runtime flag which is disabled for non
trunk builds by default.

Some known issues:
1) Lifetime improvement of UIThreadWorkerResponseCallbackWrapper is missing.
I am currently cleaning up on RPH shutdown, but this is not idea, we should
be cleaning up on worker thread shutdown.

2) IPC send<->receive from/to Worker Thread can be made better, see
comment above WorkerThreadDispatcher in patch set #1.

3) Currently if an extension is not "active" in a process, then API calls
from that process is not classified correctly. I'm using
"extension_service_worker" context, which allows running tabs API. In
theory since a worker cannot be spawned from outside of the extension
process (TODO: Verify this), we can <fingers_crossed>safely</fingers_crossed>
classify this as a blessed context.

BUG=602442
Test=Register an extension with "tabs" permission, register a service
worker from the extension and call chrome.tabs.create() from SW code!
Expect it to work
==========

[lazyboy, 2016-04-22 00:17:39]

On 2016/04/21 07:50:44, haraken wrote:
> > When I run and print ScriptController::registeredExtensions() for debugging,
I
> > get the following 3, where I would have expected only the SafeBuiltins one:
> > extensions::SafeBuiltins
> > v8/External
> > v8/LoadTimes
> > 
> 
> I don't know the last two either, but it sounds a bit strange that you're
going
> to enable different set of extensions for service workers and the main thread.

I found the comprehensive list for these V8 extensions. Actually there can be
more:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ch...
> 
> > Since WorkerOrWorkerScriptController::registeredExtensions() is only
> > accessed/read in worker thread, isn't using
> > ScriptController::registeredExtensions() non-thread safe as someone can add
> more
> > stuff to it, possibly from main renderer?
> 
> We can add a lock. But in the first place, as far as I understand, extensions
> can be added to the list only before Blink creates the first window, right?
> Otherwise, the extensions won't be enabled on the window.

Yes, I think that's correct. Thread issues shouldn't happen.
Likely I was hitting some other crash? I can't repro the crash anymore today.
Grrr.
> 
> > 
> > If I use ScriptController::registeredExtensions() here, extension service
> > workers work, but regular web service worker renderer crashes. I haven't
> figured
> > out the exact details here.
> 
> Hmm. I'm sorry I'm blocking you but I'd like to clarify:
> 
> - Whether we should use the same set of extensions for service workers and the
> main thread.
> - If the answer is yes, I think we should unconditionally use
> ScriptController::registeredExtensions().
> - If the answer is no, I'm curious why we need different sets of extensions.

Using the same set of extensions for service workers shouldn't hurt, though we
only care about SafeBuiltins for extensions/ code.

I've moved to using ScriptController's set of extensions unconditionally.

I don't think SafeBuiltins would be much costly, but I don't know about cost of
registering other extensions in service worker context.

It would be better if we can ask embedder (content/renderer)
whether to allow these extension scripts or not, similar to FrameLoaderClient's
allowScriptExtension()
method.
If you agree, can you point me how to reach out to content/renderer from here?
I think if we can just look at the Scheme of script_url and see that it's a
chrome-extension:// scheme
we can allowScriptExtension().
> 
>
https://codereview.chromium.org/1880933002/diff/100001/third_party/WebKit/Sou...
> File
> third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp
> (right):
> 
>
https://codereview.chromium.org/1880933002/diff/100001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp:190:
> if (m_globalScope->isServiceWorkerGlobalScope()) {
> 
> Slightly better:
> 
>   CHECK(m_globalScope->isServiceWorkerGlobalScope() ||
> registeredExtensions().size() == 0);

https://codereview.chromium.org/1880933002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp
(right):

https://codereview.chromium.org/1880933002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp:190:
if (m_globalScope->isServiceWorkerGlobalScope()) {
On 2016/04/21 07:50:44, haraken wrote:
> 
> Slightly better:
> 
>   CHECK(m_globalScope->isServiceWorkerGlobalScope() ||
> registeredExtensions().size() == 0);

Since the latest patch uses ScriptController::registeredExtensions(),
registeredExtensions().size can be > 0 for non service worker global scope.

[haraken, 2016-04-22 02:11:14]

On 2016/04/22 00:17:39, lazyboy wrote:
> On 2016/04/21 07:50:44, haraken wrote:
> > > When I run and print ScriptController::registeredExtensions() for
debugging,
> I
> > > get the following 3, where I would have expected only the SafeBuiltins
one:
> > > extensions::SafeBuiltins
> > > v8/External
> > > v8/LoadTimes
> > > 
> > 
> > I don't know the last two either, but it sounds a bit strange that you're
> going
> > to enable different set of extensions for service workers and the main
thread.
> 
> I found the comprehensive list for these V8 extensions. Actually there can be
> more:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ch...
> > 
> > > Since WorkerOrWorkerScriptController::registeredExtensions() is only
> > > accessed/read in worker thread, isn't using
> > > ScriptController::registeredExtensions() non-thread safe as someone can
add
> > more
> > > stuff to it, possibly from main renderer?
> > 
> > We can add a lock. But in the first place, as far as I understand,
extensions
> > can be added to the list only before Blink creates the first window, right?
> > Otherwise, the extensions won't be enabled on the window.
> 
> Yes, I think that's correct. Thread issues shouldn't happen.
> Likely I was hitting some other crash? I can't repro the crash anymore today.
> Grrr.
> > 
> > > 
> > > If I use ScriptController::registeredExtensions() here, extension service
> > > workers work, but regular web service worker renderer crashes. I haven't
> > figured
> > > out the exact details here.
> > 
> > Hmm. I'm sorry I'm blocking you but I'd like to clarify:
> > 
> > - Whether we should use the same set of extensions for service workers and
the
> > main thread.
> > - If the answer is yes, I think we should unconditionally use
> > ScriptController::registeredExtensions().
> > - If the answer is no, I'm curious why we need different sets of extensions.
> 
> Using the same set of extensions for service workers shouldn't hurt, though we
> only care about SafeBuiltins for extensions/ code.
> 
> I've moved to using ScriptController's set of extensions unconditionally.
> 
> I don't think SafeBuiltins would be much costly, but I don't know about cost
of
> registering other extensions in service worker context.
> 
> It would be better if we can ask embedder (content/renderer)
> whether to allow these extension scripts or not, similar to
FrameLoaderClient's
> allowScriptExtension()
> method.
> If you agree, can you point me how to reach out to content/renderer from here?
> I think if we can just look at the Scheme of script_url and see that it's a
> chrome-extension:// scheme
> we can allowScriptExtension().

This is a nice idea. You can add a public API to public/Platform.h.

Now that we've decided to use the same set of extensions, we no longer need the
registerExtensionForServiceWorkerScript API, right?


> > 
> >
>
https://codereview.chromium.org/1880933002/diff/100001/third_party/WebKit/Sou...
> > File
> >
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/1880933002/diff/100001/third_party/WebKit/Sou...
> >
>
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp:190:
> > if (m_globalScope->isServiceWorkerGlobalScope()) {
> > 
> > Slightly better:
> > 
> >   CHECK(m_globalScope->isServiceWorkerGlobalScope() ||
> > registeredExtensions().size() == 0);
> 
>
https://codereview.chromium.org/1880933002/diff/100001/third_party/WebKit/Sou...
> File
> third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp
> (right):
> 
>
https://codereview.chromium.org/1880933002/diff/100001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp:190:
> if (m_globalScope->isServiceWorkerGlobalScope()) {
> On 2016/04/21 07:50:44, haraken wrote:
> > 
> > Slightly better:
> > 
> >   CHECK(m_globalScope->isServiceWorkerGlobalScope() ||
> > registeredExtensions().size() == 0);
> 
> Since the latest patch uses ScriptController::registeredExtensions(),
> registeredExtensions().size can be > 0 for non service worker global scope.

[yhirano, 2016-04-22 02:28:11]

Sorry for the delay.

https://codereview.chromium.org/1880933002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.h
(right):

https://codereview.chromium.org/1880933002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.h:55:
typedef WTF::Vector<v8::Extension*> V8Extensions;
Please use |using|.

https://codereview.chromium.org/1880933002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp
(right):

https://codereview.chromium.org/1880933002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp:125:
v8::RegisterExtension(extension);
Naive question, is this function thread-safe?

https://codereview.chromium.org/1880933002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.h
(right):

https://codereview.chromium.org/1880933002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.h:55:
typedef WTF::Vector<v8::Extension*> V8Extensions;
Can you put this typedef in WorkerOrWorkletScriptController?

[lazyboy, 2016-04-22 02:48:36]

FYI, I failed to upload my latest patchset before sending out review comments.
Let me ping the thread once I do that, thanks.

[lazyboy, 2016-04-22 03:25:06]

See patchset #7.

On 2016/04/22 02:11:14, haraken wrote:
> On 2016/04/22 00:17:39, lazyboy wrote:
> > On 2016/04/21 07:50:44, haraken wrote:
> > > > When I run and print ScriptController::registeredExtensions() for
> debugging,
> > I
> > > > get the following 3, where I would have expected only the SafeBuiltins
> one:
> > > > extensions::SafeBuiltins
> > > > v8/External
> > > > v8/LoadTimes
> > > > 
> > > 
> > > I don't know the last two either, but it sounds a bit strange that you're
> > going
> > > to enable different set of extensions for service workers and the main
> thread.
> > 
> > I found the comprehensive list for these V8 extensions. Actually there can
be
> > more:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ch...
> > > 
> > > > Since WorkerOrWorkerScriptController::registeredExtensions() is only
> > > > accessed/read in worker thread, isn't using
> > > > ScriptController::registeredExtensions() non-thread safe as someone can
> add
> > > more
> > > > stuff to it, possibly from main renderer?
> > > 
> > > We can add a lock. But in the first place, as far as I understand,
> extensions
> > > can be added to the list only before Blink creates the first window,
right?
> > > Otherwise, the extensions won't be enabled on the window.
> > 
> > Yes, I think that's correct. Thread issues shouldn't happen.
> > Likely I was hitting some other crash? I can't repro the crash anymore
today.
> > Grrr.
> > > 
> > > > 
> > > > If I use ScriptController::registeredExtensions() here, extension
service
> > > > workers work, but regular web service worker renderer crashes. I haven't
> > > figured
> > > > out the exact details here.
> > > 
> > > Hmm. I'm sorry I'm blocking you but I'd like to clarify:
> > > 
> > > - Whether we should use the same set of extensions for service workers and
> the
> > > main thread.
> > > - If the answer is yes, I think we should unconditionally use
> > > ScriptController::registeredExtensions().
> > > - If the answer is no, I'm curious why we need different sets of
extensions.
> > 
> > Using the same set of extensions for service workers shouldn't hurt, though
we
> > only care about SafeBuiltins for extensions/ code.
> > 
> > I've moved to using ScriptController's set of extensions unconditionally.
> > 
> > I don't think SafeBuiltins would be much costly, but I don't know about cost
> of
> > registering other extensions in service worker context.
> > 
> > It would be better if we can ask embedder (content/renderer)
> > whether to allow these extension scripts or not, similar to
> FrameLoaderClient's
> > allowScriptExtension()
> > method.
> > If you agree, can you point me how to reach out to content/renderer from
here?
> > I think if we can just look at the Scheme of script_url and see that it's a
> > chrome-extension:// scheme
> > we can allowScriptExtension().
> 
> This is a nice idea. You can add a public API to public/Platform.h.

Let's see if I can figure that out. Any hints appreciated.
> 
> Now that we've decided to use the same set of extensions, we no longer need
the
> registerExtensionForServiceWorkerScript API, right?
Yes, it's gone.
> 
> 
> > > 
> > >
> >
>
https://codereview.chromium.org/1880933002/diff/100001/third_party/WebKit/Sou...
> > > File
> > >
> third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1880933002/diff/100001/third_party/WebKit/Sou...
> > >
> >
>
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp:190:
> > > if (m_globalScope->isServiceWorkerGlobalScope()) {
> > > 
> > > Slightly better:
> > > 
> > >   CHECK(m_globalScope->isServiceWorkerGlobalScope() ||
> > > registeredExtensions().size() == 0);
> > 
> >
>
https://codereview.chromium.org/1880933002/diff/100001/third_party/WebKit/Sou...
> > File
> >
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/1880933002/diff/100001/third_party/WebKit/Sou...
> >
>
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp:190:
> > if (m_globalScope->isServiceWorkerGlobalScope()) {
> > On 2016/04/21 07:50:44, haraken wrote:
> > > 
> > > Slightly better:
> > > 
> > >   CHECK(m_globalScope->isServiceWorkerGlobalScope() ||
> > > registeredExtensions().size() == 0);
> > 
> > Since the latest patch uses ScriptController::registeredExtensions(),
> > registeredExtensions().size can be > 0 for non service worker global scope.

[lazyboy, 2016-04-22 03:25:25]

https://codereview.chromium.org/1880933002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp
(right):

https://codereview.chromium.org/1880933002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp:125:
v8::RegisterExtension(extension);
On 2016/04/22 02:28:11, yhirano wrote:
> Naive question, is this function thread-safe?

Good question, It seems this would eventually call
RegisteredExtension::Register() in v8, which seems to build some kind of link
between |extension|.

I've got rid of this though after haraken@'s suggestion.

https://codereview.chromium.org/1880933002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.h
(right):

https://codereview.chromium.org/1880933002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.h:55:
typedef WTF::Vector<v8::Extension*> V8Extensions;
On 2016/04/22 02:28:11, yhirano wrote:
> Can you put this typedef in WorkerOrWorkletScriptController?

Sorry for the mis-upload, patchset #7 should be the latest and this change is
gone from there.

[yhirano, 2016-04-22 04:02:31]

bindings/ lgtm

[haraken, 2016-04-22 18:33:21]

Thanks for simplifying the CL significantly! WebKit LGTM.

https://codereview.chromium.org/1880933002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp
(right):

https://codereview.chromium.org/1880933002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp:175:
const V8Extensions& extensions = ScriptController::registeredExtensions();

Nit: Now it doesn't make sense to put the registered extensions on
ScriptController since they are used by WorkerOrWorkletScriptController as well.
Maybe we should move them to V8Initializer.

(You don't need to address the issue in this CL.)

[lazyboy, 2016-04-22 20:45:17]

lazyboy@chromium.org changed reviewers:
+ jochen@chromium.org

[lazyboy, 2016-04-22 20:45:19]

+jochen for OWNERS review:

public/ changes:

content/child/blink_platform_impl.*
third_party/WebKit/public/platform/Platform.h
content/public/common/content_client.*
content/public/render/content_renderer_client.*

implementations:

chrome/common/chrome_content_client.*
chorme/renderer/chrome_content_renderer_client.*

https://codereview.chromium.org/1880933002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp
(right):

https://codereview.chromium.org/1880933002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp:175:
const V8Extensions& extensions = ScriptController::registeredExtensions();
On 2016/04/22 18:33:21, haraken wrote:
> 
> Nit: Now it doesn't make sense to put the registered extensions on
> ScriptController since they are used by WorkerOrWorkletScriptController as
well.
> Maybe we should move them to V8Initializer.
> 
> (You don't need to address the issue in this CL.)

OK, I'll move it in a separate CL, I've also fixed the todo above to check
whether this worker's url is an extension URL or not, through Platform.

[jochen (gone - plz use gerrit), 2016-04-27 10:09:40]

I don't have enough context from the linked bug to why we'd want to mix service
workers and extensions.

Could you share the design doc please?

[lazyboy, 2016-04-28 04:57:46]

On 2016/04/27 10:09:40, jochen wrote:
> I don't have enough context from the linked bug to why we'd want to mix
service
> workers and extensions.
> 
> Could you share the design doc please?

I've added some context here (google.com):
https://docs.google.com/document/d/1BldpJdIyTCPguq_Dh8jfqNsHwO73uDyTguZ5fOaAA...

[jochen (gone - plz use gerrit), 2016-04-28 15:50:19]

hum, so the initial document says that this is a chance to reconcile extension
APIs to be more webby, e.g. using promises.

But now it looks like you're "just" exposing the existing APIs. Was there some
discussion whether or not do this?

Esp. the way the service worker registers for events is very different from how
extensions register for extensions.

[jochen (gone - plz use gerrit), 2016-04-28 15:54:06]

I'd propose to send an email to platform-architecture-dev@chromium.org to get
the attention of the web platform folks

[lazyboy, 2016-04-28 18:13:56]

On 2016/04/28 15:50:19, jochen wrote:
> hum, so the initial document says that this is a chance to reconcile extension
> APIs to be more webby, e.g. using promises.
> 
> But now it looks like you're "just" exposing the existing APIs. Was there some
> discussion whether or not do this?

This has come up before, regardless of Extension Service Workers:
https://crbug.com/328932
I just talked to Devlin, and we agree it would be nice to have promise based
APIs.
We didn't do this as we wouldn't win us as much: It's much easier for
extension developers to write a shim that wraps callback APIs and turns them
into
promise based equivalent but its a bit more work to support that in extension
system.
I wouldn't consider this a blocker for this CL, as this can be thought as a
syntactic
sugar. Let me know if you think otherwise.

> 
> Esp. the way the service worker registers for events is very different from
how
> extensions register for extensions.

Cool, I'll send a quick mail out to  platform-architecture-dev@chromium.org
about
this.

Thanks.

[jochen (gone - plz use gerrit), 2016-05-03 11:31:28]

lgtm for now

if the architecture teams wants to propose changes, this can happen after this
has landed

[lazyboy, 2016-05-03 20:54:16]

lazyboy@chromium.org changed reviewers:
+ dcheng@chromium.org

[lazyboy, 2016-05-03 20:54:18]

+dcheng for IPC review:
extensions/common/extension_messages.h
ipc/ipc_message_start.h

[lazyboy, 2016-05-09 18:32:50]

@dcheng, ping.

[dcheng, 2016-05-11 23:41:18]

https://codereview.chromium.org/1880933002/diff/180001/extensions/browser/ext...
File extensions/browser/extension_function_dispatcher.cc (right):

https://codereview.chromium.org/1880933002/diff/180001/extensions/browser/ext...
extensions/browser/extension_function_dispatcher.cc:417:
std::unique_ptr<UIThreadWorkerResponseCallbackWrapper>(
base::WrapUnique(callback_wrapper) will save a lot of typing.

https://codereview.chromium.org/1880933002/diff/180001/extensions/browser/ext...
File extensions/browser/extension_function_dispatcher.h (right):

https://codereview.chromium.org/1880933002/diff/180001/extensions/browser/ext...
extensions/browser/extension_function_dispatcher.h:177: using
UIThreadWorkerResponseCallbackWrapperMapKey = std::pair<int, int>;
This should just be a simple struct. The typedef is extremely long, and ->first
and ->second are inscrutable.

[lazyboy, 2016-05-12 00:21:23]

https://codereview.chromium.org/1880933002/diff/180001/extensions/browser/ext...
File extensions/browser/extension_function_dispatcher.cc (right):

https://codereview.chromium.org/1880933002/diff/180001/extensions/browser/ext...
extensions/browser/extension_function_dispatcher.cc:417:
std::unique_ptr<UIThreadWorkerResponseCallbackWrapper>(
On 2016/05/11 23:41:18, dcheng wrote:
> base::WrapUnique(callback_wrapper) will save a lot of typing.

Done, thanks.

https://codereview.chromium.org/1880933002/diff/180001/extensions/browser/ext...
File extensions/browser/extension_function_dispatcher.h (right):

https://codereview.chromium.org/1880933002/diff/180001/extensions/browser/ext...
extensions/browser/extension_function_dispatcher.h:177: using
UIThreadWorkerResponseCallbackWrapperMapKey = std::pair<int, int>;
On 2016/05/11 23:41:18, dcheng wrote:
> This should just be a simple struct. The typedef is extremely long, and
->first
> and ->second are inscrutable.

Done.

[dcheng, 2016-05-12 17:17:33]

How do we validate worker_thread_id and embedded_worker_id in the browser
process? What happens if the renderer lies?

https://codereview.chromium.org/1880933002/diff/200001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp
(right):

https://codereview.chromium.org/1880933002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp:171:
if (m_globalScope->isServiceWorkerGlobalScope() &&
Platform::current()->allowScriptExtensionForServiceWorker(static_cast<WorkerGlobalScope*>(m_globalScope.get())->url()))
{
toWorkerGlobalScope(m_globalScope.get())

rather than using static_cast directly.

[lazyboy, 2016-05-17 05:13:09]

re: renderer lying  about embedded_worker_id and worker_thread_id:
ExtensionFunctionDispatcher (browser/ process) uses these in conjunction with
render_process_id, so without a correct render_process_id, these ids cannot
perform unwanted access.

https://codereview.chromium.org/1880933002/diff/200001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp
(right):

https://codereview.chromium.org/1880933002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp:171:
if (m_globalScope->isServiceWorkerGlobalScope() &&
Platform::current()->allowScriptExtensionForServiceWorker(static_cast<WorkerGlobalScope*>(m_globalScope.get())->url()))
{
On 2016/05/12 17:17:33, dcheng wrote:
> toWorkerGlobalScope(m_globalScope.get())
> 
> rather than using static_cast directly.

Done.

[dcheng, 2016-05-17 06:16:38]

https://codereview.chromium.org/1880933002/diff/220001/extensions/browser/ext...
File extensions/browser/extension_function_dispatcher.cc (right):

https://codereview.chromium.org/1880933002/diff/220001/extensions/browser/ext...
extensions/browser/extension_function_dispatcher.cc:265: // TODO(lazyboy): Kill
the offending process.
This shouldn't be too hard, can we just base this off something like
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
?

[lazyboy, 2016-05-17 17:35:16]

https://codereview.chromium.org/1880933002/diff/220001/extensions/browser/ext...
File extensions/browser/extension_function_dispatcher.cc (right):

https://codereview.chromium.org/1880933002/diff/220001/extensions/browser/ext...
extensions/browser/extension_function_dispatcher.cc:265: // TODO(lazyboy): Kill
the offending process.
On 2016/05/17 06:16:37, dcheng wrote:
> This shouldn't be too hard, can we just base this off something like
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> ?

Done.

[haraken, 2016-05-17 23:31:44]

I noticed that this CL would not be enough to enable extensions on SW, because
some methods (e.g., DOMWrapperWorld.cpp) are assuming that isolated worlds exist
only on the main thread (e.g., we have isMainThread() checks).

I'm okay with landing this CL as is, but you'll need to address the issue in a
follow up.

[lazyboy, 2016-05-18 23:46:49]

On 2016/05/17 23:31:44, haraken wrote:
> I noticed that this CL would not be enough to enable extensions on SW, because
> some methods (e.g., DOMWrapperWorld.cpp) are assuming that isolated worlds
exist
> only on the main thread (e.g., we have isMainThread() checks).
> 
> I'm okay with landing this CL as is, but you'll need to address the issue in a
> follow up.

Sure, I'll follow up with that, can you provide a bit details on a crbug? I can
take it from there, thanks.

@dcheng, ping.

[dcheng, 2016-05-18 23:54:00]

https://codereview.chromium.org/1880933002/diff/220001/extensions/browser/ext...
File extensions/browser/extension_function_dispatcher.cc (right):

https://codereview.chromium.org/1880933002/diff/220001/extensions/browser/ext...
extensions/browser/extension_function_dispatcher.cc:265: // TODO(lazyboy): Kill
the offending process.
On 2016/05/17 at 17:35:16, lazyboy wrote:
> On 2016/05/17 06:16:37, dcheng wrote:
> > This shouldn't be too hard, can we just base this off something like
> >
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> > ?
> 
> Done.

Can we share the kill logic rather than copy and paste?

[lazyboy, 2016-05-19 00:25:46]

https://codereview.chromium.org/1880933002/diff/220001/extensions/browser/ext...
File extensions/browser/extension_function_dispatcher.cc (right):

https://codereview.chromium.org/1880933002/diff/220001/extensions/browser/ext...
extensions/browser/extension_function_dispatcher.cc:265: // TODO(lazyboy): Kill
the offending process.
On 2016/05/18 23:54:00, dcheng wrote:
> On 2016/05/17 at 17:35:16, lazyboy wrote:
> > On 2016/05/17 06:16:37, dcheng wrote:
> > > This shouldn't be too hard, can we just base this off something like
> > >
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> > > ?
> > 
> > Done.
> 
> Can we share the kill logic rather than copy and paste?

Done.

[dcheng, 2016-05-19 00:28:04]

ipc lgtm

[falken, 2016-05-23 04:51:24]

I think now I only need to review:
content/renderer/service_worker/service_worker_context_client.cc

That lgtm

[lazyboy, 2016-05-23 23:21:27]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2016-05-23 23:21:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, haraken@chromium.org, yhirano@chromium.org,
jochen@chromium.org, dcheng@chromium.org, falken@chromium.org
Link to the patchset: https://codereview.chromium.org/1880933002/#ps320001
(title: "sync @tott")

[commit-bot: I haz the power, 2016-05-23 23:22:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1880933002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1880933002/320001

[commit-bot: I haz the power, 2016-05-24 00:55:26]

Description was changed from

==========
Begin to enable extension APIs in Extension Service Worker.

This CL adds "tabs" API support in extension SW and adds an end-to-end
test for it.
This CL also puts the feature behind a runtime flag which is disabled for non
trunk builds by default.

Some known issues:
1) Lifetime improvement of UIThreadWorkerResponseCallbackWrapper is missing.
I am currently cleaning up on RPH shutdown, but this is not idea, we should
be cleaning up on worker thread shutdown.

2) IPC send<->receive from/to Worker Thread can be made better, see
comment above WorkerThreadDispatcher in patch set #1.

3) Currently if an extension is not "active" in a process, then API calls
from that process is not classified correctly. I'm using
"extension_service_worker" context, which allows running tabs API. In
theory since a worker cannot be spawned from outside of the extension
process (TODO: Verify this), we can <fingers_crossed>safely</fingers_crossed>
classify this as a blessed context.

BUG=602442
Test=Register an extension with "tabs" permission, register a service
worker from the extension and call chrome.tabs.create() from SW code!
Expect it to work
==========

to

==========
Begin to enable extension APIs in Extension Service Worker.

This CL adds "tabs" API support in extension SW and adds an end-to-end
test for it.
This CL also puts the feature behind a runtime flag which is disabled for non
trunk builds by default.

Some known issues:
1) Lifetime improvement of UIThreadWorkerResponseCallbackWrapper is missing.
I am currently cleaning up on RPH shutdown, but this is not idea, we should
be cleaning up on worker thread shutdown.

2) IPC send<->receive from/to Worker Thread can be made better, see
comment above WorkerThreadDispatcher in patch set #1.

3) Currently if an extension is not "active" in a process, then API calls
from that process is not classified correctly. I'm using
"extension_service_worker" context, which allows running tabs API. In
theory since a worker cannot be spawned from outside of the extension
process (TODO: Verify this), we can <fingers_crossed>safely</fingers_crossed>
classify this as a blessed context.

BUG=602442
Test=Register an extension with "tabs" permission, register a service
worker from the extension and call chrome.tabs.create() from SW code!
Expect it to work
==========

[commit-bot: I haz the power, 2016-05-24 00:55:30]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2016-05-24 00:56:36]

Description was changed from

==========
Begin to enable extension APIs in Extension Service Worker.

This CL adds "tabs" API support in extension SW and adds an end-to-end
test for it.
This CL also puts the feature behind a runtime flag which is disabled for non
trunk builds by default.

Some known issues:
1) Lifetime improvement of UIThreadWorkerResponseCallbackWrapper is missing.
I am currently cleaning up on RPH shutdown, but this is not idea, we should
be cleaning up on worker thread shutdown.

2) IPC send<->receive from/to Worker Thread can be made better, see
comment above WorkerThreadDispatcher in patch set #1.

3) Currently if an extension is not "active" in a process, then API calls
from that process is not classified correctly. I'm using
"extension_service_worker" context, which allows running tabs API. In
theory since a worker cannot be spawned from outside of the extension
process (TODO: Verify this), we can <fingers_crossed>safely</fingers_crossed>
classify this as a blessed context.

BUG=602442
Test=Register an extension with "tabs" permission, register a service
worker from the extension and call chrome.tabs.create() from SW code!
Expect it to work
==========

to

==========
Begin to enable extension APIs in Extension Service Worker.

This CL adds "tabs" API support in extension SW and adds an end-to-end
test for it.
This CL also puts the feature behind a runtime flag which is disabled for non
trunk builds by default.

Some known issues:
1) Lifetime improvement of UIThreadWorkerResponseCallbackWrapper is missing.
I am currently cleaning up on RPH shutdown, but this is not idea, we should
be cleaning up on worker thread shutdown.

2) IPC send<->receive from/to Worker Thread can be made better, see
comment above WorkerThreadDispatcher in patch set #1.

3) Currently if an extension is not "active" in a process, then API calls
from that process is not classified correctly. I'm using
"extension_service_worker" context, which allows running tabs API. In
theory since a worker cannot be spawned from outside of the extension
process (TODO: Verify this), we can <fingers_crossed>safely</fingers_crossed>
classify this as a blessed context.

BUG=602442
Test=Register an extension with "tabs" permission, register a service
worker from the extension and call chrome.tabs.create() from SW code!
Expect it to work

Committed: https://crrev.com/ee4adef0b70f13cb80a886ef639fc910c0b5ce6f
Cr-Commit-Position: refs/heads/master@{#395494}
==========

[commit-bot: I haz the power, 2016-05-24 00:56:38]

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