Bug 209587

Summary: Move applyUserAgentIfNeeded calls to a more central place
Product: WebKit Reporter: Rob Buis <rbuis>
Component: New BugsAssignee: Rob Buis <rbuis>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, cdumez, darin, ews-watchlist, japhet, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 210863, 213626    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Rob Buis 2020-03-26 01:53:27 PDT
Move applyUserAgentIfNeeded calls to a more central place.
Comment 1 Rob Buis 2020-03-26 01:55:15 PDT
Created attachment 394585 [details]
Patch
Comment 2 Rob Buis 2020-03-26 03:33:06 PDT
Created attachment 394589 [details]
Patch
Comment 3 Rob Buis 2020-03-26 04:13:49 PDT
Created attachment 394594 [details]
Patch
Comment 4 EWS 2020-03-27 09:12:41 PDT
Committed r259116: <https://trac.webkit.org/changeset/259116>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394594 [details].
Comment 5 Radar WebKit Bug Importer 2020-03-27 09:13:13 PDT
<rdar://problem/60970124>
Comment 6 Chris Dumez 2020-04-22 08:37:35 PDT
Reverted r259116 for reason:

Broke login flow on some apple-internal sites (rdar://problem/61905262)

Committed r260511: <https://trac.webkit.org/changeset/260511>
Comment 7 Darin Adler 2020-04-22 08:40:54 PDT
We know that this revision caused the login failures. We don’t know what behavior changed. Need to figure that out, make tests, to make sure future refactoring doesn’t break this again.
Comment 8 Chris Dumez 2020-04-22 08:43:16 PDT
(In reply to Darin Adler from comment #7)
> We know that this revision caused the login failures. We don’t know what
> behavior changed. Need to figure that out, make tests, to make sure future
> refactoring doesn’t break this again.

Understood. I will follow-up with more investigation and hopefully a test. For now, my priority is to get us back into a working state.
Comment 9 Chris Dumez 2020-04-22 08:58:05 PDT
(In reply to Chris Dumez from comment #8)
> (In reply to Darin Adler from comment #7)
> > We know that this revision caused the login failures. We don’t know what
> > behavior changed. Need to figure that out, make tests, to make sure future
> > refactoring doesn’t break this again.
> 
> Understood. I will follow-up with more investigation and hopefully a test.
> For now, my priority is to get us back into a working state.

I did some digging with Web Inspector and it looks like if you do a navigation like so:
"window.location = 'test.html';"

then WebCore no longer sets the User-Agent header on the main resource request to test.html
Comment 10 Chris Dumez 2020-04-22 09:03:35 PDT
Comment on attachment 394594 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394594&action=review

> Source/WebCore/loader/FrameLoader.cpp:-2979
> -    applyUserAgentIfNeeded(request);

For the scenario I mentioned, this used to add the user-agent header.
Comment 11 Chris Dumez 2020-04-22 10:05:59 PDT
(In reply to Chris Dumez from comment #10)
> Comment on attachment 394594 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394594&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:-2979
> > -    applyUserAgentIfNeeded(request);
> 
> For the scenario I mentioned, this used to add the user-agent header.

I was wrong, the User-Agent header seems to be missing from sync XHR requests. I have a test almost ready.
Comment 12 Rob Buis 2020-04-22 10:11:11 PDT
(In reply to Chris Dumez from comment #11)
> (In reply to Chris Dumez from comment #10)
> > Comment on attachment 394594 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=394594&action=review
> > 
> > > Source/WebCore/loader/FrameLoader.cpp:-2979
> > > -    applyUserAgentIfNeeded(request);
> > 
> > For the scenario I mentioned, this used to add the user-agent header.
> 
> I was wrong, the User-Agent header seems to be missing from sync XHR
> requests. I have a test almost ready.

Thanks for your work here! My patch probably did not add an applyUserAgentIfNeeded to FrameLoader::loadResourceSynchronously, to correct the applyUserAgentIfNeeded that was removed from addExtraFieldsToRequest.
Comment 13 Chris Dumez 2020-04-22 10:25:23 PDT
(In reply to Darin Adler from comment #7)
> We know that this revision caused the login failures. We don’t know what
> behavior changed. Need to figure that out, make tests, to make sure future
> refactoring doesn’t break this again.

Added test that would have failed via https://bugs.webkit.org/show_bug.cgi?id=210863.
Comment 14 Rob Buis 2020-04-22 12:05:07 PDT
Created attachment 397229 [details]
Patch
Comment 15 Darin Adler 2020-04-22 14:22:57 PDT
Comment on attachment 397229 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397229&action=review

> Source/WebCore/loader/cache/CachedResourceLoader.h:178
> -    void updateHTTPRequestHeaders(CachedResource::Type, CachedResourceRequest&);
> +    void updateHTTPRequestHeaders(FrameLoader&, CachedResource::Type, CachedResourceRequest&);

This doesn’t seem good.
Comment 16 Rob Buis 2020-04-22 21:39:03 PDT
Comment on attachment 397229 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397229&action=review

>> Source/WebCore/loader/cache/CachedResourceLoader.h:178
>> +    void updateHTTPRequestHeaders(FrameLoader&, CachedResource::Type, CachedResourceRequest&);
> 
> This doesn’t seem good.

Can you expand on what is not good? This is the original patch plus the fix to loadResourceSynchronously.
Comment 17 Darin Adler 2020-04-23 09:15:56 PDT
Comment on attachment 397229 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397229&action=review

>>> Source/WebCore/loader/cache/CachedResourceLoader.h:178
>>> +    void updateHTTPRequestHeaders(FrameLoader&, CachedResource::Type, CachedResourceRequest&);
>> 
>> This doesn’t seem good.
> 
> Can you expand on what is not good? This is the original patch plus the fix to loadResourceSynchronously.

Having to pass a FrameLoader here bothers me. But it’s not really about this one patch.
Comment 18 Rob Buis 2020-04-23 13:33:43 PDT
Comment on attachment 397229 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397229&action=review

>>>> Source/WebCore/loader/cache/CachedResourceLoader.h:178
>>>> +    void updateHTTPRequestHeaders(FrameLoader&, CachedResource::Type, CachedResourceRequest&);
>>> 
>>> This doesn’t seem good.
>> 
>> Can you expand on what is not good? This is the original patch plus the fix to loadResourceSynchronously.
> 
> Having to pass a FrameLoader here bothers me. But it’s not really about this one patch.

I see, one impovement may be to inline this more in CachedResourceLoader::requestResource, and make sure to order well and add the right comments and spec links. There is probably still quite some we can do to improve that method, I made a start here https://bugs.webkit.org/show_bug.cgi?id=210925.
Comment 19 EWS 2020-04-23 13:37:58 PDT
Committed r260596: <https://trac.webkit.org/changeset/260596>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397229 [details].
Comment 20 Alex Christensen 2020-06-25 16:37:47 PDT
This broke our API.  See https://bugs.webkit.org/show_bug.cgi?id=213626