Closed Bug 1331275 Opened 3 years ago Closed 6 days ago

SiteSpecific overrides no more work properly since the changes in bug #1148544

Categories

(Core :: Networking: HTTP, defect, P2)

49 Branch
All
Linux
defect

Tracking

()

RESOLVED WONTFIX

People

(Reporter: dmitry, Assigned: droeh)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(3 files)

Site-specific overrides no more work properly (since the change in the bug #1148544).

Previously, any browser request which match site-specific preferencies takes effect in "any place" of such a request.

Now, only "initial" one (or as typed in urlbar) is used. Any additional requests, which can be needed to load the page (for .css, .js, etc.), are not affected, and the standard UA string is used for them (or the common general.useragent.override, if any).

Certainly, it might looks strange from the server side, why the same browser loads additional components of the same page with dufferent UA strings...

Additionally, now it is impossible at all to alter UA string for any "non-urlbar" situations -- such as described in https://bugzilla.mozilla.org/show_bug.cgi?id=1148544#c35 


Test case (for Linux):

Consider a site, fe. "http://ssl.panoramio.com" (don't bother about ssl in the name, just http://).
Specify "general.useragent.override.ssl.panoramio.com" with the dirrerent value -- fe. change the version (to, say, "38.0"). Restart the browser.
Now go to "http://ssl.panoramio.com". (To test things properly, the full "Reload" of the page will be needed, to avoid any caching.)
Invoke raw IP packet dumping stuff, fe. "tcpdump -w dumpfile port 80"; perform reload of the page in the browser; then stop dumping.
Filter out the needed info from the dump, fe. "sed -n '/Host:/,/User-Agent:/ p' dumpfile"
Attached file expected results
The expected results, as for versions without the changes, or after the reverting patch applied
Some thoughts:

The bug #1148544 has "Platform: All Android". Is it suitable not to apply those changes for non-Android world?

The SiteSpecific useragent overrides seem to be rare enough for the common desktop usage. If the optimizations in their stuff are actually needed? If so, probably just set general.useragent.site_specific_overrides to false by default?
Blocks: 1148544
based on the tagged regression bug, dylan can you sort this out?
Assignee: nobody → droeh
Flags: needinfo?(droeh)
Whiteboard: [necko-active]
We knew that bug 1148544 would result in this change in behavior (all subresource requests getting the same UA as the top level resource request) at the time I wrote the patch and judged that it was a worthwhile trade-off considering the performance benefits -- I don't know if benchmarking was done on desktop, but on Android we see in some instances savings of hundreds of milliseconds in page-load time. I would definitely prefer not to revert the changes i made for bug 1148544, and I don't see any obvious alternative that preserves the old behavior while keeping the performance gains. I'm open to suggestions if anyone has them, but otherwise I'm inclined to think this should be WONTFIXed.
Flags: needinfo?(droeh)
> all subresource requests getting the same UA as the top level resource request
Could you please explain a little more what you mean "the same"?

According to my test results (attached above), "all subresource requests" getting the default UA, not the "top level resource request"...

> I don't know if benchmarking was done on desktop
> I don't see any obvious alternative that preserves the old behavior while keeping the
> performance gains.
Maybe make this change Android-specific (by #ifdef in the code)?

Anyway, to keep the performance gain (is it actually needed for desktop?) and the same time to preserve the needed functionality, it could be useful to have an additional config option, or to alter the existing "general.useragent.site_specific_overrides" to be 3 level (0 - no overrides, 1 - new behavior, 2 - old (full overrides) behavior).

Please note, that old "good" behaviour works fine for years; people expect it still works.
This feature was initially implemented for some reasons, and such reasons still exist.
(In reply to Dmitry Butskoy from comment #6)
> Could you please explain a little more what you mean "the same"?
> 
> According to my test results (attached above), "all subresource requests"
> getting the default UA, not the "top level resource request"...

Ah, sorry, I misread your log. There is indeed something going on here -- the subresource requests should be getting the user agent override. I tested that before landing the patch, but only on Android -- so either something desktop specific is happening here or there has been a regression since bug 1148544. I'll certainly look into that.

> > I don't know if benchmarking was done on desktop
> > I don't see any obvious alternative that preserves the old behavior while keeping the
> > performance gains.
> Maybe make this change Android-specific (by #ifdef in the code)?
> 
> Anyway, to keep the performance gain (is it actually needed for desktop?)
> and the same time to preserve the needed functionality, it could be useful
> to have an additional config option, or to alter the existing
> "general.useragent.site_specific_overrides" to be 3 level (0 - no overrides,
> 1 - new behavior, 2 - old (full overrides) behavior).
> 
> Please note, that old "good" behaviour works fine for years; people expect
> it still works.
> This feature was initially implemented for some reasons, and such reasons
> still exist.

I can't speak for performance concerns on desktop. I would definitely prefer not to #ifdef this based on platform. Having 3 levels for the general.useragent.site_specific_overrides pref is a more interesting solution; I don't think I have the time to work on that at the moment, but it's probably the best approach if we want to preserve the old way of handling user agent overrides.
(In reply to Dylan Roeh (:droeh) from comment #7)
> Ah, sorry, I misread your log. There is indeed something going on here --
> the subresource requests should be getting the user agent override. I tested
> that before landing the patch, but only on Android -- so either something
> desktop specific is happening here or there has been a regression since bug
> 1148544. I'll certainly look into that.

Follow-up: This is something that broke between when I landed the patch for bug 1148544 and now, I'm fairly certain. When determining what user agent to use for a subresource request, I only apply the cached user agent in the loadgroup if the request does not already have a user agent (ie if the user agent is an empty string); this is to avoid overriding the user agent in an XHR with an explicit user agent supplied. At the time I landed the patch, a request did not have a user agent set at this point by default, and so this check against an empty string was ok. But it seems that in the intervening time things have changed so that subresource requests will already have the default user agent when nsHttpChannel::SetLoadGroupUserAgentOverride() is called, which causes us to skip overriding the user agent for all subresource requests.

mcmanus, there are two issues here:
* We are currently not overriding the UA for any subresource request when we should, which is a regression that happened sometime after bug 1148544 landed (see above).
* bug 1148544 changes user agent override behavior, but we knew this going in and I think the fact that we've gone nearly a year with very few minor/obscure issues supports our decision to go ahead with the change. I would support splitting general.useragent.site_specific_overrides to have 3 levels, but I don't think it's something I can justify making a high priority for myself right now.
Flags: needinfo?(mcmanus)
> I would support splitting general.useragent.site_specific_overrides to have 3 levels, but I don't
> think it's something I can justify making a high priority for myself right now.

At least for 52ESR time could be fine.

Just to be sure to not forget things -- please, consider an example user case described in bug 1148544 comment 35 , which shows that such a 3 level thing is actually needed now, and might be needed in the future for a lot of similar rare/ugly issues, unfortunately happen from time to time.

BTW, it seems that on 45ESR site-specific overrides do not work at all (don't investigate why).
nick, after dealing with h2/0rtt do you think you'd have a chance to try and figure out where things went off the rails (comment 8)?
Flags: needinfo?(mcmanus)
nick - comment 10. sorry I missed the ni first time around
Flags: needinfo?(hurley)
(In reply to Patrick McManus [:mcmanus] from comment #10)
> nick, after dealing with h2/0rtt do you think you'd have a chance to try and
> figure out where things went off the rails (comment 8)?

I've got a couple other things on my plate as well, so not sure I can dedicate as much time to this as would be necessary. I do seem to remember some patch flying by that moved the code related to this by a few lines, but I don't know if that's what would've caused this. Leaving the ni? so I remember to do a little archaeology to see if that's the problem...
Bulk priority update: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P1
Priority: P1 → P2
Whiteboard: [necko-active]
Whiteboard: [necko-triaged]
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(u408661)

Does it affect SeaMonkey for now?
If yes, than all the efforts to avoid browser sniffing (by general.useragent.override.example.com etc.) just failed...

Flags: needinfo?(frgrahl)

Does it affect SeaMonkey for now?

Gecko so yes.

Flags: needinfo?(frgrahl)

Since we removed site specific UA handling in bug 1513574, we can now close this bug.

Status: ASSIGNED → RESOLVED
Closed: 6 days ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.