Bug 209587 – Move applyUserAgentIfNeeded calls to a more central place

• Component: New Bugs
• Status: RESOLVED FIXED

[Rob Buis]

Move applyUserAgentIfNeeded calls to a more central place.

[Rob Buis]

Created attachment 394585 [details]
Patch

[Rob Buis]

Created attachment 394589 [details]
Patch

[Rob Buis]

Created attachment 394594 [details]
Patch

[EWS]

Committed r259116: <https://trac.webkit.org/changeset/259116>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394594 [details].

[Radar WebKit Bug Importer]

<rdar://problem/60970124>

[Chris Dumez]

Reverted r259116 for reason:

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

Committed r260511: <https://trac.webkit.org/changeset/260511>

[Darin Adler]

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.

[Chris Dumez]

(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.

[Chris Dumez]

(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

[Chris Dumez]

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.

[Chris Dumez]

(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.

[Rob Buis]

(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.

[Chris Dumez]

(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.

[Rob Buis]

Created attachment 397229 [details]
Patch

[Darin Adler]

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.

[Rob Buis]

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.

[Darin Adler]

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.

[Rob Buis]

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.

[EWS]

Committed r260596: <https://trac.webkit.org/changeset/260596>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397229 [details].

[Alex Christensen]

This broke our API.  See https://bugs.webkit.org/show_bug.cgi?id=213626