Expose the ServiceWorker's v8 context to embedders.

Expose the ServiceWorker's v8 context to embedders.

BUG=390726

[Jeffrey Yasskin, 2014-07-12 04:47:30]

I'm hoping to use this in
https://github.com/jyasskin/chromium/compare/service-worker-apps...ext-sw-hel...
Does it look close to the right way to pass out the v8::Context? For WebFrames,
we pass the context in WebFrameClient::didCreateScriptContext, but it seems less
necessary to use a callback here since the script context is the whole point of
a ServiceWorkerScriptContext.

[abarth-chromium, 2014-07-14 00:50:57]

On 2014/07/12 at 04:47:30, jyasskin wrote:
> I'm hoping to use this in
https://github.com/jyasskin/chromium/compare/service-worker-apps...ext-sw-hel...
> Does it look close to the right way to pass out the v8::Context? For
WebFrames, we pass the context in WebFrameClient::didCreateScriptContext, but it
seems less necessary to use a callback here since the script context is the
whole point of a ServiceWorkerScriptContext.

WebFrames can have many contexts over time, so you need a notification pathway
for the embedder to see when it changes.

When is it safe for the embedder to call this function?  Is there ever a time
during initialization of this object when calling this function returns an empty
context?

If not, LGTM

[Jeffrey Yasskin, 2014-07-14 02:55:01]

On Sun, Jul 13, 2014 at 5:50 PM,  <abarth@chromium.org> wrote:
> On 2014/07/12 at 04:47:30, jyasskin wrote:
>>
>> I'm hoping to use this in
>
>
https://github.com/jyasskin/chromium/compare/service-worker-apps...ext-sw-hel...
>>
>> Does it look close to the right way to pass out the v8::Context? For
>
> WebFrames, we pass the context in WebFrameClient::didCreateScriptContext,
> but it
> seems less necessary to use a callback here since the script context is the
> whole point of a ServiceWorkerScriptContext.
>
> WebFrames can have many contexts over time, so you need a notification
> pathway
> for the embedder to see when it changes.

For Service Workers, I _believe_ that the Global Scope has exactly the
same lifetime as its javascript context, but Dominic will know better
than me.

> When is it safe for the embedder to call this function?  Is there ever a
> time
> during initialization of this object when calling this function returns an
> empty
> context?

I'll trace through tomorrow to check unless Dominic chimes in overnight.

> If not, LGTM
>
> https://codereview.chromium.org/383063008/

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

[dominicc (has gone to gerrit), 2014-07-14 04:54:33]

On 2014/07/14 02:55:01, Jeffrey Yasskin wrote:
> On Sun, Jul 13, 2014 at 5:50 PM,  <mailto:abarth@chromium.org> wrote:
> > On 2014/07/12 at 04:47:30, jyasskin wrote:
> >>
> >> I'm hoping to use this in
> >
> >
>
https://github.com/jyasskin/chromium/compare/service-worker-apps...ext-sw-hel...
> >>
> >> Does it look close to the right way to pass out the v8::Context? For
> >
> > WebFrames, we pass the context in WebFrameClient::didCreateScriptContext,
> > but it
> > seems less necessary to use a callback here since the script context is the
> > whole point of a ServiceWorkerScriptContext.
> >
> > WebFrames can have many contexts over time, so you need a notification
> > pathway
> > for the embedder to see when it changes.
> 
> For Service Workers, I _believe_ that the Global Scope has exactly the
> same lifetime as its javascript context, but Dominic will know better
> than me.
> 
> > When is it safe for the embedder to call this function?  Is there ever a
> > time
> > during initialization of this object when calling this function returns an
> > empty
> > context?
> 
> I'll trace through tomorrow to check unless Dominic chimes in overnight.

This is more of a worker question so I did some reading myself.

Startup:

There appears to be a small window where the context is empty:
WorkerThread::workerThread creates the context, but not before it has disclosed
the ServiceWorkerGlobalScopeProxy via
WorkerReportingProxy::workerGlobalScopeStarted. It might be possible to do that
disclosure a wee bit later? I'm not sure how other workers use that hook.

Later:

It looks like there are times when it would not be safe to call this, eg
WorkerGlobalScope::clearScript, called by the shutdown task and dispose, will
cause script() to be null.

In the Service Worker world that would be triggered by
ServiceWorkerGlobalScopeProxy::workerGlobalScopeClosed; it looks like the script
can do that at any time via close()?

> > If not, LGTM
> >
> > https://codereview.chromium.org/383063008/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

[Jeffrey Yasskin, 2014-07-15 03:59:03]

On 2014/07/14 04:54:33, dominicc wrote:
> On 2014/07/14 02:55:01, Jeffrey Yasskin wrote:
> > On Sun, Jul 13, 2014 at 5:50 PM,  <mailto:abarth@chromium.org> wrote:
> > > On 2014/07/12 at 04:47:30, jyasskin wrote:
> > >>
> > >> I'm hoping to use this in
> > >
> > >
> >
>
https://github.com/jyasskin/chromium/compare/service-worker-apps...ext-sw-hel...
> > >>
> > >> Does it look close to the right way to pass out the v8::Context? For
> > >
> > > WebFrames, we pass the context in WebFrameClient::didCreateScriptContext,
> > > but it
> > > seems less necessary to use a callback here since the script context is
the
> > > whole point of a ServiceWorkerScriptContext.
> > >
> > > WebFrames can have many contexts over time, so you need a notification
> > > pathway
> > > for the embedder to see when it changes.
> > 
> > For Service Workers, I _believe_ that the Global Scope has exactly the
> > same lifetime as its javascript context, but Dominic will know better
> > than me.
> > 
> > > When is it safe for the embedder to call this function?  Is there ever a
> > > time
> > > during initialization of this object when calling this function returns an
> > > empty
> > > context?
> > 
> > I'll trace through tomorrow to check unless Dominic chimes in overnight.
> 
> This is more of a worker question so I did some reading myself.
> 
> Startup:
> 
> There appears to be a small window where the context is empty:
> WorkerThread::workerThread creates the context, but not before it has
disclosed
> the ServiceWorkerGlobalScopeProxy via
> WorkerReportingProxy::workerGlobalScopeStarted. It might be possible to do
that
> disclosure a wee bit later? I'm not sure how other workers use that hook.
>
> Later:
> 
> It looks like there are times when it would not be safe to call this, eg
> WorkerGlobalScope::clearScript, called by the shutdown task and dispose, will
> cause script() to be null.
> 
> In the Service Worker world that would be triggered by
> ServiceWorkerGlobalScopeProxy::workerGlobalScopeClosed; it looks like the
script
> can do that at any time via close()?

I'm looking into this, and writing a test to make sure it keeps working in
whatever way we document. I'm expecting to switch the order of
script->initializeContextIfNeeded() and workerGlobalScopeStarted() in
WorkerThread::workerThread(), and document that closing the global scope causes
v8Context() to return a null handle thenceforth.

The test is turning out to be tricky: it appears to be the first unittest for a
worker in Source/web/tests, and I'm having to guess at several things to get
control to pass between the threads correctly. I'm happy to push on through, but
if a Blink-side test is obviously the wrong way to go for this, feel free to
redirect me.

Thanks,
Jeffrey

[Jeffrey Yasskin, 2014-07-16 23:14:58]

I had to do some fairly unnatural things to get this test to run, so I wonder if
this ought to be live in content instead of blink. The reason to keep it in
blink would be that it's intended to catch changes in the blink public API's
behavior. It's also possible that I'd have to do just as many unnatural things
to make it work in content.

I didn't need to change WorkerThread at all to make this pass. I haven't yet dug
into where the Context actually gets created.

https://codereview.chromium.org/383063008/diff/40001/Source/web/ServiceWorker...
File Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/383063008/diff/40001/Source/web/ServiceWorker...
Source/web/ServiceWorkerGlobalScopeProxy.cpp:67: void
ServiceWorkerGlobalScopeProxy::evaluate(const ScriptSourceCode& javascript)
This is used in the test, but doesn't currently need to be exposed outside of
Blink.

https://codereview.chromium.org/383063008/diff/40001/Source/web/WebEmbeddedWo...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/383063008/diff/40001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.cpp:172: void
WebEmbeddedWorkerImpl::postTask(PassOwnPtr<ExecutionContextTask> task)
This is again used by the test.

In Content, this purpose is served by
WorkerTaskRunner::PostTask(worker_thread_id), so it probably won't need to be
exposed from the Blink API. That mechanism works by recording all WorkerRunLoops
into a global variable in BlinkPlatformImpl::didStartWorkerRunLoop().

https://codereview.chromium.org/383063008/diff/40001/Source/web/tests/Service...
File Source/web/tests/ServiceWorkerGlobalScopeProxyTest.cpp (right):

https://codereview.chromium.org/383063008/diff/40001/Source/web/tests/Service...
Source/web/tests/ServiceWorkerGlobalScopeProxyTest.cpp:52: class RunLoop {
I was kind of shocked not to find anything like this in the existing test
helpers. Does it really not exist in Blink, or did I miss it? Should I put it
somewhere reusable?

https://codereview.chromium.org/383063008/diff/40001/Source/web/tests/Service...
Source/web/tests/ServiceWorkerGlobalScopeProxyTest.cpp:116:
Platform::current()->unitTestSupport()->serveAsynchronousMockedRequests();
This gets flagged by
https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi....
I can't use pumpPendingRequests(WebFrame*) directly because WebEmbeddedWorker
doesn't expose the WebFrame it uses for loading. I could, of course, expose
that, but it seems less relevant to the overall abstraction than the other
things I'm exposing.

Should I pull this use out into a WorkerTestHelpers file, along with the
RunLoop? Something else?

[Jeffrey Yasskin, 2014-07-16 23:31:07]

On 2014/07/16 23:14:58, Jeffrey Yasskin wrote:
> I didn't need to change WorkerThread at all to make this pass. I haven't yet
dug
> into where the Context actually gets created.

Ah, it's created by the first evaluate() call in
ServiceWorkerGlobalScope::create().

WorkerGlobalScope::clearScript() is only called after the MessageQueue is
killed, so only cleanup tasks can see a null WorkerScriptController*.

[dominicc (has gone to gerrit), 2014-07-17 02:43:45]

Isn't this basically racy? Between workerContextStarted, and actually creating
the context on the worker thread.

https://codereview.chromium.org/383063008/diff/60001/Source/web/ServiceWorker...
File Source/web/ServiceWorkerGlobalScopeProxy.h (right):

https://codereview.chromium.org/383063008/diff/60001/Source/web/ServiceWorker...
Source/web/ServiceWorkerGlobalScopeProxy.h:72: void evaluate(const
WebCore::ScriptSourceCode&);
No result?

https://codereview.chromium.org/383063008/diff/60001/Source/web/tests/Service...
File Source/web/tests/ServiceWorkerGlobalScopeProxyTest.cpp (right):

https://codereview.chromium.org/383063008/diff/60001/Source/web/tests/Service...
Source/web/tests/ServiceWorkerGlobalScopeProxyTest.cpp:62: void Run()
Run -> run

https://codereview.chromium.org/383063008/diff/60001/Source/web/tests/Service...
Source/web/tests/ServiceWorkerGlobalScopeProxyTest.cpp:73: void QuitWhenIdle()
quitWhenIdle.

https://codereview.chromium.org/383063008/diff/60001/Source/web/tests/Service...
Source/web/tests/ServiceWorkerGlobalScopeProxyTest.cpp:92: virtual void run()
The lack of vertical whitespace here seems inconsistent.

https://codereview.chromium.org/383063008/diff/60001/Source/web/tests/Service...
Source/web/tests/ServiceWorkerGlobalScopeProxyTest.cpp:106: // Continuously
pumps the mocked-requests queue during the current execution of runLoop->Run(),
or next if Run() isn't currently running.
Run -> run, right?

https://codereview.chromium.org/383063008/diff/60001/Source/web/tests/Service...
Source/web/tests/ServiceWorkerGlobalScopeProxyTest.cpp:118: if
(!m_runLoop->isQuitting()) {
No {}s for single line bodies unless it is clearer that way.

https://codereview.chromium.org/383063008/diff/60001/Source/web/tests/Service...
Source/web/tests/ServiceWorkerGlobalScopeProxyTest.cpp:127: class
V8ContextTestSWContextClient : public WebServiceWorkerContextClient {
SW -> ServiceWorker

I am all for brevity but you would be the first to achieve it this way. Why not
just TestContextClient?

[Jeffrey Yasskin, 2014-07-17 16:19:33]

On 2014/07/17 02:43:45, dominicc wrote:
> Isn't this basically racy? Between workerContextStarted, and actually creating
> the context on the worker thread.

workerContextStarted is called on the worker thread
(https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...),
so that's not a race. Let me know if you see other ones though.

https://codereview.chromium.org/383063008/diff/60001/Source/web/ServiceWorker...
File Source/web/ServiceWorkerGlobalScopeProxy.h (right):

https://codereview.chromium.org/383063008/diff/60001/Source/web/ServiceWorker...
Source/web/ServiceWorkerGlobalScopeProxy.h:72: void evaluate(const
WebCore::ScriptSourceCode&);
On 2014/07/17 02:43:44, dominicc wrote:
> No result?

Yeah, WorkerScriptController::evaluate() doesn't provide one. I could expose the
error object instead of always passing it to reportException() if you like.

https://codereview.chromium.org/383063008/diff/60001/Source/web/tests/Service...
File Source/web/tests/ServiceWorkerGlobalScopeProxyTest.cpp (right):

https://codereview.chromium.org/383063008/diff/60001/Source/web/tests/Service...
Source/web/tests/ServiceWorkerGlobalScopeProxyTest.cpp:62: void Run()
On 2014/07/17 02:43:44, dominicc wrote:
> Run -> run

Oops. Got my coding conventions confused.

https://codereview.chromium.org/383063008/diff/60001/Source/web/tests/Service...
Source/web/tests/ServiceWorkerGlobalScopeProxyTest.cpp:73: void QuitWhenIdle()
On 2014/07/17 02:43:44, dominicc wrote:
> quitWhenIdle.

Done.

https://codereview.chromium.org/383063008/diff/60001/Source/web/tests/Service...
Source/web/tests/ServiceWorkerGlobalScopeProxyTest.cpp:92: virtual void run()
On 2014/07/17 02:43:44, dominicc wrote:
> The lack of vertical whitespace here seems inconsistent.

Done.

https://codereview.chromium.org/383063008/diff/60001/Source/web/tests/Service...
Source/web/tests/ServiceWorkerGlobalScopeProxyTest.cpp:106: // Continuously
pumps the mocked-requests queue during the current execution of runLoop->Run(),
or next if Run() isn't currently running.
On 2014/07/17 02:43:44, dominicc wrote:
> Run -> run, right?

Done.

https://codereview.chromium.org/383063008/diff/60001/Source/web/tests/Service...
Source/web/tests/ServiceWorkerGlobalScopeProxyTest.cpp:118: if
(!m_runLoop->isQuitting()) {
On 2014/07/17 02:43:44, dominicc wrote:
> No {}s for single line bodies unless it is clearer that way.

After goto-fail, I've been more inclined to keep the braces always (not that I
always remember), but I can conform to the surrounding style.

https://codereview.chromium.org/383063008/diff/60001/Source/web/tests/Service...
Source/web/tests/ServiceWorkerGlobalScopeProxyTest.cpp:127: class
V8ContextTestSWContextClient : public WebServiceWorkerContextClient {
On 2014/07/17 02:43:44, dominicc wrote:
> SW -> ServiceWorker
> 
> I am all for brevity but you would be the first to achieve it this way. Why
not
> just TestContextClient?

If it's just TestContextClient, that'll make it harder to write future tests, so
I'm prefixing the name of the test. I can drop "ServiceWorker" though. I'd just
define the class inside the test, but it's used as a template argument in
createCrossThreadTask().

[Jeffrey Yasskin, 2014-07-17 18:32:09]

https://codereview.chromium.org/383063008/diff/100001/Source/web/tests/Servic...
File Source/web/tests/ServiceWorkerGlobalScopeProxyTest.cpp (right):

https://codereview.chromium.org/383063008/diff/100001/Source/web/tests/Servic...
Source/web/tests/ServiceWorkerGlobalScopeProxyTest.cpp:118:
Platform::current()->unitTestSupport()->serveAsynchronousMockedRequests();
dcheng says that this call is probably the best we can do now, and that I should
just force past the presubmit. In the long run, we think we should add a flag to
WebURLLoaderMockFactory that makes its LoadAsynchronouly method automatically
post a task to serve the request.

[dominicc (has gone to gerrit), 2014-07-19 01:08:49]

The calls to evaluate in ServiceWorkerGlobalScope::create this relies on will be
going away when the native Cache implementation lands...

LGTM to land this to keep moving, particularly since we have the test, but we
may need to add the "v8 context created" protocol or wean callers from the
context down the road.

[Jeffrey Yasskin, 2014-07-19 01:29:08]

On 2014/07/19 01:08:49, dominicc wrote:
> The calls to evaluate in ServiceWorkerGlobalScope::create this relies on will
be
> going away when the native Cache implementation lands...
> 
> LGTM to land this to keep moving, particularly since we have the test, but we
> may need to add the "v8 context created" protocol or wean callers from the
> context down the road.

Apps-on-ServiceWorkers aren't happening in the near term, so we don't need this
change anymore. Closing. Sorry for the wasted review time.