Bug 25215 – Support loading script for nested workers.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jian Li]

Support loading script for nested workers.

[Jian Li]

Created attachment 29509 [details]
Proposed Patch

[Alexey Proskuryakov]

Comment on attachment 29509 [details]
Proposed Patch

This means that workers will no longer use CachedResource machinery. I'm not sure how bad it is, you may need to consult with someone familiar with its design.

Why is m_loadingAborted necessary (given that we have WorkerMessagingProxy::m_askedToTerminate)? Does checking it affect visible behavior of the worker? Does this change need a test?

This patch doesn't have sufficient information in ChangeLog - it doesn't even say that it is not a full implementation of the feature, but just a first step. Ideally, each modified function should have a comment of its own.

[Jian Li]

(In reply to comment #2)
> (From update of attachment 29509 [details] [review])
> This means that workers will no longer use CachedResource machinery. I'm not
> sure how bad it is, you may need to consult with someone familiar with its
> design.
Who can I ping to consult with this design? Sending an email to webkit-dev?
> 
> Why is m_loadingAborted necessary (given that we have
> WorkerMessagingProxy::m_askedToTerminate)? Does checking it affect visible
> behavior of the worker? Does this change need a test?
I think it will be better to abort the script loading as early as possible when Worker::terminate() triggers. This should not cause any visible change to worker behavior because we will not call WorkerContextProxy::startWorkerContext when worker is asked to be terminated and then the script loading is finished. This is same as the check in WorkerMessagingProxy::startWorkerContext.

WorkerMessagingProxy::m_askedToTerminate is not exposed to WorkerContextProxy interface that can be accessed from Worker. I would prefer use a direct flag here since all the logic is in Worker scope.
> 
> This patch doesn't have sufficient information in ChangeLog - it doesn't even
> say that it is not a full implementation of the feature, but just a first step.
> Ideally, each modified function should have a comment of its own.
This is the full implementation of script loading for both workers and nested workers, though it is one of steps to support nested workers. Do you want me to say something like this in ChangeLog.

I just ran prepareChangeLog to create the change log. Do you want me to add comments to modified functions in ChangeLog like the following:
   (WebCore::Worker::didFail): Called when the loading fails.
> 

(In reply to comment #2)
> (From update of attachment 29509 [details] [review])
> This means that workers will no longer use CachedResource machinery. I'm not
> sure how bad it is, you may need to consult with someone familiar with its
> design.
> 
> Why is m_loadingAborted necessary (given that we have
> WorkerMessagingProxy::m_askedToTerminate)? Does checking it affect visible
> behavior of the worker? Does this change need a test?
> 
> This patch doesn't have sufficient information in ChangeLog - it doesn't even
> say that it is not a full implementation of the feature, but just a first step.
> Ideally, each modified function should have a comment of its own.
> 

[Alexey Proskuryakov]

> Who can I ping to consult with this design? Sending an email to webkit-dev?

Yes, an e-mail to webkit-dev sounds fine.

> This is the full implementation of script loading for both workers and nested
> workers, though it is one of steps to support nested workers.

How does it fix a FIXME in WorkerThreadableLoader::MainThreadBridge::mainThreadCreateLoader? The current implementation of ThreadableLoader doesn't work in nested workers.

> I just ran prepareChangeLog to create the change log. Do you want me to add
> comments to modified functions in ChangeLog like the following:
>    (WebCore::Worker::didFail): Called when the loading fails.

As you imply, this comment would not be helpful.

The purpose of comments is two-fold. First, they let people not directly involved with implementation of a feature to follow the progress easily, and learn new concepts. Second, they help when investigating bugs introduced by a patch - if a particular change has bad side effects, the person fixing the code needs to know its primary purpose in order to not defeat it when fixing side effects.

This patch is not trivial, and it definitely needs a detailed ChangeLog. You should mention at least the following:
- Worker loading now uses ThreadableLoader instead of CachedScript, because the latter is not thread safe. Thus, it implements a different set of callbacks;
- terminate() now has a different effect on loading;
- there may be little to say about didFail(), but a comment for didFailRedirectCheck() can explain how worker loading is supposed to handle redirects;
- a comment for abortLoading() should explain when it is supposed to be called - e.g. it's very surprising that it is called from didFail() in your patch;
- anything else that required your special attention, or that could confuse others reading your patch.

[Jian Li]

Created attachment 29588 [details]
Proposed Patch

[Jian Li]

(In reply to comment #4)
> > Who can I ping to consult with this design? Sending an email to webkit-dev?
> 
> Yes, an e-mail to webkit-dev sounds fine.

I've sent an email to webkit-dev.
> 
> > This is the full implementation of script loading for both workers and nested
> > workers, though it is one of steps to support nested workers.
> 
> How does it fix a FIXME in
> WorkerThreadableLoader::MainThreadBridge::mainThreadCreateLoader? The current
> implementation of ThreadableLoader doesn't work in nested workers.
> 
It seems that the inappropriate asserts prevent WorkerThreadableLoader from working in nested workers. I changed the asserts and update the FIXME. As pointed out in FIXME, the current implementation of WorkerThreadableLoader is not the final elegant solution, but it just works for script loading in nested workers. I think levin is working on improving it.

> > I just ran prepareChangeLog to create the change log. Do you want me to add
> > comments to modified functions in ChangeLog like the following:
> >    (WebCore::Worker::didFail): Called when the loading fails.
> 
> As you imply, this comment would not be helpful.
> 
> The purpose of comments is two-fold. First, they let people not directly
> involved with implementation of a feature to follow the progress easily, and
> learn new concepts. Second, they help when investigating bugs introduced by a
> patch - if a particular change has bad side effects, the person fixing the code
> needs to know its primary purpose in order to not defeat it when fixing side
> effects.
> 
> This patch is not trivial, and it definitely needs a detailed ChangeLog. You
> should mention at least the following:
> - Worker loading now uses ThreadableLoader instead of CachedScript, because the
> latter is not thread safe. Thus, it implements a different set of callbacks;
> - terminate() now has a different effect on loading;
> - there may be little to say about didFail(), but a comment for
> didFailRedirectCheck() can explain how worker loading is supposed to handle
> redirects;
> - a comment for abortLoading() should explain when it is supposed to be called
> - e.g. it's very surprising that it is called from didFail() in your patch;
> - anything else that required your special attention, or that could confuse
> others reading your patch.
> 
I updated the ChangeLog. Thank you very much for helping me on this.

The reason for didFail() calling abortLoading() is to cleanup the loading resource since I use abortLoading to server dual purposes: abort loading and/or cleanup resources.

[Chris Dumez]

https://commits.webkit.org/254597@main

[Radar WebKit Bug Importer]

<rdar://problem/100079583>