The new workbox service worker implementation uses UrlHelper.absolute. Since this is a compiled asset, it stores the full URL, containing the full hostname of the primary host in a multisite environment.
I think it should be using UrlHelper.local_cdn_url instead. This also circumvents the need to recompile assets after changing the hostname.
I think the general consensus is that Service Workers shouldn’t ever be served from a CDN:
Oh so you mean the importScripts files? Do you have a multi-site cluster where every site has a different CDN URL ?
Yes, importScripts and modulePathPrefix, on lines 3 and 6 of that ERB file.
But you are not understanding me completely. The issues are happening where there is no CDN on a multisite cluster.
Both UrlHelper.absolute and UrlHelper.local_cdn_url can handle situations with and without CDN.
absolute:
no cdn: https://primarysite.ofmultisitecluster.com/javascripts/workbox/workbox-sw.js **bad - remote origin for all sites but the primary, exposing primary cluster hostname **
cdn: //cdnurl/javascripts/workbox/workbox-sw.js
local_cdn_url:
no cdn:/javascripts/workbox/workbox-sw.js good - relative url
cdn: //cdnurl/javascripts/workbox/workbox-sw.js
so the latter is what we want.
The problem is that those lines (3 ad 6) are not about the service worker file.
The service worker comes from the base domain (as it can’t be served from a CDN) but this service worker lazily imports scripts at runtime (in this case the workbox library files) and those can be served from a CDN.
So the problem is that your multisite cluster doesn’t have a CDN configured so that is what exposes this bug, which is masked when DISCOURSE_CDN is set. Just wanted to know why it doesn’t affect us.
You are correct, I’ve edited my second post to correct my mistakes.
It’s not about the service worker file, it’s IN the service worker file.
Yes, that is exactly when the bug is being exposed.
Just checking up - is my diagnosis correct and this this a bug that is going to be fixed? Is there anything you need from us that would help?
Yes, it looks a bug that needs fixing. I will get to it this week!
Hmmm I just tried it out on a Meta console:
## Current
[1] pry(main)> UrlHelper.absolute("/javascripts/workbox/workbox-sw.js")
=> "https://d3bpeqsaub0i6y.cloudfront.net/javascripts/workbox/workbox-sw.js"
### Proposed change
[2] pry(main)> UrlHelper.local_cdn_url("/javascripts/workbox/workbox-sw.js")
=> "/javascripts/workbox/workbox-sw.js"
Those doesn’t look interchangeable functions to me.
You are correct, local_cdn_url replaces a local URL with a CDN URL. And this is not even a local URL but a relative one.
So I think these would suffice instead of those UrlHelper calls?
importScripts("<%= (Discourse.asset_host || '') + "/javascripts/workbox/workbox-sw.js" %>");
and
modulePathPrefix: (Discourse.asset_host || '') + "/javascripts/workbox",
Reading the current implementation of UrlHelper.absolute:
https://github.com/discourse/discourse/blob/master/lib/url_helper.rb#L28-L31
Looks like it will compose the URL by concatenating Discourse.base_url_no_prefix plus the parameter when CDN is nil, which is your case.
So the problem is that Discourse.base_url_no_prefix is returning always the first host in the multisite environment?
Looking into the code 
https://github.com/discourse/discourse/blob/master/lib/discourse.rb#L286-L293
the name of the variable here current_hostname @ 288 strongly suggests something multisite aware 
and by
https://github.com/discourse/discourse/blob/master/lib/discourse.rb#L273-L276
it looks like it is. Dead end so far…
Looking elsewhere, this route gained some special sauce because browsers love to hammer it HARD, and we aren’t allowed to put it on a CDN and make it someone else problem. While doing this, we had a bug involving a multisite leak, which was fixed by @sam one year ago:
https://github.com/discourse/discourse/commit/abf0b1c5bd8afdfdc53a035eccac3485de659bfa
Is there a possibility that the way you are serving this multisite cluster is caching this route in a leaky way, like we were in early 2018?
No, the problem is that it does that when precompiling assets, and that becomes an issue on multisite.
So the solution is to not include the hostname in the assets ever (except for an asset CDN if it happens to be set since that is always shared between the multisite hosts anyway)
@falco did you see my proposed solution two posts above this one?
Yes, but in my tests it doesn’t cover subfolders without a CDN 
I think we will need to use:
"#{Discourse.asset_host}#{Discourse.base_prefix}/javascripts/workbox"
Hmm good point about the subfolder.
But…Discourse.asset_host can be nil and I’ve never heard of Discourse.base_prefix ?
How about this:
importScripts("<%= (Discourse.asset_host || GlobalSetting.relative_url_root) + "/javascripts/workbox/workbox-sw.js" %>");
modulePathPrefix: "<%= (Discourse.asset_host || GlobalSetting.relative_url_root) + "/javascripts/workbox" %>",
That is exactly what we want in this case:
irb(main):001:0> puts "a#{nil}bc"
abc
Oh I meant Discourse.base_path.
Just commited a fix, please check it out.
Looks good for our case… thanks!
But… just to make sure… I’m not sure how you guys are handling assets on a CDN in combination with a subfolder, but if you use Discourse.asset_host, do you still prefix all paths on the asset host with the subfolder path as well? Because that is what the code is doing now.
If that is what you’re doing then you can completely ignore this paragraph 
This whole subfolder + CDN thing caused use plenty of problems indeed. @featheredtoast did some work streamlining this and we spent a lot of time ensuring all our asset serving code can properly work on those weird combinations of buckets, subfolders, etc.
I think we are safe 
, but if needed we can reopen this.
Thanks for the bug report!
This topic was automatically closed after 7 days. New replies are no longer allowed.