Soft Update and Register can race when setting registration scriptURL

[wanderview]

Consider the following sequence of events.

1. .register('script1.js') succeeds and resolves
2. iframe within scope is created and becomes controlled by script1.js
3. navigation triggered soft update is queued to run after document loads
4. .register('script2.js') is called which sets registration's script URL to script2.js and queues an update to run

There is now a race between:

• The navigation soft update running and setting registration scriptURL to the newest worker's script URL (step 6 of soft update)
• The register's update running

If the navigation soft update triggers prior to the register's update job getting popped from the queue then the registrations scriptURL is reset to script1.js.

Why does soft update reset the registration's script URL? It seems using the current registration script URL is probably what we want. This is the solution I am going to implement in gecko for now.

[wanderview]

Or alternatively, maybe the soft update should not run if it sees the registration script URL is different from the newest worker script URL. In that case its clear a new script is being registered and no update is needed.

[wanderview]

Yea, I'm going to implement this change. In Soft Update, instead of:

6. Set registration's registering script url to newestWorker's script url.

Instead do:

6. If newestWorker's script url does not equal registration's registering script url, abort
    these steps.

[wanderview]

Actually, the whole registering script URL setup is racy and should probably be changed.

Algorithms like Register, Soft Update, and Update all do:

Set registration's registering script url to X

Where X is either the register() script URL or the newestWorker's script URL. After running this step these algorithms all then invoke Update.

This is a problem because Update uses a queue to perform operations. So the actual work is often (usually) done asynchronously from where the script URL is set up. This means the work performed by the Update can easily operate on the wrong registering script URL.

I think the registering script URL should really be an argument to Update. The source of current truth for the registration's script URL should be the newestWorker.scriptURL.

[wanderview]

To clarify, I propose:

1. Update algorithm take an optional script URL as an argument
2. In Update, after the queue synchronization:
   1. If script URL was not provided, then set script URL to newestWorker.scriptURL
   2. Use script URL instead of registration's registering script URL

Then Register algorithm passes the content provided script URL to the Update algorithm. The update() and Soft Update algorithms do not pass a script URL to the Update algorithm, and therefore get the default "newest worker" script URL.

And in the Register algorithm, change step 4.2.3 from

If newestWorker is not null, scriptURL equals newestWorker's script url, and scriptURL equals registration's registering script url, then:

To just:

If newestWorker is not null, and scriptURL equals newestWorker's script url, then:

[wanderview]

@mattto What does chrome actually do here? I suspect the spec doesn't describe the chrome behavior accurately. It would be nice to sync to that if we can.

[mfalken]

In Chrome, register jobs triggered by register(script_url) are marked with script_url when scheduled, and update jobs triggered by Soft Update or update() are marked with the newest worker's URL when scheduled. When an update job runs, if the newest worker's URL has changed from the one the job was marked with, it aborts. When a register runs, if the newest worker's URL has NOT changed, the job aborts (usually... if the registration was uninstalling it cancels that and it can also promote waiting -> active if there's no active worker but that's an impl detail). Otherwise, it performs an update with the script URL it was marked with.

I think that's almost what you propose, except that in your proposal update job wouldn't be marked with a URL when scheduled, and would instead simply take the newest worker URL once the job actually begins.

[jungkees]

I understood the problem pointed by @wanderview. Mutating the registration's registering script url from the concurrently running main threads (Register, update(), Soft Update) shouldn't be allowed.

Marking a url for an Update from @matto's comment seems to be able to be specified as passing an argument to Update algorithm as @wanderview suggested as that's the place where the job is scheduled.

I think we still have to decide between 1) Blink's behavior: schedule and abort when newest worker's url was changed and 2) Gecko's suggestion: use newest worker's url when job begins for update jobs. @wanderview will it be okay to conform to what Blink already implemented for this? If so, the Update algorithm can take a script url as a (non-optional) argument and check if the given url is the same as the newest worker's script url before continuing with further steps.

I'll address the decision once I'm back to work.

[wanderview]

I'm happy to accept blink's behavior here. Thanks!

[wanderview]

@matto, does this match the current spec:

When a register runs, if the newest worker's URL has NOT changed, the job aborts

As far as I can tell the spec says that the check for matching URL should run synchronously when .register() is called. We also defer it until the job runs, but I have a bug open to fix that.

[mfalken]

I think you're right, the spec says it should run synchronously when .register() is called.

As a general note, differences like this are artifacts of how Chrome implemented a register/update/unregister job queue before the spec had them defined (lots of discussion was at #396 ). Chrome should probably fix these but I suspect they are minor differences.

[wanderview]

As a general note, differences like this are artifacts of how Chrome implemented a register/update/unregister job queue before the spec had them defined (lots of discussion was at #396 ). Chrome should probably fix these but I suspect they are minor differences.

To be honest, I think we should change the spec on this one. I don't like that .register() accesses shared state outside of the job queue synchronization mechanism. See issue #783.

[jungkees]

I'll take a look how I can address this requirement. Seems like some refactoring is needed in Register and Update.

[jungkees]

Merged to #783.