Closed Bug 1072050 Opened 5 years ago Closed 5 years ago

Add pref for setting device identifier in UA string

Categories

(Core :: Networking, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: sicking, Assigned: ethan)

Details

Attachments

(1 file, 4 obsolete files)

On FirefoxOS we have a lot of partners that set the UA string to include a device identifier. As much as we'd like that to stop, it's not going to stop until we've actually solved websites needs for doing UA-string detection, and that still hasn't been solved.

So in the meantime we should add a pref specifically for this to ensure that it's done in a consistent and correct way.

Ken, could you find someone on your platform teams that could do this?
Flags: needinfo?(kchang)
Gerv, Lawrence, can you provide information about specifically where in the UA-string that the device-model-identifier should go? And if we have any requirements on its syntax, like "no semicolons" that should be enforced in the code.
(In reply to Jonas Sicking (:sicking) from comment #0)
> On FirefoxOS we have a lot of partners that set the UA string to include a
> device identifier. As much as we'd like that to stop, it's not going to stop
> until we've actually solved websites needs for doing UA-string detection,
> and that still hasn't been solved.
> 
> So in the meantime we should add a pref specifically for this to ensure that
> it's done in a consistent and correct way.
> 
> Ken, could you find someone on your platform teams that could do this?

Which UA string(Browser, MMS, RTSP, or..) are you talking about? But I think you are talking about all types of UA strings.
Flags: needinfo?(kchang)
Flags: needinfo?(kchang)
I was just talking about the value that we use when we add the "User-Agent" header to all HTTP requests.

I don't *think* that affexts MMS or RTSP, but I don't know those protocols well, so I could be wrong.
Ethan, I remember that you had fixed the similar bug for RTSP. Please help this bug. Thanks. Let's talk with PM about this.
Assignee: nobody → ettseng
Flags: needinfo?(kchang)
Jonas: we have a UA override whitelist; we should make sure that all partners who are asking for this are made aware that if it's just for their own use or for a small set of sites, that is a better solution (in terms of web compat) than overriding the UA everywhere.

The location and requirements for the device model, if it is to be added, are documented here:
https://wiki.mozilla.org/B2G/User_Agent/Device_Model_Inclusion_Requirements
The code could enforce the location in the string, plus requirement 1).

Gerv
And yes, this should be for web HTTP only, not for MMS, RTSP or anything else.

Gerv
(In reply to Ken Chang[:ken] from comment #4)
> Ethan, I remember that you had fixed the similar bug for RTSP. Please help
> this bug. Thanks. Let's talk with PM about this.

In bug 1026923, we changed RTSP code to directly reuse the HTTP UA string (from nsIHttpProtocolHandler).
Which means UA strings are the same in HTTP and RTSP.
(In reply to Gervase Markham [:gerv] from comment #5)
> Jonas: we have a UA override whitelist; we should make sure that all
> partners who are asking for this are made aware that if it's just for their
> own use or for a small set of sites, that is a better solution (in terms of
> web compat) than overriding the UA everywhere.
> The location and requirements for the device model, if it is to be added,
> are documented here:
> https://wiki.mozilla.org/B2G/User_Agent/Device_Model_Inclusion_Requirements
> The code could enforce the location in the string, plus requirement 1).
> Gerv

Thanks Gervase for this information.
I will try to add a preference for the UA string first.
Gerv: The request we get from most partners aren't of the form "Website X doesn't work without sticking a device identifier in the UA string", but rather "all other browsers do this and we're worried an unknown number of websites will not work unless we do this".

I dislike the device identifier in the UA string as much as you do. But after having talked to web developers I get the impression that removing it is starting at the wrong end. Just like removing the OS identifier or indeed the Gecko version identifier.

We first need to provide them with usable alternatives.
(In reply to Jonas Sicking (:sicking) from comment #9)
> Gerv: The request we get from most partners aren't of the form "Website X
> doesn't work without sticking a device identifier in the UA string", but
> rather "all other browsers do this and we're worried an unknown number of
> websites will not work unless we do this".

When we looked at this, our compat testing showed exactly the opposite - websites break when you start doing this. Particularly those who made specific accommodation for the standard FxOS UA string. Of course, the situation may be different now; I haven't tested recently. Karl is the man to ask for the latest info.

Gerv
Yes, I think there are many websites too that break. I suspect that that situation remains unchanged.
Attached patch bug-1072050-wip.patch (obsolete) — Splinter Review
A WIP patch.
Comment on attachment 8498615 [details] [diff] [review]
bug-1072050-wip.patch

Not sure if b2g.useragent.device_id is the right pref. Other user agent prefs are in general.useragent.*. And if the code is going in netwerk/protocol/http/nsHttpHandler.cpp , that implies genericity. But then, perhaps we want to indicate to people that other OSes and browser builds definitely shouldn't set it...

Gerv
(In reply to Gervase Markham [:gerv] from comment #13)
> Comment on attachment 8498615 [details] [diff] [review]
> bug-1072050-wip.patch
Gerv, thanks for your feedback. :)

> Not sure if b2g.useragent.device_id is the right pref. Other user agent
> prefs are in general.useragent.*.
I also noticed these preferences exist for packaged UA override list.
And I wasn't sure if it's adequate to add new preference here.
So, what about naming it as "general.useragent.device_id"?

> And if the code is going in
> netwerk/protocol/http/nsHttpHandler.cpp , that implies genericity. But then,
> perhaps we want to indicate to people that other OSes and browser builds
> definitely shouldn't set it...
Understood. Do you have suggestion of where to specify the device ID for customized UA?

Ethan
I think general.useragent.device_id is probably the right name.

Also, remember, we want to enforce the restrictions on its content in code (no semicolons, no spaces, etc.) from https://wiki.mozilla.org/B2G/User_Agent/Device_Model_Inclusion_Requirements . So please add that.

Gerv
Attached patch bug-1072050-wip.patch (obsolete) — Splinter Review
1. Name the pref as "general.useragent.device_id".
2. Enforce syntax restriction to Device ID.
Attachment #8498615 - Attachment is obsolete: true
Hi Gerv,

Thanks for your reminder. Restrictions were added in the patch.

In terms of genericity you mentioned in comment 13, the code of Device ID is wrapped in a
"#if defined(ANDROID) || defined(MOZ_B2G)" block, do you think that's enough to indicate this code 
is not generic?
Flags: needinfo?(gerv)
:ethan: I'm sure that's fine.

Gerv
Flags: needinfo?(gerv)
(In reply to Gervase Markham [:gerv] from comment #18)
> :ethan: I'm sure that's fine.
Thanks!
Could you help review the patch? Otherwise, do you know who can review it?
Status: NEW → ASSIGNED
I'm not the guy to review this patch. Check who reviewed previous UA string-editing patches. 

Gerv
(In reply to Ethan Tseng [:ethan] from comment #17)
> Hi Gerv,
> 
> Thanks for your reminder. Restrictions were added in the patch.
> 
> In terms of genericity you mentioned in comment 13, the code of Device ID is
> wrapped in a
> "#if defined(ANDROID) || defined(MOZ_B2G)" block, do you think that's enough
> to indicate this code 
> is not generic?

That will change Fennec's UA. Do we want that? Also, MOZ_B2G is true on b2g desktop builds, where this does not really make sense. I think you just want MOZ_WIDGET_GONK.
(In reply to Fabrice Desré [:fabrice] from comment #21)
> That will change Fennec's UA. Do we want that? 

Surely only if someone sets the pref?

If I'm wrong about that, then no, we certainly don't want that to happen.

Gerv
Comment on attachment 8499984 [details] [diff] [review]
bug-1072050-wip.patch

Hi Kyle,
Could you help to review my patch?
Attachment #8499984 - Flags: review?(khuey)
Comment on attachment 8499984 [details] [diff] [review]
bug-1072050-wip.patch

Cancel the review because of Fabrice's suggestion in comment 21.
I'll update the patch soon.
Attachment #8499984 - Flags: review?(khuey)
Attached patch Add device ID in UA string (obsolete) — Splinter Review
Hi Kyle,
Could you help to review this patch?
Attachment #8499984 - Attachment is obsolete: true
Attachment #8504127 - Flags: review?(khuey)
Comment on attachment 8504127 [details] [diff] [review]
Add device ID in UA string

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

I assume you've verified that the user agent is unchanged with the default value of the pref.

sr? to sicking to make sure this is what he wanted.

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +710,5 @@
>  
> +#if defined(MOZ_WIDGET_GONK)
> +    // Device model identifier should be a simple token, which can be composed
> +    // of letters, numbers, hyphen ("-") and dot (".").
> +    // Occurrence of any other character causes the identifier invalid.

Any other character means the identifier is invalid and ignored.

@@ +725,5 @@
> +            }
> +        }
> +        if (valid) {
> +            mDeviceModelId = deviceId;
> +        }

If the device model id is ignored we should print something to logcat.
Attachment #8504127 - Flags: superreview?(jonas)
Attachment #8504127 - Flags: review?(khuey)
Attachment #8504127 - Flags: review+
Attachment #8504127 - Flags: superreview?(jonas) → superreview+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #26)
> I assume you've verified that the user agent is unchanged with the default
> value of the pref.
Yes. Confirmed. :)
 
> > +        if (valid) {
> > +            mDeviceModelId = deviceId;
> > +        }
> If the device model id is ignored we should print something to logcat.
Sure. I'll add it.
Attached patch Add device ID in UA string (obsolete) — Splinter Review
Addressed issues in the review comment.
Attachment #8504127 - Attachment is obsolete: true
Refresh comment "r=khuey, sr=sicking".
Attachment #8504540 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/fa03ad915ea5
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.