Firefox abuses vendor portions of user agent string

RESOLVED FIXED in mozilla1.8beta2

Status

()

Core
Networking: HTTP
RESOLVED FIXED
13 years ago
10 years ago

People

(Reporter: Christopher Aillon (sabbatical, not receiving bugmail), Assigned: dbaron)

Tracking

Trunk
mozilla1.8beta2
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch])

Attachments

(1 attachment, 4 obsolete attachments)

Redistributors of Firefox should be able to add their vendor name and versions
of distributions to the user agent string, but Firefox currently steals vendor
and vendorSub.

I talked to dbaron and we came up with either adding a vendor2 and vendorSub2 or
creating app and appSub for Firefox to use.  I prefer the latter to keep
consistency across the apps.
Created attachment 168878 [details] [diff] [review]
Patch

This is the patch I added to our firefox RPMs.
(Assignee)

Comment 2

13 years ago
Comment on attachment 168878 [details] [diff] [review]
Patch

>@@ -99,12 +99,22 @@ interface nsIHttpProtocolHandler : nsIPr

Rev UUID?

>@@ -520,12 +522,14 @@ nsHttpHandler::BuildUserAgent()
>                            mOscpu.Length() +
>                            mLanguage.Length() +
>                            mMisc.Length() +
>                            mProduct.Length() +
>                            mProductSub.Length() +
>                            mProductComment.Length() +
>+                           mApp.Length() +
>+                           mAppSub.Length() +
>                            mVendor.Length() +
>                            mVendorSub.Length() +
>                            mVendorComment.Length() +
>                            22);

24?



One other thing:  I didn't realize when I was talking to you that the Mozilla
and 5.0 were currently called mAppName and mAppVersion.  Adding something
called mApp and mAppSub seems a little confusing, but maybe it's OK.
(In reply to comment #2)
> (From update of attachment 168878 [details] [diff] [review] [edit])
> >@@ -99,12 +99,22 @@ interface nsIHttpProtocolHandler : nsIPr
> 
> Rev UUID?

Yeah, should do that.  :-\


> 
> >@@ -520,12 +522,14 @@ nsHttpHandler::BuildUserAgent()
> >                            mOscpu.Length() +
> >                            mLanguage.Length() +
> >                            mMisc.Length() +
> >                            mProduct.Length() +
> >                            mProductSub.Length() +
> >                            mProductComment.Length() +
> >+                           mApp.Length() +
> >+                           mAppSub.Length() +
> >                            mVendor.Length() +
> >                            mVendorSub.Length() +
> >                            mVendorComment.Length() +
> >                            22);
> 
> 24?


Oops, right.  Good catch.


> One other thing:  I didn't realize when I was talking to you that the Mozilla
> and 5.0 were currently called mAppName and mAppVersion.  Adding something
> called mApp and mAppSub seems a little confusing, but maybe it's OK.

Couldn't think of anything else at the time.  product and productSub are already
taken by Gecko/BUILDID.  Maybe prog/progSub?  ::shrug::  Other suggestions welcome.

Comment 4

13 years ago
Comment on attachment 168878 [details] [diff] [review]
Patch

can these values not be gotten via prefs?  i'm not happy to see
nsIHttpProtocolHandler changing.  it is one of the only ways for an embedder or
extension author to find out the Gecko BuildID at runtime.  i know of at least
one embedder who is forced to use it.
(Assignee)

Comment 5

13 years ago
So in a discussion with darin, he pointed out the idea of using enumeration of a
tree of preferences to solve this problem.  This allows enumeration of an
arbitrary number of comments.

I suspect the cleanest way to do this would be to get rid of the vendor and
vendorSub prefs and add a comment.* tree of prefs, such that Firefox would set
the pref:

pref("general.useragent.comment.firefox", "Firefox/@APP_VERSION@");

and Fedora could set the pref

pref("general.useragent.comment.fedora", "Fedora Core/3");

etc.


(Does nsIPrefBranch and the default branch stuff mean this also provides a way
to add the string programmatically by adding to the default branch, which would
allow an extension to add only when it's being used?)
Seems like a good idea.  Does order matter?  Firefox potentially can be in the
middle of several items.

> (Does nsIPrefBranch and the default branch stuff mean this also provides a way
> to add the string programmatically by adding to the default branch, which would
> allow an extension to add only when it's being used?)

Yep, that can be done.  I'll work on getting something hacked up.
Status: NEW → ASSIGNED
(Assignee)

Comment 7

13 years ago
We should probably be consistent about the order (i.e., given a set of prefs we
should always produce the same output), but I don't think it should matter.
(Assignee)

Updated

12 years ago
Assignee: caillon → dbaron
Status: ASSIGNED → NEW
Component: General → Networking: HTTP
OS: Linux → All
Product: Firefox → Core
Hardware: PC → All
Target Milestone: --- → mozilla1.8beta2
Version: 1.0 Branch → Trunk
(Assignee)

Comment 8

12 years ago
Created attachment 179889 [details] [diff] [review]
patch

NS_STRINGIFY didn't work and probably wasn't tested.

I'm not sure whether I want to use .extra. or .vendor., but .extra. seems more
generic and also doesn't have any potential issues with having both .vendor and
.vendor.* .

I probably need to test the app changes here a bit more (like making sure they
all compile).
Attachment #168878 - Attachment is obsolete: true
(Assignee)

Comment 9

12 years ago
Created attachment 179890 [details] [diff] [review]
patch

oops, wrong patch
Attachment #179889 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Attachment #179890 - Flags: review?(darin)
(Assignee)

Updated

12 years ago
Whiteboard: [patch]
(Assignee)

Comment 10

12 years ago
Comment on attachment 179890 [details] [diff] [review]
patch

>Index: netwerk/protocol/http/src/nsHttpHandler.h
>Index: calendar/sunbird/app/Makefile.in
> BUILD_ID = $(shell cat $(DEPTH)/config/build_number)
> DEFINES += -DBUILD_ID=\"$(BUILD_ID)\"

I removed these two lines as well, since I'm using NS_STRINGIFY now, and
BUILD_ID is global.

Comment 11

12 years ago
Comment on attachment 179890 [details] [diff] [review]
patch

>Index: netwerk/protocol/http/src/nsHttpHandler.cpp

>+    if (!mExtraUA.IsEmpty()) {
>+        mUserAgent += mExtraUA;
>+    }

nit pick: this code avoids brackets when possible


>+    if (MULTI_PREF_CHANGED(UA_PREF("extra."))) {
>+        mExtraUA.Truncate();
>+
>+        // Unfortunately, we can't do this using the pref branch.
>+        nsCOMPtr<nsIPrefService> service =
>+            do_GetService(NS_PREFSERVICE_CONTRACTID);

So, you should be able to get back to the nsIPrefService using
QueryInterface from the nsIPrefBranch passed to this function.
That might be slightly better.


>+            PRUint32 extraCount;
>+            char **extraItems;
>+            branch->GetChildList("", &extraCount, &extraItems);
>+            for (char **item = extraItems, **item_end = extraItems + extraCount;
>+                 item < item_end; ++item) {
>+                nsXPIDLCString valStr;
>+                branch->GetCharPref(*item, getter_Copies(valStr));
>+                if (!valStr.IsEmpty()) {
>+                    mExtraUA += NS_LITERAL_CSTRING(" ") + valStr;
>+                }

nit: remove extra brackets.

It looks like extraItems is leaked.

also, it's interesting that the order in which these extra values
are appended to the UA string is sort of arbitrary, or are we 
relying on some sorting done by the preferences code?  i think it
might be nice to ensure that these are always listed in a consistent
order even though people shouldn't really care.


>Index: xpcom/base/nscore.h

>+#define NS_STRINGIFY_HELPER(x_) #x_
>+#define NS_STRINGIFY(x_) NS_STRINGIFY_HELPER(x_)

So, I'm curious... why is this needed?


>Index: browser/app/profile/firefox.js

>+pref("general.useragent.extra.firefox", "Firefox/@APP_VERSION@");

We should make the same change to
mozilla/xulrunner/examples/simple/simple-prefs.js
before too many people start copying the old firefox way.


>Index: mail/app/Makefile.in

>+ifdef MOZ_JPROF
>+LIBS += -ljprof
>+endif

this change is unrelated right?


r=darin with these issues addressed
Attachment #179890 - Flags: review?(darin) → review+

Comment 12

12 years ago
Did some version of Fedora Core really ship with those changes to
nsIHttpProtocolHandler?  /me hopes not...
(Assignee)

Comment 13

12 years ago
(In reply to comment #11)
> So, you should be able to get back to the nsIPrefService using
> QueryInterface from the nsIPrefBranch passed to this function.
> That might be slightly better.

I tried that first and got a null pointer.

> >Index: xpcom/base/nscore.h
> 
> >+#define NS_STRINGIFY_HELPER(x_) #x_
> >+#define NS_STRINGIFY(x_) NS_STRINGIFY_HELPER(x_)
> 
> So, I'm curious... why is this needed?

Currently NS_STRINGIFY(BUILD_ID) produces "BUILD_ID".

Comment 14

12 years ago
> I tried that first and got a null pointer.

Wow.  That's interesting.  It probably has to do with when we get nsIPrefBranch
via the Observe callback.


> > So, I'm curious... why is this needed?
> 
> Currently NS_STRINGIFY(BUILD_ID) produces "BUILD_ID".

Yikes.  That has surely caused interesting problems for Firefox nightlies :-(
(Assignee)

Comment 15

12 years ago
Created attachment 179917 [details] [diff] [review]
patch

This should address darin's comments.  It does a sort by pref name since the
prefs code looks like it just uses hashtable enumeration order.

I haven't actually tested this and may not be able to for a few hours since
XRemote doesn't differentiate between profiles anymore, but it does compile.

Do you mind doing r+sr as well?
Attachment #179890 - Attachment is obsolete: true
Attachment #179917 - Flags: superreview?(darin)
(Assignee)

Comment 16

12 years ago
Created attachment 179919 [details] [diff] [review]
patch

I didn't have as many browser windows open as I thought, so tested now.
Attachment #179917 - Attachment is obsolete: true
Attachment #179919 - Flags: superreview?(darin)
(Assignee)

Updated

12 years ago
Attachment #179917 - Flags: superreview?(darin)

Comment 17

12 years ago
Comment on attachment 179919 [details] [diff] [review]
patch

r+sr=darin
Attachment #179919 - Flags: superreview?(darin)
Attachment #179919 - Flags: superreview+
Attachment #179919 - Flags: review+
(Assignee)

Comment 18

12 years ago
Comment on attachment 179919 [details] [diff] [review]
patch

This makes user-agent addition much easier for distributors and reduces the
chance that they'll make binary-incomptabile changes to the HTTP code.
Attachment #179919 - Flags: approval1.8b2?

Comment 19

12 years ago
Comment on attachment 179919 [details] [diff] [review]
patch

a=mkaply
Attachment #179919 - Flags: approval1.8b2? → approval1.8b2+
(Assignee)

Comment 20

12 years ago
So, actually, my calendar checkout was way out of date, since I wasn't pulling
it by default, and once I fixed that the calendar changes looked just like the
other app changes...
(Assignee)

Comment 21

12 years ago
Fix checked in to trunk, 2005-04-07 11:11 -0700.

I'll try to document this on
http://www.mozilla.org/build/revised-user-agent-strings.html shortly.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Apparently, this change broke Sunbird (at least on Windows XP). I filed bug
289786. See bug 289786 comment 0 and bug 289786 comment 1 for more information.

Should this bug be reopened or do we want to fix this in bug 289786?

Updated

12 years ago
Blocks: 289786

Comment 23

12 years ago
This has broken at least one well-known detection script
(http://www.webreference.com/tools/browser/javascript.html) - and will probably
break many more that make the same assumptions (looking at navigator.vendor for
the string "Firefox").

Would it be possible to still make navigator.vendor return "Firefox"?
(Assignee)

Comment 24

12 years ago
Firefox is not and never was a vendor.
(Assignee)

Comment 25

12 years ago
(And any sniffing script that distinguishes Firefox from other Geckos is
seriously broken anyway.)
not to mention that this checkin should not have any visible effects to web
developers...
(nevermind my last comment. instead, see comment 24)

Comment 28

12 years ago
(In reply to comment #25)
> (And any sniffing script that distinguishes Firefox from other Geckos is
> seriously broken anyway.)

Not exactly. There are some cases where it does not mean bad code. For instance, I have a version of IE Skin for Mozilla, Firefox and Netscape. All three are available on my web site. But if you click on, say, a link for the theme for Netscape using Firefox, you get an alert saying "This theme requires Netscape".   This prevents the user from getting a failed-to-install error. Besides, this does not stop the user from downloading them anyway.

bamm.gabriana.com/ieskin.html
Duplicate of this bug: 217407
You need to log in before you can comment on or make changes to this bug.