Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

consider allow late importScripts/import for local URLs #1595

Open
wanderview opened this issue Jun 10, 2021 · 44 comments
Open

consider allow late importScripts/import for local URLs #1595

wanderview opened this issue Jun 10, 2021 · 44 comments

Comments

@wanderview
Copy link
Member

Could we allow late import() and importScripts() after installation if the script URL is considered "local"?

https://fetch.spec.whatwg.org/#local-scheme

So for example, you could do a late import() or importScripts() to a data: or blob: URL.

@jakearchibald
Copy link
Contributor

(taking a look at this with @mfalken)

This seems fair to us, since you can eval(str) already. What's the use-case? I guess it's taking stuff out of the cache API?

@wanderview
Copy link
Member Author

Motivation is that perhaps we could also then treat extension URLs as local since they don't hit network. That would provide a path for extension service workers to do late import/importScripts, etc. It seemed like perhaps a principled way to solve the problem.

@wanderview
Copy link
Member Author

@annevk do you have an opinion on this? Would it be fair to treat an extension URL as "local" if its guaranteed to not hit network? For example, I note that file: does not hit network, but its not considered "local". (Although maybe that's because file can in theory have a hostname?)

@annevk
Copy link
Member

annevk commented Jun 16, 2021

I'm not sure we considered file: fully, but it can definitely hit the network depending on your environment, as I understand it. Will this lead to a pattern of folks using fetch() + this or maybe they are already doing that with eval()?

For blob: and data: URLs this seems somewhat reasonable I suppose, but it does make the model a bit more complex. If that's only for extensions I'm not sure that's enough justification to also change it for the web.

@wanderview
Copy link
Member Author

One non-extension use case is loading script from IDB as a blob. Today you would have to read it all into memory and use eval, but importing from blob url would be more natural. I know of one major site storing script in IDB, although I don't think they need to import it in their service worker today.

I think the main alternative is #1585. That would support some form of import() for the web, but require similar early initialization for extensions. It also continues the unfortunate behavior of "offlining" script resources that are in fact stored locally in the extension bundle (or blob, etc).

I know we could just throw this all behind a big "if extension" conditional and do whatever we want, but it would be nice if we could find a more principled alignment with the spec. This leads to fewer surprises for developers, less maintenance burden, etc.

@asutherland
Copy link

It would be great to get the WECG to document/spec how WebExtensions upgrades relate to ServiceWorker upgrades. Is there an invariant that if a WebExtension ServiceWorker upgrades from v1 to v2 that there is a mechanism or strict ordering for the upgrade so that the v1 SW would always see the resources from the v1 WebExtension and the v2 SW would always see the resources from the v2 WebExtension? Or would implementing the local-only optimization create a problem where the SW's can effectively be out of sync with the "local" WebExtension resources?

If that's addressed, this seems like a reasonable proposal.

@tophf
Copy link

tophf commented Jun 16, 2021

how WebExtensions upgrades relate to ServiceWorker upgrades

I can describe what happens currently in Chrome/Firefox.

All files in an installed extension (from the addon gallery) are frozen to their original installed content, it will never change, so AFAICT there is no need for the upgrade event in the service worker. That said, browser makers may want to offer a simplified hot reload for the service worker of a locally developed unpacked extension, but it's not implemented yet.

When the entire extension is updated (or the reload button is pressed on a locally developed unpacked extension), the browser simply uninstalls the old extension (retaining its data), and installs the new one.

@mkruisselbrink
Copy link
Collaborator

One non-extension use case is loading script from IDB as a blob. Today you would have to read it all into memory and use eval, but importing from blob url would be more natural.

I'm not sure how this would work? We explicitly disallowed service workers from creating blob URLs, because lifetime of them would be even more confusing then blob URLs created elsewhere. For this particular use case it seems like the lifetime of them would be fine, but we'd have to re-enable blob URL creating in service workers for this to be possible.

@ghazale-hosseinabadi
Copy link

@asutherland When reloading an extension, we first Deactivate the extension, which stops the old service worker. When Deactivate is finished, we Activate the extension, which installs the new service worker. So, v1 SW would always see the resources from the v1 Extension and the v2 SW would always see the resources from the v2 Extension.

@asutherland
Copy link

Then it seems like letting local URL's be imported whenever is fine. I do wonder if it might make sense to be conservative and also define that the local URL must also be same-origin. This would mainly be to help side-step issues like extensions depending on the script resources of other extensions. (Maybe this is forbidden in practice by the underlying extension schemes, but there's no spec yet to clarify that.) This would forbid data URLs for late import as a side effect, but I'm not sure that's a bad thing, as it seems desirable to instead let import take Blobs or Responses or anything other than enabling createObjectURL or incentivizing data URLs (which can be a strain on devtools / introspection tools that don't optimize for multi-megabyte URLs).

@wanderview
Copy link
Member Author

I'm not sure I understand why we would need to require late imports to be same-origin. No other worker has that restriction. What is it about this situation that would require a same-origin restriction?

@wanderview
Copy link
Member Author

@ghazale-hosseinabadi is it possible for one extension to load URLs targeting resources in a different extension? Or are they isolated from one another?

@ghazale-hosseinabadi
Copy link

Extensions are isolated from one another and it is not possible for one extension to load URLs targeting resources in a different extension.

@asutherland
Copy link

@ghazale-hosseinabadi Is it possible for content to load URLs from extensions?

@wanderview
Copy link
Member Author

wanderview commented Jun 23, 2021

While talking with @mfalken about this issue we started wondering what service workers do for data URL imports today. It seems we have some inconsistencies:

https://sw-import-data-url.glitch.me/

Chrome seems to run importScripts() for data URLs, but firefox returns an error. Also, it seems chrome does not actually offline the data URL since it gives an error on the next sw script evaluation complaining that its a late import.

Edit: But firefox does seem to support importScripts() of data urls in dedicated workers.

@asutherland
Copy link

Thank you both for looking into that! As you are probably aware, Firefox stores ServiceWorker scripts in its Cache API implementation, and this likely makes data URIs subject to the scheme enforcement of step 4 of Cache.put.

@ghazale-hosseinabadi
Copy link

@asutherland yes, please refer to https://developer.chrome.com/docs/extensions/mv3/content_scripts/:

They can also access the URL of an extension's file with chrome.runtime.getURL() and use the result the same as other URLs.

// Code for displaying /images/myimage.png:
var imgURL = chrome.runtime.getURL("images/myimage.png");
document.getElementById("someImage").src = imgURL;

@tophf
Copy link

tophf commented Jun 23, 2021

@ghazale-hosseinabadi, @asutherland, only files exposed explicitly via web_accessible_resources are accessible for the content scripts and web pages. By default, non-exposed URLs are not accessible from outside of the extension's own origin which is chrome-extension://<id>. Note that technically chrome.runtime.getURL doesn't provide access to a URL: this method simply adds a literal string containing the extension's origin e.g. 'chrome-extension://<id>/' if that wasn't present in the parameter.

@wanderview
Copy link
Member Author

In theory local scripts would not need to be offlined in any storage, so firefox's use of cache API would not be a problem if we made an early exemption in the algorithm like "if local, do a normal importScripts()".

@jakearchibald
Copy link
Contributor

In theory local scripts would not need to be offlined in any storage, so firefox's use of cache API would not be a problem if we made an early exemption in the algorithm like "if local, do a normal importScripts()".

Agreed. If we're going to make an exception for 'local' always-available-offline-anyway URLs, then we'll bypass the map for them.

@jakearchibald
Copy link
Contributor

jakearchibald commented Jun 25, 2021

@mkruisselbrink

We explicitly disallowed service workers from creating blob URLs, because lifetime of them would be even more confusing then blob URLs created elsewhere. For this particular use case it seems like the lifetime of them would be fine, but we'd have to re-enable blob URL creating in service workers for this to be possible.

Is the lifetime of blob urls tied to the client lifetime or is it more complicated than that?

I agree it's a footgun if folks start creating blob urls in a service worker and expect to be able to use them in a page. I guess there's no easy way to create a blob URL that can only be used from the client that created it?

It's still possible to turn these things into data URLs, so it might not be a big deal.

@jakearchibald
Copy link
Contributor

jakearchibald commented Jun 25, 2021

I guess rather than encourage developers to use more blob URLs, we could somehow make importScripts(responseObject) work (but I guess the synchronous nature of importScripts creates a series of deadlocks here).

Just making data: work for now seems like the simplest choice.

@annevk
Copy link
Member

annevk commented Jun 25, 2021

Yeah, continuing to not allow blob: URLs sounds excellent and if people need them we should offer alternatives whereby the Blob instance can be used directly. (Note that w3c/FileAPI#153 will limit where blob: URLs can be used as well, including disallowing messaging one to a service worker.)

@jakearchibald
Copy link
Contributor

if people need them we should offer alternatives whereby the Blob instance can be used directly.

Ahh yeah, that works around the deadlock issue with responses.

@annevk
Copy link
Member

annevk commented Jun 25, 2021

Oh, Response is probably better to be fair and I'm not sure I see the deadlock issue. If there is one it would be there with Blob too I imagine.

@jakearchibald
Copy link
Contributor

Oh, Response is probably better to be fair and I'm not sure I see the deadlock issue.

I think the issue is if Response has a stream created in the same event loop. Since importScripts is synchronous, and streams are asynchronous, you'd get a deadlock.

@annevk
Copy link
Member

annevk commented Jun 25, 2021

I guess importScripts() is another API we don't really want to extend. 😊 Anyway, if there is a use case, we can find an alternative way of fulfilling it that does not bring along all the mistakes of the past.

@jakearchibald
Copy link
Contributor

Yeah, let's just stick with data: for now (and the same path can be used for Chrome extensions) and see how far that gets us.

@wanderview
Copy link
Member Author

I'm confused. Service workers being prevented from creating blob URLs seems orthogonal to if importScripts() can load a blob URL. For example, a window could create the blob URL and then postMessage it to the service worker.

I'm not saying there is a strong use case there, but the creation of blob URLs and loading of blob URLs are separate things.

@annevk
Copy link
Member

annevk commented Jun 25, 2021

See #1595 (comment) and w3c/FileAPI#153.

@wanderview
Copy link
Member Author

See #1595 (comment) and w3c/FileAPI#153.

Yea, I missed that. I have to say I don't understand why we would introduce some new kind of isolation instead of just making blob URLs only readable by contexts with the same StorageKey. Left a comment on that issue to that effect.

@wanderview
Copy link
Member Author

Also, one of my goals with this issue was to minimize how many special exceptions we have to our general rules. Saying "local urls work like X" seemed like a simple rule. But saying "local URLs, except blob URLs, work like X" seems less principled to me and I don't really understand the motivation.

@wanderview
Copy link
Member Author

But even if we do w3c/FileAPI#153, why would we need something in service worker importscripts to treat blob URL specially? Couldn't the importscripts algorithm just operate on all local url equally and let blob url loading algorithm handle whether to fail or not?

@wanderview
Copy link
Member Author

Do we have consensus on this? To use the normal non-SW importScripts() algorithm for local URLs? If blob URLs become blocked based on agent cluster, then they would fail to load.

@jakearchibald
Copy link
Contributor

Yeah, ok, my objection to blob URLs isn't particularly strong. A page and a service worker would be in a different agent cluster, yeah?

@wanderview
Copy link
Member Author

A page and a service worker would be in a different agent cluster, yeah?

Yes. A service worker is always in a separate agent cluster I believe. The only way that would change is if we allowed nested dedicated workers in a service worker, which we want, but have not done yet.

@jakearchibald
Copy link
Contributor

Ok, that sounds good to me. I'm happy with your proposal.

@asutherland
Copy link

asutherland commented Jul 6, 2021

I'm fine with this. I think this does introduce somewhat of an edge case in terms of potential breakage via changes to web_accessible_resources (which are local but not same-origin as I understand it) mentioned in #1595 (comment). However, I think for Firefox our plan will be to unregister (or update) any ServiceWorker registration touched by a web extension in any way when the extension is upgraded or uninstalled, which side-steps the issue.

I don't think anyone from Safari/WebKit has weighed in?

@jakearchibald
Copy link
Contributor

jakearchibald commented Jul 6, 2021

@youennf any thoughts on this? The proposal is to allow importScripts(url) at any time if url is a local URL (data:, blob: etc).

@youennf
Copy link

youennf commented Jul 6, 2021

Seems ok to me, though I am not a big fan of blob URLs in general.
@bradeeoh might have some thoughts, since the use case seems related to web extensions.

@annevk
Copy link
Member

annevk commented Jul 20, 2021

My worry is allowing blob URLs to work temporarily and then later not being able to prevent them from working.

@wanderview
Copy link
Member Author

My worry is allowing blob URLs to work temporarily and then later not being able to prevent them from working.

Don't you already have that problem with SharedWorker? A SharedWorker can be in a different agent cluster and I believe blob URL works for importScripts() there already.

It seems to me the best approach for your concern would be to get to consensus in w3c/FileAPI#153 whether to attempt agent cluster isolation or not. And if you want to attempt it, then you can get consensus there to not implement blob URL loading in service workers for now. But that seems orthogonal to whether to support late loading of local URLs in service workers.

But if you can at least get consensus to try then implementations could have a check for blob's in the service worker code with a TODO comment saying that this should be converted to an agent cluster check in the blob loader in the future.

I think in the end, though, this kind of restriction should be part of the blob spec/impl and not buried in some exceptional if-statement in the middle of the service worker spec/impl.

@annevk
Copy link
Member

annevk commented Jul 20, 2021

Well, but URL.createObjectURL() is exposed in shared workers, so I think that is a different situation.

I do agree on what this should look like long term and if you want to block resolving this on w3c/FileAPI#153 that would work for me as well.

@wanderview
Copy link
Member Author

I don't think we need to block on w3c/FileAPI#153. It looks like @arichiv is already investigating agent cluster isolation in chrome. So I think an implementation check and TODO comment pointing to that investigation would be reasonable to prevent volume increase for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants