1764598 - Refactor the Worker ScriptLoader

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, task, P3)

Description

[Yulia Startsev [:yulia]]

TL;DR -- hopefully this will bring us close to "if you know the DOM ScriptLoader, then you know the WorkerScriptLoader" and vice versa. Eventually, we might be able to factor out the common behavior into a base ScriptLoaderClass (ScriptLoaderInterface is the start of this.)

The Worker Scriptloader differs from the DOM Scriptloader in that it splits the tasks into two objects: The ScriptLoaderRunnable and the ScriptExecutorRunnable. It is slightly confusing how these two objects are being used, as the main thread runnable, ScriptLoaderRunnable, is being passed to ScriptExecutorRunnable. When it comes to module loading, In the DOM we would switch between these two threads as we traverse layers of the graph (Compiling the script: https://searchfox.org/mozilla-central/rev/86c98c486f03b598d0f80356b69163fd400ec8aa/dom/script/ScriptLoader.cpp#3327-3347 followed by the next layer of fetches: https://searchfox.org/mozilla-central/rev/86c98c486f03b598d0f80356b69163fd400ec8aa/js/loader/ModuleLoaderBase.cpp#451-453).

It is possible to implement the module loader with the existing structure but the module loader would be attached to the ScriptLoaderRunnable as its ScriptLoaderInterface because it carries the information we need, but we would also need to implement that same interface on ScriptExecutorRunnable, as this handles evaluation. It is a little weird to have two runnable, which is used for executing code, hanging around for this purpose. Instead, we can have a class that generates these runnables, and these runnables have the role of executing the necessary steps on a given thread.

I also noticed a few other spots for simplification, namely around how we do loading from cache or from the network. The goal here is to have a similar shape to the DOM Script loader.

Comment 1

[Yulia Startsev [:yulia]]

Attachment: Bug 1764598 - WSL Part 1: Separate Runnable from WorkerScriptLoader; r=asuth

We are gradually moving closer towards a ScriptLoader implementation based on the DOM ScriptLoader.
For now, the class ScriptLoaderRunnable was being passed between runnables to access information
that was shared. That role will now be done by the WorkerScriptLoader which will eventually inherit
from ScriptLoaderInterface. This is the basis for building up the Module Loader implementation for
workers.

In the next few patches, methods from the ScriptExecutorRunnable will also be moved into this class,
and the runnables themselves will no longer hold execution information. Instead, they will be used
as dispatchers to have the work run in the correct place. The goal of this patch queue is to align
with the DOM Script Loader so that future refactorings are easier.

Depends on D144277

Comment 2

[Yulia Startsev [:yulia]]

Attachment: Bug 1764598 - WSL Part 3: Move ScriptExecutorRunnable methods to WorkerScriptLoader; r=asuth

Depends on D144326

Comment 3

[Yulia Startsev [:yulia]]

Attachment: Bug 1764598 - Cleanup: Migrate GetBaseURI function to WorkerScriptLoader method; r=asuth

Depends on D144327

Comment 4

[Yulia Startsev [:yulia]]

Attachment: Bug 1764598 - LoadHandler Part 1: Move onStreamComplete and onStartRequest methods from WorkerScriptLoader to LoaderListener; r=asuth

The LoaderListener is currently used to forward events to WorkerScriptLoader. It fills a similar
role to the ScriptLoadHandler on the DOM both for OnStartRequest and OnStreamComplete. In the case
of the DOM loader, the responsibility for performing certain channel related tasks is left to the
ScriptLoadHandler, in particular the decoding of the string into UTF8 or UTF16.

While the decoding is embedded in the onStreamCompleteInternal method, it is not a 1:1 mapping.
Some of the responsibilities handled in the OnStreamCompleteInternal method are instead handled in
ScriptLoader::PrepareLoadedRequest, such as populating the SourceMaps field, managing the channel,
etc are normally handled within the ScriptLoader.

However, in the case of the WorkerScriptLoader we have two paths to reach a completed load:

• from the network
• from the cache

These have different requirements in terms of preparing the loaded request, for example the cache
does not forward a SourceMap.

In the interest of not complicating things too much, for now the method is moved wholesale into
LoaderListener. In a separate PR I will handle renaming. The next step will be to do a similar move
for the CacheScriptLoader.

Depends on D144835

Comment 5

[Yulia Startsev [:yulia]]

Attachment: Bug 1764598 - LoadHandler Part 2: Rename LoaderListener {OnStartRequest, OnStreamComplete}Internal methods; r=asuth

Depends on D144836

Comment 6

[Yulia Startsev [:yulia]]

Attachment: Bug 1764598 - LoadHandler part 3: Move {DataReceivedFromCache,DataReceived} from WorkerScriptLoader to CacheScriptLoader; r=asuth

CacheScriptLoader

Similar to what was done with the loader listener, this can be described as the companion method to
loading from the network, with special requirements in terms of how it populates the loaded request
data. At some point, it may make sense to have the service worker loader as a child class of the
main worker loader, but that won't be done here (I hope anyway).

Depends on D144837

Comment 7

[Yulia Startsev [:yulia]]

Attachment: WIP: Bug 1764598 - Split ScriptLoadInfo into it's own file

Depends on D144838

Comment 8

[Yulia Startsev [:yulia]]

Attachment: WIP: Bug 1764598 - Move WorkerScriptLoader class definition to header file

Depends on D144839

Comment 9

[Yulia Startsev [:yulia]]

Attachment: WIP: Bug 1764598 - Split ScriptResponseHeaderProcessor into own file

Depends on D144840

Comment 10

[Yulia Startsev [:yulia]]

Attachment: WIP: Bug 1764598 - Split CacheScriptLoader & helpers into own file

Depends on D144841

Comment 11

[Yulia Startsev [:yulia]]

Attachment: WIP: Bug 1764598 - Split out LoadListener into it's own file

Depends on D144842

Comment 12

[Yulia Startsev [:yulia]]

Attachment: WIP: Bug 1764598 - Use IsDebuggerScript instead of accessing field

Depends on D144843

Comment 13

[Yulia Startsev [:yulia]]

Attachment: WIP: Bug 1764598 - Use IsMainScript instead of accessing field

Depends on D144859

Comment 14

[Yulia Startsev [:yulia]]

Attachment: WIP: Bug 1764598 - Use {Set,Get}Controller instead of accessing field

Depends on D144860

Comment 15

[Yulia Startsev [:yulia]]

Attachment: WIP: Bug 1764598 - Use GetCacheCreator and IsCancelled instead of accessing fields

Depends on D144861

Comment 16

[Yulia Startsev [:yulia]]

Attachment: WIP: Bug 1764598 - Rename RunInternal to LoadScripts

Depends on D144862

Comment 17

[Yulia Startsev [:yulia]]

Attachment: WIP: Bug 1764598 - Reduce friend classes of WorkerScriptLoader

Depends on D144863

Comment 18

[Yulia Startsev [:yulia]]

Attachment: WIP: Bug 1764598 - Rename RunInternal to LoadScripts

Depends on D144327

Comment 19

[Yulia Startsev [:yulia]]

Attachment: Bug 1764598 - Cleanup: Use IsDebuggerScript instead of accessing field; r=asuth

Depends on D144959

Comment 20

[Yulia Startsev [:yulia]]

Attachment: Bug 1764598 - WSL Part 4: Move contents of Prerun and WorkerRun to named functions on WorkerScriptLoader; r=asuth

Depends on D144960

Comment 21

[Yulia Startsev [:yulia]]

Attachment: WIP: Bug 1764598 - Create a DispatchLoadScripts method

This encompasses the dispatching of the loader runnable as part of the api of WorkerScriptLoader.
The same pattern will be used for the ScriptExecutorRunnable. We might be able to just use an NS
runnable method instead here.

Depends on D144326

Comment 22

[Yulia Startsev [:yulia]]

Attachment: Bug 1764598 - WSL Part 5: Rename ExecuteFinishedScripts to "DispatchProcessPendingRequests"; r=asuth

This is mostly to bring us in line with the naming of everything else, and also make it clear that
we are crossing a thread boundary.

Depends on D144961

Comment 23

[Yulia Startsev [:yulia]]

Attachment: Bug 1764598 - WSL Part 2: Create a DispatchLoadScripts method; r=asuth

This encompasses the dispatching of the loader runnable as part of the api of WorkerScriptLoader.
The same pattern will be used for the ScriptExecutorRunnable. We might be able to just use an NS
runnable method instead here.

Depends on D144326

Comment 24

[Yulia Startsev [:yulia]]

Attachment: Bug 1764598 - LoadHandler part 4: Rename CacheScriptLoader and LoaderListener to {Cache,Network}LoadHandler; r=asuth

This makes the relationship between these classes and the dom ScriptLoadHandler clearer. Once we
move to ScriptLoadRequests, these three classes will share a loadhandler class which will take care
of script decoding.

Depends on D144838

Comment 25

[Yulia Startsev [:yulia]]

Attachment: Bug 1764598 - WSL Part 6: Rename WorkerScriptLoader Evaluation methods; r=asuth

This just brings us a bit closer to the DOM pattern of Script Loading. One difference is that we
evaluate a source buffer instead of a JSScript, so rather than "Execute", we will still ahandle data
using "Evaluate". Maybe later we can make these two methods on the SpiderMonkey side a bit more
descriptive. For now this gets us pretty close to the goal of "if you know the DOM script loader, you know the
Worker Script loader" and vice-versa.

Depends on D144990

Comment 26

[Phabricator Automation]

Comment on attachment 9274072 [details]
WIP: Bug 1764598 - Split ScriptLoadInfo into it's own file

Revision D144839 was moved to bug 1766995. Setting attachment 9274072 [details] to obsolete.

Comment 27

[Phabricator Automation]

Comment on attachment 9274074 [details]
WIP: Bug 1764598 - Split ScriptResponseHeaderProcessor into own file

Revision D144841 was moved to bug 1766995. Setting attachment 9274074 [details] to obsolete.

Comment 28

[Phabricator Automation]

Comment on attachment 9274075 [details]
WIP: Bug 1764598 - Split CacheScriptLoader & helpers into own file

Revision D144842 was moved to bug 1766995. Setting attachment 9274075 [details] to obsolete.

Comment 29

[Phabricator Automation]

Comment on attachment 9274076 [details]
WIP: Bug 1764598 - Split out LoadListener into it's own file

Revision D144843 was moved to bug 1766995. Setting attachment 9274076 [details] to obsolete.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c6faca0eb13d
WSL Part 1: Separate Runnable from WorkerScriptLoader; r=asuth
https://hg.mozilla.org/integration/autoland/rev/eae0eba22fee
WSL Part 2: Create a DispatchLoadScripts method; r=asuth
https://hg.mozilla.org/integration/autoland/rev/01a550dc0115
WSL Part 3: Move ScriptExecutorRunnable methods to WorkerScriptLoader; r=asuth
https://hg.mozilla.org/integration/autoland/rev/9c4852865679
WSL Part 4: Move contents of Prerun and WorkerRun to named functions on WorkerScriptLoader; r=asuth
https://hg.mozilla.org/integration/autoland/rev/5a6425cc2eea
WSL Part 5: Rename ExecuteFinishedScripts to "DispatchProcessPendingRequests"; r=asuth
https://hg.mozilla.org/integration/autoland/rev/5fe4460f1180
WSL Part 6: Rename WorkerScriptLoader Evaluation methods; r=asuth
https://hg.mozilla.org/integration/autoland/rev/8c1767e59650
LoadHandler Part 1: Move onStreamComplete and onStartRequest methods from WorkerScriptLoader to LoaderListener; r=asuth
https://hg.mozilla.org/integration/autoland/rev/716bf260e521
LoadHandler Part 2: Rename LoaderListener {OnStartRequest, OnStreamComplete}Internal methods; r=asuth
https://hg.mozilla.org/integration/autoland/rev/040260a2333a
LoadHandler part 3: Move {DataReceivedFromCache,DataReceived} from WorkerScriptLoader to CacheScriptLoader; r=asuth
https://hg.mozilla.org/integration/autoland/rev/9a685a347995
LoadHandler part 4: Rename CacheScriptLoader and LoaderListener to {Cache,Network}LoadHandler; r=asuth
https://hg.mozilla.org/integration/autoland/rev/22c92519a7b3
Cleanup: Migrate GetBaseURI function to WorkerScriptLoader method; r=asuth
https://hg.mozilla.org/integration/autoland/rev/eafbb8ec101f
Cleanup: Use IsDebuggerScript instead of accessing field; r=asuth

Comment 31

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/c6faca0eb13d
https://hg.mozilla.org/mozilla-central/rev/eae0eba22fee
https://hg.mozilla.org/mozilla-central/rev/01a550dc0115
https://hg.mozilla.org/mozilla-central/rev/9c4852865679
https://hg.mozilla.org/mozilla-central/rev/5a6425cc2eea
https://hg.mozilla.org/mozilla-central/rev/5fe4460f1180
https://hg.mozilla.org/mozilla-central/rev/8c1767e59650
https://hg.mozilla.org/mozilla-central/rev/716bf260e521
https://hg.mozilla.org/mozilla-central/rev/040260a2333a
https://hg.mozilla.org/mozilla-central/rev/9a685a347995
https://hg.mozilla.org/mozilla-central/rev/22c92519a7b3
https://hg.mozilla.org/mozilla-central/rev/eafbb8ec101f