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

fix(audit): iframe with redirect shouldn't be marked as critical #6704

Merged
merged 6 commits into from
Apr 17, 2019

Conversation

wardpeet
Copy link
Collaborator

@wardpeet wardpeet commented Dec 2, 2018

Summary

Fixes 302 iframes inside a page marked as critical requests

Related Issues/PRs
#6675

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having a hard time evaluating this PR... This comment describes this original impl, which has changed a lot.

And unfortunately our test page doesn't have an iframe behind a redirect any more. I created a test page (in #7352) but it never gets an entry in the preload audit. (only preconnect)

Going back to the original bug report #6675, the concern is that we suggest to preload an iframe the user is deliberately lazyloading. We could see that an iframe's request was script-initiated and skip those if we want, but I don't think that's neccessary.

At this point I don't know what redirects have to do with this, but maybe there's more context I'm missing.

@kirill-chirkov-at-clouty
Copy link

kirill-chirkov-at-clouty commented Mar 1, 2019

@paulirish I might guess that 302 status on our current iframe (which described in #6675) is pure coincidental, point of issue was that iframe was loaded not with the document but later, JS-initiated, thus not blocking initial parsing and render. Fixing 302 as non-critical request would help in our sole case, but I might guess that won't help where programmaticaly enabled iframe would not give 302 but 200.

@wardpeet
Copy link
Collaborator Author

i'll check it out if it's still a problem ,if not we can just close this one.

@paulirish
Copy link
Member

#7352 fixed initiators and redirects. It makes a lot more sense now.

We specifically say that iframes aren't critical.

And we only want critical requests for the user to preload. So if we ever flag a redirect (that eventually is an iframe) in those audits, we have something to fix.

I couldn't get a redirect to show up in preload details. But maybe i wasn't constructing the right situation.

If they'll never show up in preload, then we're done here. :)

@wardpeet
Copy link
Collaborator Author

@paulirish it's not showing up anymore in preload so that's good but the redirect is still marked as a critical request because the resourceType is marked as undefined for a redirect.

I made a repro on glitch
https://erratic-lion.glitch.me

I'm rebasing this branch

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh sorry ward forgot to flush this like weeks ago

lighthouse-core/computed/critical-request-chains.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

lighthouse-core/computed/critical-request-chains.js Outdated Show resolved Hide resolved
@wardpeet
Copy link
Collaborator Author

Fixed comments 😄

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested this out. lgtm.

redirects to iframes (noncritical) are no longer included as critical. ✅
redirects to scripts (critical) are included as critical requests. ✅

Here's a test run where i added a redirect to a script. you can see it enters into the chain as expected. (this already worked fine in master and continues to work):
image

And test with redirects before an iframe (master on left, this PR on right):
image

let's do this.

@wardpeet
Copy link
Collaborator Author

thanks @paulirish !

@paulirish paulirish merged commit 4b1a28f into master Apr 17, 2019
@paulirish paulirish deleted the fix/critical-undefined branch April 17, 2019 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants