Check whether the shadow page has been loaded in WebEmbeddedWorkerImpl::terminateWorkerContext().

Check whether the shadow page has been loaded in WebEmbeddedWorkerImpl::terminateWorkerContext().

WebEmbeddedWorkerImpl::terminateWorkerContext() could be called between
loadShadowPage() and didFinishDocumentLoad(). But the current code doesn't
handle this situation correctly.

BUG=499646, 497029
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197243

[horo, 2015-06-16 06:06:22]

horo@chromium.org changed reviewers:
+ kinuko@chromium.org

[horo, 2015-06-16 06:06:23]

kinuko@
Could you please review this?

I said

> I think we can fix this problem by checking |m_askedToTerminate| in the top of
WebEmbeddedWorkerImpl::didFinishDocumentLoad().
> WebEmbeddedWorkerImpl::terminateWorkerContext() could be called between
loadShadowPage() and didFinishDocumentLoad(), but the current code doesn't
handle this situation correctly.

in https://code.google.com/p/chromium/issues/detail?id=499646#c11.

And I created the Patch Set 1 for it.
But, I changed to check whether the shadow page has been loaded in
WebEmbeddedWorkerImpl::terminateWorkerContext() in Patch Set 2.

It is because WebEmbeddedWorkerImpl::terminateWorkerContext() is called soon
after the browser process sends EmbeddedWorkerMsg_StopWorker message,
but WebEmbeddedWorkerImpl::didFinishDocumentLoad() is called when the user opens
Chrome.
I think the renderer process should handle StopWorker message as soon as
possible.

[falken, 2015-06-16 06:48:14]

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

[falken, 2015-06-16 06:48:15]

https://codereview.chromium.org/1187813010/diff/20001/Source/web/WebEmbeddedW...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/1187813010/diff/20001/Source/web/WebEmbeddedW...
Source/web/WebEmbeddedWorkerImpl.cpp:157: m_askedToTerminate = true;
Maybe we also need a workerContextFailedtoStart here if m_loadingShadowPage is
true?

https://codereview.chromium.org/1187813010/diff/20001/Source/web/WebEmbeddedW...
Source/web/WebEmbeddedWorkerImpl.cpp:352: ASSERT(m_workerContextClient);
Should we deal with m_askedToTerminate here? As I understand it, the bug is that
terminate() comes in between loadShadowPage() and didFinishDocumentLoad(). In
that case we'd continue on loading and then bail out in onScriptLoaderFinished.

[horo, 2015-06-16 07:21:52]

https://codereview.chromium.org/1187813010/diff/20001/Source/web/WebEmbeddedW...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/1187813010/diff/20001/Source/web/WebEmbeddedW...
Source/web/WebEmbeddedWorkerImpl.cpp:157: m_askedToTerminate = true;
On 2015/06/16 06:48:15, falken wrote:
> Maybe we also need a workerContextFailedtoStart here if m_loadingShadowPage is
> true?

No.
This destructor is called from workerContextFailedToStart() when
m_loadingShadowPage is true.

ServiceWorkerContextClient::workerContextFailedToStart()
 -> EmbeddedWorkerDispatcher::WorkerContextDestroyed()
 -> EmbeddedWorkerDispatcher::WorkerWrapper::~WorkerWrapper()
 -> WebEmbeddedWorkerImpl::~WebEmbeddedWorkerImpl()

https://codereview.chromium.org/1187813010/diff/20001/Source/web/WebEmbeddedW...
Source/web/WebEmbeddedWorkerImpl.cpp:352: ASSERT(m_workerContextClient);
On 2015/06/16 06:48:15, falken wrote:
> Should we deal with m_askedToTerminate here? As I understand it, the bug is
that
> terminate() comes in between loadShadowPage() and didFinishDocumentLoad(). In
> that case we'd continue on loading and then bail out in
onScriptLoaderFinished.

Added ASSERT().
This method will not be called after we call "m_mainFrame->setClient(0);" in the
destructor.

[falken, 2015-06-16 12:03:43]

I see, I didn't realize workerContextFailedToStart() deletes the worker.

I think this lgtm.

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

https://codereview.chromium.org/1187813010/diff/40001/Source/web/WebEmbeddedW...
Source/web/WebEmbeddedWorkerImpl.cpp:201: // This may delete 'this'.
With the new assert we're really saying "This deletes 'this'", correct? (not
maybe).

[kinuko, 2015-06-16 14:35:29]

lgtm

Would be nice to update the comment as falken suggests.

[horo, 2015-06-16 23:20:59]

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

https://codereview.chromium.org/1187813010/diff/40001/Source/web/WebEmbeddedW...
Source/web/WebEmbeddedWorkerImpl.cpp:201: // This may delete 'this'.
On 2015/06/16 12:03:43, falken wrote:
> With the new assert we're really saying "This deletes 'this'", correct? (not
> maybe).

Done.

[horo, 2015-06-16 23:21:21]

horo@chromium.org changed reviewers:
+ tkent@chromium.org

[horo, 2015-06-16 23:21:21]

tkent@
Could you please review this?

[tkent, 2015-06-16 23:27:24]

On 2015/06/16 23:21:21, horo wrote:
> But the current code doesn't
> handle this situation correctly.

What happens in such situation with the current code?  Can we write a test?

[horo, 2015-06-16 23:40:42]

On 2015/06/16 23:27:24, tkent wrote:
> On 2015/06/16 23:21:21, horo wrote:
> > But the current code doesn't
> > handle this situation correctly.
> 
> What happens in such situation with the current code?  Can we write a test?

If WebEmbeddedWorkerImpl::terminateWorkerContext() is called between
loadShadowPage() and didFinishDocumentLoad(), workerContextFailedToStart() will
not be called forever.
So the content can't know whether the termination succeeded.

I think it is possible to write a test.
I will try.

[horo, 2015-06-17 06:14:40]

On 2015/06/16 23:40:42, horo wrote:
> On 2015/06/16 23:27:24, tkent wrote:
> > On 2015/06/16 23:21:21, horo wrote:
> > > But the current code doesn't
> > > handle this situation correctly.
> > 
> > What happens in such situation with the current code?  Can we write a test?
> 
> If WebEmbeddedWorkerImpl::terminateWorkerContext() is called between
> loadShadowPage() and didFinishDocumentLoad(), workerContextFailedToStart()
will
> not be called forever.
> So the content can't know whether the termination succeeded.
> 
> I think it is possible to write a test.
> I will try.

I added unit tests.

[tkent, 2015-06-17 06:20:38]

lgtm

[horo, 2015-06-17 06:21:28]

On 2015/06/17 06:20:38, tkent wrote:
> lgtm

Thank you.

[horo, 2015-06-17 06:21:31]

The CQ bit was checked by horo@chromium.org

[horo, 2015-06-17 06:21:32]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
falken@chromium.org
Link to the patchset: https://codereview.chromium.org/1187813010/#ps80001
(title: "add tests")

[commit-bot: I haz the power, 2015-06-17 06:21:45]

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

[commit-bot: I haz the power, 2015-06-17 07:35:02]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197243