Closed Bug 1148544 Opened 5 years ago Closed 4 years ago

Spend less time figuring out UA overrides

Categories

(Core :: Networking: HTTP, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: snorp, Assigned: droeh)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(3 files, 12 obsolete files)

1.84 MB, text/plain
Details
15.60 KB, patch
droeh
: review+
Details | Diff | Splinter Review
16.37 KB, patch
jchen
: review+
Details | Diff | Splinter Review
Attached file Profile (obsolete) —
With the attached profile, we spend a decent amount of time in HTTP_on_modify_request @ UserAgentOverrides.jsm. It seems like we could be smarter about this.
Duplicate of this bug: 1148450
Attached file Better profile
Attachment #8584731 - Attachment is obsolete: true
In the attached profile, you can clearly see that running the request observers takes the bulk of the time when starting a new request. Since we seem to mostly (only?) use this for UA overrides, we should look into a more streamlined solution.
Blocks: 1149612
No longer blocks: 857359
I think instead of running all of this stuff to determine the UA for each request, we should just run it once for the document URI. Mike what do you think about that? Would this adversely affect webcompat stuff?
Flags: needinfo?(miket)
> we should just run it once for the document URI. Mike what do you think about that? Would this adversely affect webcompat stuff?

Would that affect being able to have an override per-host, i.e., yahoo.com or mail.google.com?
(In reply to Mike Taylor [:miketaylr] from comment #5)
> > we should just run it once for the document URI. Mike what do you think about that? Would this adversely affect webcompat stuff?
> 
> Would that affect being able to have an override per-host, i.e., yahoo.com
> or mail.google.com?

It would if for instance you were navigating to foobar.com and it loaded some resource (js, css) from yahoo.com. The resource load for yahoo.com would use whatever UA we determined for the containing document (in this case foobar.com). But if you went directly to yahoo.com or loaded yahoo.com in an iframe, things would work as they do now more or less.
Flags: needinfo?(snorp)
Cool. Off the top of my head I can't think of an example where that would be catastrophic since we're usually spoofing for some domain to make things better. But there's only one way to find out...™
Assignee: nobody → droeh
Attached patch Proposed patch (obsolete) — Splinter Review
Proposed patch; some tests will need to be updated since they rely on figuring out the UA override for subresources.
Attached patch Proposed patch (obsolete) — Splinter Review
Proposed patch that caches the UA override in the LoadGroup.
Attachment #8702682 - Attachment is obsolete: true
Attachment #8712351 - Flags: review?(mcmanus)
Comment on attachment 8712351 [details] [diff] [review]
Proposed patch

Review of attachment 8712351 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsLoadGroup.h
@@ +102,5 @@
>  
>      /* For nsPILoadGroupInternal */
>      uint32_t                        mTimedNonCachedRequestsUntilOnEndPageLoad;
> +
> +    nsAutoCString                   mUserAgentOverride;

typically auto is only used on the stack

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5114,5 @@
>      // notify "http-on-modify-request" observers
>      CallOnModifyRequestObservers();
>  
> +    if (mLoadGroup) {
> +      nsLoadGroup* loadGroup = static_cast<nsLoadGroup*>(mLoadGroup.get());

you're probably going to need mLoadGroup->GetRootLoadGroup() because they can end up chained.

also, for e10s reasons I forget we had to stop doing something similar for the h2 push cache.. nick fixed it so I'll have him review too

::: netwerk/protocol/http/nsIHttpProtocolHandler.idl
@@ +114,5 @@
>  #define NS_HTTP_ON_EXAMINE_CACHED_RESPONSE_TOPIC "http-on-examine-cached-response"
>  
> +/**
> + * The observer of this topic is notified when a channel for a Document URI is
> + * created.

that comment doesn't really describe it..
Attachment #8712351 - Flags: review?(mcmanus) → review?(hurley)
Comment on attachment 8712351 [details] [diff] [review]
Proposed patch

Review of attachment 8712351 [details] [diff] [review]:
-----------------------------------------------------------------

In addition to mcmanus' comments, see mine.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5114,5 @@
>      // notify "http-on-modify-request" observers
>      CallOnModifyRequestObservers();
>  
> +    if (mLoadGroup) {
> +      nsLoadGroup* loadGroup = static_cast<nsLoadGroup*>(mLoadGroup.get());

Right, so the problem here is that load groups don't exist on the parent process, so this won't work in e10s. I worked around it by adding an nsISchedulingContext object, whose lifetime is directly tied to that of the load group in the child process. (All the history for this, by the way, is in bug 1127618.)

I see two options here, based on a relatively cursory reading of HttpChannelChild::AsyncOpen.

The first (which I think is preferable, if it works) is to do this work in HttpChannelChild before sending the AsyncOpen call to the parent. We already appear to do something similar by calling HttpBaseChannel::SetDocshellUserAgentOverride in HttpChannelChild::AsyncOpen. You'll also have to ensure this work gets done in the parent if we're running in single-process mode.

The second, and less preferable option, is to piggy-back on the scheduling context work mentioned above. It probably doesn't make sense to re-create all that infrastructure I created around scheduling contexts for this option, so perhaps a minor refactoring/renaming of the concrete class and a new IDL for the UA overrides is the way to go here. Then you can probably just hang the override off the nsIUserAgentOverride (or whatever) that's backed by the same concrete class that backs nsISchedulingContext.
Attachment #8712351 - Flags: review?(hurley)
Attached patch Proposed patch (updated) (obsolete) — Splinter Review
Thanks for the feedback. Nick, I went with your first recommended approach and it seems to be good. 

This also includes fixes for the issues McManus brought up.
Attachment #8712351 - Attachment is obsolete: true
Attachment #8712922 - Flags: review?(hurley)
Comment on attachment 8712922 [details] [diff] [review]
Proposed patch (updated)

Review of attachment 8712922 [details] [diff] [review]:
-----------------------------------------------------------------

This is definitely headed in the right direction, just a few more things to think about/take care of and I think we're there!

::: netwerk/base/nsLoadGroup.h
@@ +54,5 @@
>      virtual ~nsLoadGroup();
>  
>      nsresult Init();
>  
> +    nsCString GetUserAgentOverride() const

Thinking about this more, I think I'd rather have userAgentOverride as an attribute on nsILoadGroup - that way we don't have all the mess of static_casts below, we can work totally with QI.

@@ +59,5 @@
> +    {
> +      return mUserAgentOverride;
> +    }
> +
> +    void SetUserAgentOverride(nsAutoCString aUserAgentOverride)

Use |nsCString &| as the argument type

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1808,5 @@
>  
>    // Set user agent override
>    HttpBaseChannel::SetDocshellUserAgentOverride();
>  
> +  if (mLoadGroup) {

Let's factor everything in this if block into something in HttpBaseChannel, so you're not duplicating between here and nsHttpChannel. Then just do a call like above.

@@ +1812,5 @@
> +  if (mLoadGroup) {
> +    nsLoadGroup* loadGroup = static_cast<nsLoadGroup*>(mLoadGroup.get());
> +    nsILoadGroup* iLoadGroup = nullptr;
> +    loadGroup->GetRootLoadGroup(&iLoadGroup);
> +    loadGroup = static_cast<nsLoadGroup*>(iLoadGroup);

Either use QI here (and null-check pointers/check rv) or, if you stick with this approach, make sure you null-check iLoadGroup before using it.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5126,5 @@
> +        loadGroup->SetUserAgentOverride(ua);
> +      } else {
> +        SetRequestHeader(NS_LITERAL_CSTRING("User-Agent"), loadGroup->GetUserAgentOverride(), false);
> +      }
> +    }

Is there a reason this codepath doesn't have the else that the one in HttpChannelChild has? Also, we probably only want to do this if we're running in single-process mode (since it's already done otherwise). I realize that's the case if mLoadGroup != nullptr, but would be nice to make it explicit (at least with a comment).
Attachment #8712922 - Flags: review?(hurley) → feedback+
Attached patch Proposed patch (updated) (obsolete) — Splinter Review
Thanks again for the feedback. I think this version addresses everything you brought up, let me know what you think.
Attachment #8712922 - Attachment is obsolete: true
Attachment #8714808 - Flags: review?(hurley)
Comment on attachment 8714808 [details] [diff] [review]
Proposed patch (updated)

Review of attachment 8714808 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! Just a couple minor nits, and we're good to go. I'd like to see the final patch before r+ and checkin.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +2469,5 @@
> +void
> +HttpBaseChannel::SetLoadGroupUserAgentOverride()
> +{
> +  nsCOMPtr<nsILoadGroupChild> childLoadGroup = do_QueryInterface(mLoadGroup);
> +  nsCOMPtr<nsILoadGroup> rootLoadGroup = nullptr;

No need to explicitly initialize to nullptr, ctor does that by default

@@ +2470,5 @@
> +HttpBaseChannel::SetLoadGroupUserAgentOverride()
> +{
> +  nsCOMPtr<nsILoadGroupChild> childLoadGroup = do_QueryInterface(mLoadGroup);
> +  nsCOMPtr<nsILoadGroup> rootLoadGroup = nullptr;
> +  if (childLoadGroup) childLoadGroup->GetRootLoadGroup(getter_AddRefs(rootLoadGroup));

braces around the body, and body on separate line, like:

if (childLoadGroup) {
   ...
}
Attachment #8714808 - Flags: review?(hurley)
Attached patch Proposed patch (updated) (obsolete) — Splinter Review
OK, this should address those suggestions.
Attachment #8714808 - Attachment is obsolete: true
Attachment #8715310 - Flags: review?(hurley)
Comment on attachment 8715310 [details] [diff] [review]
Proposed patch (updated)

Review of attachment 8715310 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome, thanks! (don't forget to update your commit message before landing)
Attachment #8715310 - Flags: review?(hurley) → review+
Attached patch Mochitest patch (obsolete) — Splinter Review
Jim, do you mind reviewing the updates to the UAO mochitests?
Attachment #8716361 - Flags: review?(nchen)
Attached patch Proposed patch (updated) (obsolete) — Splinter Review
Nick, I had to make a small change to fix some test breakage related to XHRs with explicit user agents. The only change from the last version is a check in HttpBaseChannel.cpp to see if the channel already has a user agent set before reading from the cached UA override in the loadgroup.
Attachment #8715310 - Attachment is obsolete: true
Attachment #8716993 - Flags: review?(hurley)
Comment on attachment 8716993 [details] [diff] [review]
Proposed patch (updated)

Review of attachment 8716993 [details] [diff] [review]:
-----------------------------------------------------------------

Just a couple nits on this last one. Fix them up before landing, but no need to re-request review. Thanks!

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +2479,5 @@
> +      GetRequestHeader(NS_LITERAL_CSTRING("User-Agent"), ua);
> +      rootLoadGroup->SetUserAgentOverrideCache(ua);
> +    } else {
> +      GetRequestHeader(NS_LITERAL_CSTRING("User-Agent"), ua);
> +      // If the user agent is already set, don't override it.

Explain the why here, not just the what - perhaps something like:

We only overwrite an empty U-A here, because otherwise we would break XHRs with explicitly-set U-As.

@@ +2480,5 @@
> +      rootLoadGroup->SetUserAgentOverrideCache(ua);
> +    } else {
> +      GetRequestHeader(NS_LITERAL_CSTRING("User-Agent"), ua);
> +      // If the user agent is already set, don't override it.
> +      if (NS_LITERAL_CSTRING("") == ua) {

if (ua.IsEmpty()) { ...
Attachment #8716993 - Flags: review?(hurley) → review+
Comment on attachment 8716361 [details] [diff] [review]
Mochitest patch

Review of attachment 8716361 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/test/mochitests/mochitest.ini
@@ -21,5 @@
>  skip-if = e10s
>  [test_uri_scheme.html]
>  [test_user_agent_overrides.html]
>  skip-if = e10s
> -[test_user_agent_updates.html]

Why are you removing "test_user_agent_updates"? I didn't see anything about it in the bug. I assume we still want updates with the new override mechanism.

::: netwerk/test/mochitests/test_user_agent_overrides.html
@@ +225,5 @@
>  
> +const Services = SpecialPowers.Cu.import("resource://gre/modules/Services.jsm").Services;
> +
> +let chromeWin = Services.wm.getMostRecentWindow("navigator:browser");
> +let BrowserApp = chromeWin.BrowserApp;

Have you run this with Fennec? I think your BrowserApp usage is specific to desktop. Is it possible to use standard APIs (e.g. window.open)?
Attachment #8716361 - Flags: review?(nchen) → feedback+
(In reply to Jim Chen [:jchen] [:darchons] from comment #21)
> Comment on attachment 8716361 [details] [diff] [review]
> > -[test_user_agent_updates.html]
> 
> Why are you removing "test_user_agent_updates"? I didn't see anything about
> it in the bug. I assume we still want updates with the new override
> mechanism.

Yes, please don't remove this test, we rely on the functionality for dynamic UA overrides in mobile-land (see ua-update.json.in).
Attached patch Mochitest patch (updated) (obsolete) — Splinter Review
Right, I thought test_user_agent_updates.html was redundant at first, but I see what it's doing now. This patch includes updates to those tests, but with an ugly workaround in testDownload().
Attachment #8716361 - Attachment is obsolete: true
Attachment #8717493 - Flags: review?(nchen)
Comment on attachment 8717493 [details] [diff] [review]
Mochitest patch (updated)

Review of attachment 8717493 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/test/mochitests/test_user_agent_overrides.html
@@ +38,4 @@
>  
> +  browser.addEventListener("load", function handle() {
> +    ok(sameQ == (browser.contentDocument.body.innerHTML.indexOf(expected) != -1), message);
> +    ok(!testNavQ || (navSameQ == (browser.contentWindow.navigator.userAgent.indexOf(expected) != -1)), navMessage);

"navExpected" instead of "expected"? Also, can you wrap the ok statement inside | if (testNavQ) |

::: netwerk/test/mochitests/test_user_agent_updates.html
@@ +56,5 @@
> +  let browser = tab.browser;
> +
> +  browser.addEventListener("load", function handle() {
> +    ok(sameQ == (browser.contentDocument.body.innerHTML.indexOf(expected) != -1), message);
> +    ok(!testNavQ || (navSameQ == (browser.contentWindow.navigator.userAgent.indexOf(expected) != -1)), navMessage);

Same here.
Attachment #8717493 - Flags: review?(nchen) → review+
Attached patch Mochitest patch (updated) (obsolete) — Splinter Review
Carry over Jim's r+
Attachment #8717493 - Attachment is obsolete: true
Attachment #8717579 - Flags: review?(nchen)
Attachment #8717579 - Flags: review?(nchen) → review+
Attached patch Proposed patch (updated) (obsolete) — Splinter Review
Carry over Nick's r+
Attachment #8716993 - Attachment is obsolete: true
Attachment #8717581 - Flags: review+
Keywords: checkin-needed
snorp noticed this broke local file loading, this just updates the previous patch to handle file:// URI's separately. Carry over Nick's r+.
Attachment #8717581 - Attachment is obsolete: true
Attachment #8717619 - Flags: review+
Attached patch Mochitest patch (updated) (obsolete) — Splinter Review
Joel,

Here's the test I was talking about. As far as I can tell, the message event listener added on line 127 never actually receives a message event when the url given to the iframe is any domain other than mochi.test
Attachment #8717579 - Attachment is obsolete: true
Flags: needinfo?(droeh)
Attachment #8721008 - Flags: feedback?(jmaher)
this is a tough problem.  I got stumped on this after applying the patch and hacking around for a while.  A few thoughts:
* there is a lot of recursion going on here, with specialPowers.pushPrefEnv and addListener calls.  Could we be just shooting ourselves in the foot?
* I looked for other examples of iframe.src and multi domain, I see:
** https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/mochitest/stricttransportsecurity/verify.sjs
** https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/mochitest/stricttransportsecurity/test_stricttransportsecurity.html#29
* the patch didn't have removelistener calls, that might be part of the problem, but adding them with removeEventListener('message', argument.callee, false); didn't help.

and then I am out of ideas :(  I am open to helping out on this more, maybe if you have other ideas and get further I can help unblock then.
The similar test Joel linked above pointed me in the right direction for this (thanks!), it should work on desktop and mobile alike now.
Attachment #8721008 - Attachment is obsolete: true
Attachment #8721008 - Flags: feedback?(jmaher)
Attachment #8721523 - Flags: review?(nchen)
Attachment #8721523 - Flags: review?(nchen) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e65bfb056ef0
https://hg.mozilla.org/mozilla-central/rev/11a7fbfbe2d1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Regression found (which gone if I revert the change).


Let's describe a user case:

Many sites use "downloadable fonts", and many download them from http://fonts.googleapis.com .

Recently, Google implemented "unicode range" feature, which allows to decrease downloadable size. Since not all the browsers support it, Google determines whether the browser supports this feature by its User-Agent string. Such, it assumes that Firefox >= 44 should be fine.

Unfortunately, to properly render such a subset of the font, a well-known "bytecode" must be supported. Whereas a patent for bytecode already expired in 2010, some systems -- exactly RedHat Exterprise Linux 6 -- did not have enough time to adapt their standart fonts to new situation, hence just have disabled bytecode at all.

When a whole font is downloaded, the rendering in RHEL6 was fine even without bytecode. But with "unicode range", only part of the font is available, and rendering looks ugly, especially on non-latin symbols. It is because the mechanism used for hinting/antialiasing (without bytecode) requires the whole font for good results...

(BTW the problem was already described at https://bugzilla.mozilla.org/show_bug.cgi?id=1249973#c15
1) A Linux system without bytecode support (in freetype library) -- at least RedHat Enterprise Linux 6
2) A user locale with non-latin fonts, ie. cyrillic in Eastern Europe )

The solution would be to cause Google to behave as for Firefox <= 43, which usually could be done by siteSpecific overrides, ie:
general.useragent.overrides.fonts.googleapis.com = Firefox/..#Firefox/38

But now it takes effect only when specifying the target url in urlbar...

It is actually a regression, because there is a situation where the previous behavior might help in a simple convinient way, but when it appears to be actually needed, the feature is gone...

</user_case>


Please, consider the user case above just as an example, showing that the original behaviour seems sometimes to be still needed. (IOW, not the current "url_bar_Specific overrides" but true "url_Specific overrides").

Could someone consider an altering the change a little -- fe. implement another "about:config" variable which restores the previous way?
You should probably file a new bug and make it block this one.
Depends on: 1331275
You need to log in before you can comment on or make changes to this bug.