Shopify / dawn

Shopify's first source available reference theme, with Online Store 2.0 features and performance built-in.
Other
2.49k stars 3.35k forks source link

[Critical] Preloading can cause stores to not load at all #1995

Closed bakura10 closed 1 year ago

bakura10 commented 2 years ago

Hi,

This issue is not directly related to Dawn, but Dawn can be impacted. Unfortunately, despite reporting this issue to the support of Shopify, we have been told it is "our theme issue", which is not. We have therefore setup a reproduction case on Dawn as well, hopefully this can lead Shopify dev team to investigate this issue.

Context

In our themes, we are preloading the images of the slideshow section using the "preload" attribute in the image_tag Liquid tag. The reason is because we cannot detect which section is the first in the page, but we are making the assumption that the slideshow section is the first one on a page, so we use the preload tag there.

However, if a merchant upload very high resolution images, and that they appear to add several slideshow on the page, then the store will simply not load (it will run forever and will never even time out).

We have reproduced this on Dawn here: https://2zx0ywank983tvc1-49272160418.shopifypreview.com/

As you can see, you won't be able to load the page.

Reproduction steps

To reproduce that, we have slightly modified the Dawn slideshow code, to use the preload. Here is the code we are using:

{{- block.settings.image | image_url: width: 3000 | image_tag: loading: 'eager', sizes: '100vw', widths: '600,700,800,1000,1200,1400,1600,1800,2000,2200,2400,2600,2800,3000', preload: true -}}

Then, you need to upload extremely high resolution images and add several slideshow on the page.

Please note that this issue cannot be reproduced in the theme editor, only outside.

This makes me think that the issue lies in the Link header that is being sent that, if becoming too large, may cause the browser / Shopify to not return anything.

Considering how critical this issue is (it can really cause any theme using preload to not load at all) I would love to have the dev team inspecting this issue urgently.

Thanks :)

ludoboludo commented 2 years ago

Just had a look into this. I believe I used the right replication steps to give it a shot and things loaded for me. I couldn't replicate the issue mentioned.

You can have a look at this test store: https://breaky.myshopify.com/ and let me know if you can replicate there as well.

I changed the code for the block image to use preload: true but the website still loaded.

bakura10 commented 2 years ago

Thanks @ludoboludo , it seems to work on your test store. I have forwarded this to the person in my team who did the debugging! I will let you know :)

dsnvs commented 2 years ago

@ludoboludo Thanks for following up!

It seems like the preload was modified and now only the first two image_tag filters will add value to the link header. So instead of doing more image_tags, I am simply adding more widths attributes to these two image_tags, and if you have enough sizes, it will also break the page.

So what I decided to do was to see the exact threshold of the link header size at which the page would stop working, here is a gist where I show the headers, the character count for each header and if it worked or not: https://gist.github.com/dsnvs/fac1fe0947c7ee0915d1bee1b0967a83

I noticed that anything above 14280 characters is unstable, and above 14300 I have never been able to load a page. I am not sure why it is unstable before it becomes unusable, but it makes me think the HTTP server being used has a limit on the response header size, and as the other headers may vary depending on time or if it hit the cache or not, the response limit may be reached or not.

You can get the link header to this size by either adding more widths to the image_tag or, by changing the image's filename. For example, with images a.jpg and b.jpg, I was able to reach 14300 characters with the code below:

{{- block.settings.image | image_url: width: 3000 | image_tag: loading: 'eager', draggable: false, class: 'slideshow__image', sizes: '100vw', widths: '2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31,32,33,34,35,36,37,38,39,40,41,42,430,440,450,460,470,480,490,500,510,520,530,540,550,560,570,580,590,600,610,620,630,640,650,660,670,680,690,700,710,720,730,740,750,760,770,780,790,800,810,820,830', preload: true -}}

Bear in mind that this issue is affected by other connections you make to the shop. I don't know why, but say I am on a homepage that doesn't load because the link header is too big. If I open another tab on the same browser and visit another page (say a collection page), then the homepage will instantly load.

Additionally, after loading the page once, you need to wait at least 1 minute without visiting any page in the shop, and after this minute, the page will stop loading again.

If you are still unable to replicate, let me know and I will try to do a step-by-step tutorial.

ludoboludo commented 2 years ago

Ah very interesting. Thanks for the detailed explanation @dsnvs 👍

So the issue being that trying to preload too many images (or any other resources), will cause the server to stall or just not send anything back. Did you get any specific errors within the inspect tool when that happened ?

Preloading is meant to be used sporadically, which doesn't work super well with a section that can be added multiple times in different area of the page. I can see how it can create a bad outcome if it's not used as intended (many slideshows and some below the fold). Though the issue you noticed is still one.

I feel like it outlines the fact that we can't assume which section is going to be shown above the fold or not. We run into the same problem ourselves. Sections aren't aware of other sections so we have no way to tell in liquid the current order of section on the template and it doesn't seem like something that will be added. That prevents us from using better approaches in terms of performance for our themes 🤔

Content-visibility in CSS could be a great thing to use but it's currently only supported in Chrome and Edge.

As for sizes used in the example from the PR description, couldn't the size used in the widths be covering less use cases ? There shouldn't be much of a loss in quality if you had bigger gaps I believe 🤔 We use 9 different widths on our slideshow while you have 14 here.

I'll try and bring this up internally to talk it over as well 👍

Edit: I can replicate with your steps on my test store now. Thanks again for replication details.

siakaramalegos commented 1 year ago

Hi, I'm responding in Slack too. This is probably related to a link header issue where the headers are too long and Cloudflare can't process it. We rolled back one change that made this more likely though that won't prevent it. We're making a PR to cap the length of the link headers to prevent this from happening.

I will say it's a performance anti-pattern both to preload too much and to provide too many image options in a srcset. I'm writing an article on it, and here's my draft text as to why:

First, don’t over-do it on the srcset. When you have too many files in a srcset, you’re reducing the chance that an end-user would see the performance benefits of caching, meaning longer network delays. If an image is not yet in an edge server, the request will need to go back to the origin server to request it. Even worse, if that image size has not yet been generated from the image API, the request will be further delayed. Cache timings are not infinite so too many sizes in the srcset means a higher likelihood of longer requests. To add insult to injury, if your website gets lower traffic, you’re even less likely to have all those image options in the CDN cache.

siakaramalegos commented 1 year ago

Going to also note here but...

Because of how browsers work, you don’t need to preload any of those images. The browser has a look-ahead preparser (preload scanner but that name can be confusing). If the image is called in the HTML, which it is with liquid, then the browser will already know as soon as the HTML arrives that it needs that image. The only case where you might preload the image is when it’s a late discovered asset - like when you use JS to render on the client side.

So I would recommend NOT to preload any images in Liquid templates. Our biggest image performance problem is too many images that are lazy-loaded, not any lack of preloads. Lazy loading above the fold makes LCP slower. Since we don't ask if a section is above the fold, I would usually default to eager loading on everything that isn't a list of images.

siakaramalegos commented 1 year ago

I'd say since you're not using preload anywhere in Dawn, it's definitely not a Dawn issue. It's a storefront renderer issue. Not sure if someone created a bug ticket for it there yet, but I know @krzksz is going to work on a PR for limiting the link headers

bakura10 commented 1 year ago

@siakaramalegos thanks a lot for the answers. However your answer confuses me a bit. From what I understood, the added benefit of preload attribute on image and why it should be done is that Shopify uses the 103 Early Hints header, which allows to preload the image before the server has actually returned the response. This therefore allows the browser to start downloading LCP images before the browser can actually parse the HTML (cf https://blog.cloudflare.com/early-hints-performance/). Is that actually incorrect?

Also, I completely agree with you: setting large number of srcset is stupid, but it was to outline and reproduce the issue. Still, in our case what happen is that some merchants made their home page only of slideshow, with 4 or 5 slides per slideshow, which triggered this issue. Not saying this is a good thing to do, but this is how some merchants use things. Also, now that you have things like app blocks, an app could theorically also do bad things, and make the support believe it is a theme issue while it is not. Which is why I really wanted this to be fixed at the platform level to avoid this complex and time consuming debugging.

Regarding the specific issue and why I posted this on Dawn, is because the Shopify support has been unable to assist us and deferred the problem to our theme (which was not) instead of properly escalating the issue to the dev team as I asked them to do. This has been a constant issue for us (theme developers) to not have a direct access to the dev team whenever we find important platform issues, so my last chance is always to post the issue here to have some visibility.

siakaramalegos commented 1 year ago

@bakura10 ...

@siakaramalegos thanks a lot for the answers. However your answer confuses me a bit. From what I understood, the added benefit of preload attribute on image and why it should be done is that Shopify uses the 103 Early Hints header, which allows to preload the image before the server has actually returned the response. This therefore allows the browser to start downloading LCP images before the browser can actually parse the HTML (cf https://blog.cloudflare.com/early-hints-performance/). Is that actually incorrect?

Theoretically, yes. However there is a bug in Cloudflare at the moment so images aren't early hinted. I think they are working on that but not sure. I would also say be careful what you early hint or preload in general - often it can result in slower perf metrics. Preload has always been a bit of an unpredictable footgun, and early hints take that one step further.

Also, I completely agree with you: setting large number of srcset is stupid, but it was to outline and reproduce the issue.

Totally understand. I'm working on an image article which will also talk about this to try to teach folks to do better. Hopefully will be live by Wednesday on the new site we're building (working on the custom domain still).

Which is why I really wanted this to be fixed at the platform level to avoid this complex and time consuming debugging.

Definitely. Mateusz has a PR out so hopefully it's fixed soon. It's also a holiday in Canada today though.

Regarding the specific issue and why I posted this on Dawn, is because the Shopify support has been unable to assist us and deferred the problem to our theme (which was not) instead of properly escalating the issue to the dev team as I asked them to do. This has been a constant issue for us (theme developers) to not have a direct access to the dev team whenever we find important platform issues, so my last chance is always to post the issue here to have some visibility.

Sorry about that. I'm still new to Shopify, so I don't know how all the processes work. I think I saw you in the partners Slack. I'm in there too so feel free to tag me on anything related to web performance. I'm not a dev advocate, but I also want the platform to have good performance since I focus on merchant web performance. I can't guarantee that I will help, but I'll try to make the right connections.

krzksz commented 1 year ago

Hey everyone!

The fix for the issue was deployed few hours ago, it should be no longer possible to break the store by using too many preloads. We've decided that the most sensible solution would be to limit the size of Link header to 2KB, skipping any resources that would make it grow bigger than that. 2KB is still a lot, so it leaves plenty of space for custom preloading if needed.

The change was done within our rendering solution so no updates to themes are needed.

Thanks!

bakura10 commented 1 year ago

Thanks, this is a good workaround. On our end, based on the advice of @siakaramalegos , we have decided to remove images from preload and to keep only the main css in it :).

ludoboludo commented 1 year ago

Awesome, thanks for the follow up @krzksz 👌 Ill close the issue then 👍