Append Upgrade-Insecure-Requests header in CachedResourceLoader, following the fetch spec [1, step 3]. [1] https://fetch.spec.whatwg.org/#concept-main-fetch
Created attachment 394730 [details] Patch
Note that addHTTPUpgradeInsecureRequestsIfNeeded could also be a free function or go into FetchIdioms.h, I just feel it does not seem to be part of FrameLoader, for one thing it is static so it does not seem to have a strong connection.
Is there any observable behavior change with this change? What motivated this change?
(In reply to Alex Christensen from comment #3) > Is there any observable behavior change with this change? What motivated > this change? There should be no observable behavior change. The motivation is to stick closer to the Fetch specification and setup requests in a more central place, as well as using more Fetch data structures, for instance here checking Fetch mode rather than to insert calls in the code path for main resources.
Comment on attachment 394730 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394730&action=review > Source/WebCore/loader/cache/CachedResourceLoader.cpp:819 > + if (request.options().mode == FetchOptions::Mode::Navigate) Spec says to check the destination is Document, might be best to do that since this is not 100% equivalent. LinkLoader for instance uses FetchOptions::Mode::Navigate. > Source/WebCore/platform/network/ResourceRequestBase.cpp:770 > + // FIXME: Identify HSTS cases and avoid adding the header. <https://bugs.webkit.org/show_bug.cgi?id=157885> If we have to check HSTS, I am not sure ResourceRequestBase is the right place to add this routine. Maybe we can inline it in CachedResourceLoader for now.
Created attachment 395057 [details] Patch
Comment on attachment 395057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395057&action=review I would have said review+ because all other changes seem obviously right, but there’s one I don’t understand > Source/WebCore/loader/FrameLoader.cpp:1522 > - addExtraFieldsToMainResourceRequest(r); > + addExtraFieldsToRequest(r, m_loadType, true); Change log doesn’t explain this change. If addExtraFieldsToMainResourceRequest still exists, why is this one call site not using it?
Comment on attachment 395057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395057&action=review >> Source/WebCore/loader/FrameLoader.cpp:1522 >> + addExtraFieldsToRequest(r, m_loadType, true); > > Change log doesn’t explain this change. If addExtraFieldsToMainResourceRequest still exists, why is this one call site not using it? I would like to remove addExtraFieldsToMainResourceRequest and addExtraFieldsToSubresourceRequest if possible and this is a step in that direction. However I can remove this part of the patch if you feel it is not appropriate here.
Comment on attachment 395057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395057&action=review >>> Source/WebCore/loader/FrameLoader.cpp:1522 >>> + addExtraFieldsToRequest(r, m_loadType, true); >> >> Change log doesn’t explain this change. If addExtraFieldsToMainResourceRequest still exists, why is this one call site not using it? > > I would like to remove addExtraFieldsToMainResourceRequest and addExtraFieldsToSubresourceRequest if possible and this is a step in that direction. However I can remove this part of the patch if you feel it is not appropriate here. I don’t mean to make any statement about whether it’s appropriate. I just don’t understand why it’s correct, nor does the change log say why. I trust you that it’s a good change, but I’d like you to explain why.
Comment on attachment 395057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395057&action=review >>>> Source/WebCore/loader/FrameLoader.cpp:1522 >>>> + addExtraFieldsToRequest(r, m_loadType, true); >>> >>> Change log doesn’t explain this change. If addExtraFieldsToMainResourceRequest still exists, why is this one call site not using it? >> >> I would like to remove addExtraFieldsToMainResourceRequest and addExtraFieldsToSubresourceRequest if possible and this is a step in that direction. However I can remove this part of the patch if you feel it is not appropriate here. > > I don’t mean to make any statement about whether it’s appropriate. I just don’t understand why it’s correct, nor does the change log say why. > > I trust you that it’s a good change, but I’d like you to explain why. Maybe it is better to keep it for now like this and have another patch that would remove addExtraFieldsToMainResourceRequest.
Comment on attachment 395057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395057&action=review >>>>> Source/WebCore/loader/FrameLoader.cpp:1522 >>>>> + addExtraFieldsToRequest(r, m_loadType, true); >>>> >>>> Change log doesn’t explain this change. If addExtraFieldsToMainResourceRequest still exists, why is this one call site not using it? >>> >>> I would like to remove addExtraFieldsToMainResourceRequest and addExtraFieldsToSubresourceRequest if possible and this is a step in that direction. However I can remove this part of the patch if you feel it is not appropriate here. >> >> I don’t mean to make any statement about whether it’s appropriate. I just don’t understand why it’s correct, nor does the change log say why. >> >> I trust you that it’s a good change, but I’d like you to explain why. > > Maybe it is better to keep it for now like this and have another patch that would remove addExtraFieldsToMainResourceRequest. Right, I can see now that it is not super obvious. Basically addExtraFieldsToMainResourceRequest is only special because it does an extra thing besides calling addExtraFieldsToRequest correctly for main resources, it calls addHTTPUpgradeInsecureRequestsIfNeeded. With this change the addHTTPUpgradeInsecureRequestsIfNeeded is only being done in CachedResourceLoader, so there is not anything special anymore about addExtraFieldsToMainResourceRequest when being called from FrameLoader so I inlined it. The situation is different for the other call site, DocumentLoader, since it has no access to m_loadType, so I left it untouched for now.
Comment on attachment 395057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395057&action=review >>>>>> Source/WebCore/loader/FrameLoader.cpp:1522 >>>>>> + addExtraFieldsToRequest(r, m_loadType, true); >>>>> >>>>> Change log doesn’t explain this change. If addExtraFieldsToMainResourceRequest still exists, why is this one call site not using it? >>>> >>>> I would like to remove addExtraFieldsToMainResourceRequest and addExtraFieldsToSubresourceRequest if possible and this is a step in that direction. However I can remove this part of the patch if you feel it is not appropriate here. >>> >>> I don’t mean to make any statement about whether it’s appropriate. I just don’t understand why it’s correct, nor does the change log say why. >>> >>> I trust you that it’s a good change, but I’d like you to explain why. >> >> Maybe it is better to keep it for now like this and have another patch that would remove addExtraFieldsToMainResourceRequest. > > Right, I can see now that it is not super obvious. > Basically addExtraFieldsToMainResourceRequest is only special because it does an extra thing besides calling addExtraFieldsToRequest correctly for main resources, it calls addHTTPUpgradeInsecureRequestsIfNeeded. With this change the addHTTPUpgradeInsecureRequestsIfNeeded is only being done in CachedResourceLoader, so there is not anything special anymore about addExtraFieldsToMainResourceRequest when being called from FrameLoader so I inlined it. The situation is different for the other call site, DocumentLoader, since it has no access to m_loadType, so I left it untouched for now. So some mention of this in the change log would have helped me understand reviewing, and maybe helped others understand later, too. Which is why I asked.
Comment on attachment 395057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395057&action=review >>>>>>> Source/WebCore/loader/FrameLoader.cpp:1522 >>>>>>> + addExtraFieldsToRequest(r, m_loadType, true); >>>>>> >>>>>> Change log doesn’t explain this change. If addExtraFieldsToMainResourceRequest still exists, why is this one call site not using it? >>>>> >>>>> I would like to remove addExtraFieldsToMainResourceRequest and addExtraFieldsToSubresourceRequest if possible and this is a step in that direction. However I can remove this part of the patch if you feel it is not appropriate here. >>>> >>>> I don’t mean to make any statement about whether it’s appropriate. I just don’t understand why it’s correct, nor does the change log say why. >>>> >>>> I trust you that it’s a good change, but I’d like you to explain why. >>> >>> Maybe it is better to keep it for now like this and have another patch that would remove addExtraFieldsToMainResourceRequest. >> >> Right, I can see now that it is not super obvious. >> Basically addExtraFieldsToMainResourceRequest is only special because it does an extra thing besides calling addExtraFieldsToRequest correctly for main resources, it calls addHTTPUpgradeInsecureRequestsIfNeeded. With this change the addHTTPUpgradeInsecureRequestsIfNeeded is only being done in CachedResourceLoader, so there is not anything special anymore about addExtraFieldsToMainResourceRequest when being called from FrameLoader so I inlined it. The situation is different for the other call site, DocumentLoader, since it has no access to m_loadType, so I left it untouched for now. > > So some mention of this in the change log would have helped me understand reviewing, and maybe helped others understand later, too. > > Which is why I asked. Thanks for the feedback, I do tend to be terse and hand-wavy, so I will try to improve here. I do like the detailed explanations given in Changelogs per file/method so maybe I'll try using that more.
Created attachment 395067 [details] Patch
Committed r259308: <https://trac.webkit.org/changeset/259308> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395067 [details].
<rdar://problem/61121370>