Closed Bug 274928 Opened 20 years ago Closed 19 years ago

Firefox abuses vendor portions of user agent string

Categories

(Core :: Networking: HTTP, defect)

Product:
Core
Component:
Networking: HTTP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.8beta2

People

(Reporter: caillon, Assigned: dbaron)

References

Details

(Whiteboard: [patch])

Attachments

(1 file, 4 obsolete files)

patch
22.87 KB, patch
darin.moz
: review+
darin.moz
: superreview+
mkaply
: approval1.8b2+
Details | Diff | Splinter Review 
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.
Attached patch Patch (obsolete) — — Splinter Review 
This is the patch I added to our firefox RPMs.
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 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.
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
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: 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
Attached patch patch (obsolete) — — Splinter Review 
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
Attached patch patch (obsolete) — — Splinter Review 
oops, wrong patch
Attachment #179889 - Attachment is obsolete: true
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 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+
Did some version of Fedora Core really ship with those changes to
nsIHttpProtocolHandler?  /me hopes not...
(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".
> 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 :-(
Attached patch patch (obsolete) — — Splinter Review 
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)
Attached patch patch — — Splinter Review 
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)
Comment on attachment 179919 [details] [diff] [review]
patch

r+sr=darin
Attachment #179919 - Flags: superreview?(darin)
Attachment #179919 - Flags: superreview+
Attachment #179919 - Flags: review+
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 on attachment 179919 [details] [diff] [review]
patch

a=mkaply
Attachment #179919 - Flags: approval1.8b2? → approval1.8b2+
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...
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
Closed: 19 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?
Blocks: 289786
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"?
Firefox is not and never was a vendor.
(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)
(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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: