Invoke WorkerScriptLoader's m_finishedCallback callback safely.

Invoke WorkerScriptLoader's m_finishedCallback callback safely.

WorkerScriptLoader instance could be deleted in the midst of running a
member method. Here is a bad scenario that could cause a crash.

1. didFail() client method would be called in the midst of processing
ThreadableLoader::create(), e.g. for forbidden cross origin requests.
2. didFail() invokes m_finishedCallback via notifyFinished().
3. m_finishedCallback could delete WorkerScriptLoader instance.
4. members could be touched after the destruction.

This patch changes the class to postpone the callback invocation until
ThreadableLoader::create() returns, and to do nothing after invoking the
callback.

Also this patch stop using |m_finishing| that's vague.
The flag is used only to check if the callback needs invocation.

BUG=504685
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198200

[Takashi Toyoshima, 2015-06-30 09:45:59]

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

[Takashi Toyoshima, 2015-06-30 10:27:01]

Can you take a look?

[kinuko, 2015-07-01 08:11:16]

https://codereview.chromium.org/1213443006/diff/1/Source/core/workers/WorkerS...
File Source/core/workers/WorkerScriptLoader.cpp (right):

https://codereview.chromium.org/1213443006/diff/1/Source/core/workers/WorkerS...
Source/core/workers/WorkerScriptLoader.cpp:203: // notifyError() would be called
before ThreadableLoader::create() returns
nit: would be -> could be ?

https://codereview.chromium.org/1213443006/diff/1/Source/core/workers/WorkerS...
Source/core/workers/WorkerScriptLoader.cpp:204: // e.g., from didFail(). Since
m_finishedCallback potentially delete this
could we also clearly note that null m_threadableLoader means that this is
called before ThreadableLoader::create() returns?

e.g.

"notifyError() could be called before ThreadableLoader::create() returns, e.g.
from didFail(), and in that case m_threadableLoader is not yet set (i.e. still
null)."

Also doing these checks in notifyError() might be clearer (so that we only call
notifyFinished once in the case too)?

https://codereview.chromium.org/1213443006/diff/1/Source/core/workers/WorkerS...
File Source/core/workers/WorkerScriptLoader.h (right):

https://codereview.chromium.org/1213443006/diff/1/Source/core/workers/WorkerS...
Source/core/workers/WorkerScriptLoader.h:60: // Note that callbacks would be
invoked before loadAsynchronously() returns.
would -> could ?

https://codereview.chromium.org/1213443006/diff/1/Source/web/WebEmbeddedWorke...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/1213443006/diff/1/Source/web/WebEmbeddedWorke...
Source/web/WebEmbeddedWorkerImpl.cpp:309: // and |this| was deleted at this
point.
nit:

would be -> might be
was deleted -> might have been deleted ?

https://codereview.chromium.org/1213443006/diff/1/Source/web/WebSharedWorkerI...
File Source/web/WebSharedWorkerImpl.cpp (right):

https://codereview.chromium.org/1213443006/diff/1/Source/web/WebSharedWorkerI...
Source/web/WebSharedWorkerImpl.cpp:204: // and |this| was deleted at this point.
ditto

[Takashi Toyoshima, 2015-07-02 05:55:04]

Thanks, PTAL.

https://codereview.chromium.org/1213443006/diff/1/Source/core/workers/WorkerS...
File Source/core/workers/WorkerScriptLoader.cpp (right):

https://codereview.chromium.org/1213443006/diff/1/Source/core/workers/WorkerS...
Source/core/workers/WorkerScriptLoader.cpp:203: // notifyError() would be called
before ThreadableLoader::create() returns
On 2015/07/01 08:11:15, kinuko wrote:
> nit: would be -> could be ?

Done.

https://codereview.chromium.org/1213443006/diff/1/Source/core/workers/WorkerS...
Source/core/workers/WorkerScriptLoader.cpp:204: // e.g., from didFail(). Since
m_finishedCallback potentially delete this
On 2015/07/01 08:11:15, kinuko wrote:
> could we also clearly note that null m_threadableLoader means that this is
> called before ThreadableLoader::create() returns?
> 
> e.g.
> 
> "notifyError() could be called before ThreadableLoader::create() returns, e.g.
> from didFail(), and in that case m_threadableLoader is not yet set (i.e. still
> null)."
> 
> Also doing these checks in notifyError() might be clearer (so that we only
call
> notifyFinished once in the case too)?

Done.

https://codereview.chromium.org/1213443006/diff/1/Source/core/workers/WorkerS...
File Source/core/workers/WorkerScriptLoader.h (right):

https://codereview.chromium.org/1213443006/diff/1/Source/core/workers/WorkerS...
Source/core/workers/WorkerScriptLoader.h:60: // Note that callbacks would be
invoked before loadAsynchronously() returns.
On 2015/07/01 08:11:15, kinuko wrote:
> would -> could ?

Done.

https://codereview.chromium.org/1213443006/diff/1/Source/web/WebEmbeddedWorke...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/1213443006/diff/1/Source/web/WebEmbeddedWorke...
Source/web/WebEmbeddedWorkerImpl.cpp:309: // and |this| was deleted at this
point.
On 2015/07/01 08:11:16, kinuko wrote:
> nit:
> 
> would be -> might be
> was deleted -> might have been deleted ?

Done.

https://codereview.chromium.org/1213443006/diff/1/Source/web/WebSharedWorkerI...
File Source/web/WebSharedWorkerImpl.cpp (right):

https://codereview.chromium.org/1213443006/diff/1/Source/web/WebSharedWorkerI...
Source/web/WebSharedWorkerImpl.cpp:204: // and |this| was deleted at this point.
On 2015/07/01 08:11:16, kinuko wrote:
> ditto

Done.

[Takashi Toyoshima, 2015-07-02 05:56:15]

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

[Takashi Toyoshima, 2015-07-02 05:58:26]

+haraken@ for web/ OWNERS review.

[haraken, 2015-07-02 06:18:06]

https://codereview.chromium.org/1213443006/diff/40001/Source/core/workers/Wor...
File Source/core/workers/WorkerScriptLoader.cpp (right):

https://codereview.chromium.org/1213443006/diff/40001/Source/core/workers/Wor...
Source/core/workers/WorkerScriptLoader.cpp:186: // Since the callback invocation
in notifyFinished() potentially delete

Another way to avoid this would be to make WorkerScriptLoader RefCounted and
protect |this|. I'm not sure which is better -- deferring the decision to
kinuko-san.

https://codereview.chromium.org/1213443006/diff/40001/Source/core/workers/Wor...
Source/core/workers/WorkerScriptLoader.cpp:204: void
WorkerScriptLoader::notifyFinished()

If this method is invoked only in a case where some failure occurs, we might
want to rename the method to notifyFailed or something like that.

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

https://codereview.chromium.org/1213443006/diff/40001/Source/web/WebEmbeddedW...
Source/web/WebEmbeddedWorkerImpl.cpp:308: // Do nothing here since
onScriptLoaderFinished() might be already invoked

might be => might have been

https://codereview.chromium.org/1213443006/diff/40001/Source/web/WebSharedWor...
File Source/web/WebSharedWorkerImpl.cpp (right):

https://codereview.chromium.org/1213443006/diff/40001/Source/web/WebSharedWor...
Source/web/WebSharedWorkerImpl.cpp:203: // Do nothing here since
onScriptLoaderFinished() might be already invoked

might be => might have been

[Takashi Toyoshima, 2015-07-02 07:33:56]

PTAL PS4

https://codereview.chromium.org/1213443006/diff/40001/Source/core/workers/Wor...
File Source/core/workers/WorkerScriptLoader.cpp (right):

https://codereview.chromium.org/1213443006/diff/40001/Source/core/workers/Wor...
Source/core/workers/WorkerScriptLoader.cpp:186: // Since the callback invocation
in notifyFinished() potentially delete
Yep, originally this class was RefCounted, and I changed it in
https://codereview.chromium.org/1190133002. That's why this regression happens
:(

I prefer to stop using RefCounted as possible since using RefCounted makes
object lifecycle management vague. But I'll follow reviewer's final decision.

https://codereview.chromium.org/1213443006/diff/40001/Source/core/workers/Wor...
Source/core/workers/WorkerScriptLoader.cpp:204: void
WorkerScriptLoader::notifyFinished()
Good point.
Maybe we should use notifyFinished() even in didFinishLoading() to invoke
m_finishedCallback and make this method to be consistent with the method name.
That would also contribute to remove redundant code.
Also, having notifyError() as a public method looks curious. I'd make some
changes around these points in the next patch set.

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

https://codereview.chromium.org/1213443006/diff/40001/Source/web/WebEmbeddedW...
Source/web/WebEmbeddedWorkerImpl.cpp:308: // Do nothing here since
onScriptLoaderFinished() might be already invoked
Oops, tense is so important here.

https://codereview.chromium.org/1213443006/diff/40001/Source/web/WebSharedWor...
File Source/web/WebSharedWorkerImpl.cpp (right):

https://codereview.chromium.org/1213443006/diff/40001/Source/web/WebSharedWor...
Source/web/WebSharedWorkerImpl.cpp:203: // Do nothing here since
onScriptLoaderFinished() might be already invoked
On 2015/07/02 06:18:06, haraken wrote:
> 
> might be => might have been

Done.

[kinuko, 2015-07-02 08:04:54]

lgtm

As was mentioned in the comments notifyError() and notifyFinished() look
somewhat duplicated and a bit confusing, would be nice if we could clean them up
too in a follow-up patch.

[haraken, 2015-07-02 08:06:50]

LGTM

[Takashi Toyoshima, 2015-07-02 08:21:03]

The CQ bit was checked by toyoshim@chromium.org

[commit-bot: I haz the power, 2015-07-02 08:21:10]

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

[commit-bot: I haz the power, 2015-07-02 09:32:56]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198200