323657 - User-Agent string should identify Mac universal binaries

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Mark Mentovai]

If - or, in my opinion, when - universal binaries of Mozilla products ship, it will be useful to have them identify themselves in the User-Agent string.  This will allow browser sniffers to determine if the running browser is universal or not, and tailor the options for the user accordingly.  For example, the browser sniffer at www.mozilla.com will most likely want to offer a universal binary download to users already using a universal browser, but can offer architecture-specific downloads to users with thin versions.

Comment 1

[Mark Mentovai]

Attachment: Determine universal status at runtime

Initially, we had planned on hardcoding the universal status of the app at build time, probably by having the build process check an environment variable (MOZ_MAC_UNIVERSAL) that would be set if the bits were destined to be merged into a universal binary.  After much thought, I've decided that this is the wrong idea.

- It prevents architecture-specific bits from being merged unless forethought is given to setting MOZ_MAC_UNIVERSAL; when MOZ_MAC_UNIVERSAL is set, those bits wouldn't be independently usable for thin single-architecture packages (without rebuilding necko and relinking the app).
- It's probably the wrong thing to do in the XULRunner world, where presumably we don't care about putting the universal status of XR in the user-agent string, but we do care about the main app itself.

This patch tailors the user-agent string at runtime based on whether the main executable is fat and contains PPC and x86 code.  An immediate benefit of this is that existing PPC and x86 builds can be merged with no additional considerations* (other than a similar change needed to identify universalness for ASU, bug 323328,) and the user-agent string will be correct.

* Merged PPC and x86 bits should have the same build ID.  Either verify that the build ID is the same, or set MOZ_BUILD_DATE = yyyymmddhh in the environment before building to ensure parity.

Comment 2

[Benjamin Smedberg]

Comment on attachment 208657 [details] [diff] [review]
Determine universal status at runtime

Darin should review this: the useragent has some unique requirements in terms of web tracking and such; it's not clear to me that we ought to be altering the string that appears as "the OS"... rather we should add a modifier separately. But he knows the details and I don't.

On the trunk I would ideally attach this to nsIXULRuntime and move that interface up into tier 9, but since we're pressed for time and can't change nsIXULRuntime on the branch let's get something coded from nsHttpHandler if that's the only expedient option.

Comment 3

[Darin Fisher]

I'd encourage you to go for a shorter UA string if possible.  " Universal" seems a bit long.  Is that what Safari is sending?

Comment 4

[Mark Mentovai]

I don't think Safari indicates its universal status, but it's Apple and they're free to identify it some other way (like by build/version number).  I'd happily go with " Uni" or even " U".

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This shouldn't be something that's easy to confuse with the " U; " (meaning strong encryption, short for USA) that's already in your UA string -- some banks, etc., may check for that in various not-very-robust ways.

Comment 6

[Darin Fisher]

How about moving the IsUniversalBinary function into mozilla/xpcom/ somewhere?  It seems like we will need to call it from mozilla/toolkit/xre/nsAppRunner.cpp as well, so it should live someplace common.

Comment 7

[Mark Mentovai]

David, let's say we go with " Uni".  Is it safe to append to the OS/CPU identifier (currently "PPC Mac OS X Mach-O" or "Intel Mac OS X") or can you pinpoint a better spot to put it? 

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Appending to OSCPU seems fine, as long as there's no semicolon, since I'd be afraid of adding the substring "; U".

Comment 9

[Eric Albert]

Is identifying the browser as being universal actually useful?  I don't think adding it to the User-Agent string has much value.  Safari certainly doesn't do it, even though both universal and non-universal versions of Safari exist.

Users will care if their browser is native for their current system, not if it's universal.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Installs on disks that could be shared are a reason for wanting something like this; I have no idea how common such things are.  (A Web site offering a plugin could then provide a universal binary version of the plugin.)

Comment 11

[Eric Albert]

(In reply to comment #10)
> Installs on disks that could be shared are a reason for wanting something like
> this; I have no idea how common such things are.  (A Web site offering a plugin
> could then provide a universal binary version of the plugin.)

I guess I should phrase what I said differently.  Once a universal version of Firefox is released, presumably all subsequent versions will be universal until the Mozilla Foundation decides to drop PowerPC support.  Similarly, once plug-in vendors release universal versions, I doubt they'll continue to provide non-universal versions.  Building and qualifying two separate distributions of the same binary is too complicated for most companies.

Another way to look at this is the example given in the first comment in this bug.  The sniffer at www.mozilla.com probably wouldn't want to differentiate between users with universal and non-universal versions.  Instead, you'd want to offer the universal build to everyone.  Make that the One True Download just as the PowerPC version is today and the entire problem goes away.

Comment 12

[Mark Mentovai]

Eric, we're not sure we're going to need to do this.  It will only be useful if there continue to be architecture-specific official released versions in addition to universals.  That decision hasn't been made yet.  I want to have this patch ready just in case.

The mozilla bouncer script and similar UA detectors for sites that offer plugins are exactly why we'd want this in a world where any given release would be available in all of (ppc, x86, uni).

Comment 13

[Mark Mentovai]

Attachment: Make isUniversalBinary part of an XPCOM service

Create a new xpcom service, and implement in nsMacUtils.  isUniversalBinary is a read-only attribute.  GetIsUniversalBinary is modified from the version in attachment 208657 [details] [diff] [review] to cache the result in a static.

Comment 14

[Mark Mentovai]

Attachment: Append " Uni" to nsHttpHandler::mOscpu when universal

This depends on the previous patch, and calls into the new service to determine whether the application is universal or not when building the UA string.

I'm soliciting review on this, as long as everyone understands that it may not be necessary.

Comment 15

[Benjamin Smedberg]

Comment on attachment 209377 [details] [diff] [review]
Make isUniversalBinary part of an XPCOM service

Please move the _CLASSNAME _CID _CONTRACTID defines out of the .idl file into nsMacUtils.h

Comment 16

[Mark Mentovai]

Attachment: Make isUniversalBinary part of an XPCOM service (v2)

Comment 17

[Mark Mentovai]

Attachment: Append " Uni" to nsHttpHandler::mOscpu when universal (v3)

Comment 18

[Darin Fisher]

Comment on attachment 209472 [details] [diff] [review]
Make isUniversalBinary part of an XPCOM service (v2)

nsMacUtils.h should have include guards (i.e., #ifndef FOO, #define FOO, #endif).

Comment 19

[Benjamin Smedberg]

Comment on attachment 209472 [details] [diff] [review]
Make isUniversalBinary part of an XPCOM service (v2)

r=me with #includeguards

Comment 20

[Mark Mentovai]

Attachment: Make isUniversalBinary part of an XPCOM service (v3)

(carrying forward bsmedberg's r+)

Comment 21

[Mark Mentovai]

At this point, it's looking increasingly like it won't be necessary to add " Uni" to the UA string, since releases from this point on will most likely be universal-only until either PPC becomes unsupported or the universal build falls out of fashon in favor of independent non-universal builds.  The patch to add the service with the utility function is still needed for bug 323355 and anything else that might need to determine universalness of the app.

I think that it might be useful to indicate in the UA (and only the UA, nowhere else) when the app is running under Rosetta.  For the time being, at least, people will probably run under Rosetta for a variety of reasons, the most significant of which is that not all plugins have been updated yet.  At the same time, I don't see the need to hunt down behavioral differences that only exist when running under Rosetta (yes, they exist).  Exposing this in the UA makes it easier to identify bogus bug reports.

Comment 22

[Eric Albert]

I don't have any objection to a UA note that the app is running translated.  I'm not sure how useful it'll be without adoption in other browsers, but you never know.

By the way, it'd be great if you'd file bug reports about any behavioral differences you see aside from performance.  They aren't something the Mozilla folks should care about, but Apple should care.

Comment 23

[Mark Mentovai]

Eric, so far the only one I came across was that an attempt to load Java crashes under Rosetta.  Maybe it shouldn't crash and maybe that's our problem, but considering that Java is advertised to not work under Rosetta, this is at least an expected difference, and I doubt you guys care about this one much more than I do.  (Although it would be nice if I could get a better stack to see if it's a crash that could be easily avoided.)

I'll let you know for sure if I find any UNexpected behavior differences.

Comment 24

[Eric Albert]

Hmm...that sounds like a Firefox bug of some sort -- maybe a failure to check return codes or something like that.  Java applets in Safari simply don't run when Safari's running translated.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 209473 [details] [diff] [review]
Append " Uni" to nsHttpHandler::mOscpu when universal (v3)

>-#elif defined (XP_MACOSX) && defined(__ppc__)
>+#elif defined (XP_MACOSX) && (defined(__ppc__)||defined(__i386__))

spaces around "||", please.  (I won't make you put them between "defined" and "(", though, even though that is local style, but feel free to do so.)

>+    mOscpu.SetCapacity(24);

I don't like magic numbers.  How about:

mOscpu.SetCapacity(sizeof("PPC Mac OS X Mach-O") - 1);

or even better:

#ifdef __ppc__
const char kMacOSCPU[] = "PPC Mac OS X Mach-O";
#else
const char kMacOSCPU[] = "Intel Mac OS X";
#endif
const char kMacUniversal[] = " Uni";
mOscpu.SetCapacity(sizeof(kMacOSCPU) + sizeof(kMacUniversal) - 2);
mOscpu.AssignLiteral(kMacOSCPU);

and then later (if universal)

mOscpu.AppendLiteral(kMacUniversal);

Comment 26

[Mark Mentovai]

Attachment: As checked in

Includes Makefile diff.

This was checked in to support bug 323328.  Looks like " Uni" won't be added to the UA.  Requesting blocking in anticipation of bug 323328 being ready for the branch.

Comment 27

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I think this may have broken the build on XULRunner: Columbia.

Comment 28

[Robert Accettura [:raccettura]]

Ok, I'm going to update the reporter tool to recognize mac universl binaries.
navigator.platform
navigator.oscpu

Someone want to help me out with the possible values for MacPPC, Universal Binaries, and x86 builds (assuming we have, or will have them)?

Comment 29

[Mark Mentovai]

Yes, it did.  There's another class of the same name (nsMacUtils) in xpfe/bootstrap/appleevents.  This one needs to be renamed.  nsIMacUtils can remain the same, I suggest naming the implementation nsMacUtilsImpl.  Soliciting review for that change.

Comment 30

[Mark Mentovai]

Robert, right now, it's looking like these values aren't going to help you determine universalness at all, the plan being to release universal-only.  The values will indicate the host CPU:

navigator.oscpu = "PPC Mac OS X Mach-O", "Intel Mac OS X"
navigator.platform = "MacPPC", "MacIntel"

If there's a major shift and attachment 209473 [details] [diff] [review] is taken, then the oscpu values will have " Uni" appended to them to signify universal builds.

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #29)
> Yes, it did.  There's another class of the same name (nsMacUtils) in
> xpfe/bootstrap/appleevents.  This one needs to be renamed.  nsIMacUtils can
> remain the same, I suggest naming the implementation nsMacUtilsImpl. 
> Soliciting review for that change.

r+sr=dbaron, although you generally don't need (advance) review for simple changes to fix bustage

Comment 32

[Robert Accettura [:raccettura]]

Mark: just what I wanted.

Just my $0.02 - IMHO browser shouldn't tell if it's universl or not... it's none of the website's business ;-).  It should clearly state if it's running on intel, or ppc though, since there is a need for that.  I don't think there's a precedent for displaying the build methodology of a client.  Only the platform.

Comment 33

[Mark Mentovai]

Attachment: Regenerated patch

Bustage fix checked in.  This patch was regenerated by date and contains the new class name, suitable for branch use.

Comment 34

[Daniel Veditz [:dveditz]]

Comment on attachment 209473 [details] [diff] [review]
Append " Uni" to nsHttpHandler::mOscpu when universal (v3)

approved for 180 branch, a=dveditz for drivers

Comment 35

[Daniel Veditz [:dveditz]]

Comment on attachment 212342 [details] [diff] [review]
Regenerated patch

approved for 180 branch, a=dveditz for drivers

Comment 36

[Mark Mentovai]

"Regenerated patch" of "Make isUniversalBinary part of an XPCOM service" checked in on 1_8 and 1_8_0.

"Append " Uni" to nsHttpHandler::mOscpu when universal" will NOT be checked in.

Comment 37

[Dave Liebreich [:davel]]

Mark, Josh, where can QA get builds to verify this fix?

Comment 38

[Mark Mentovai]

For the moment, unofficial test builds come out hourly-ish at:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/maya-Test-1.8.0-Uni/

Note that there's not much verifiable here from a user's perspective.  We didn't change the UA string at all, so it still says "PPC Mac OS X Mach-O" or "Intel Mac OS X".  The only place that this change should be at all perceptible is in the update service, which when universal, claims itself as "Darwin_Universal-gcc3", but that's bug 323328.