UserAgent cannot be changed for specific websites in workers

RESOLVED FIXED in Firefox 33

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: baku, Assigned: baku)

Tracking

Trunk
mozilla35
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox33 fixed, firefox34 fixed, firefox35 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

Currently navigator.userAgent value can be changed using nsISiteSpecificUserAgent and properties as such as general.useragent.site_specific_overrides and general.useragent.override.*.

This feature should be supported in workers too.
Posted patch useragent.patch (obsolete) — Splinter Review
It turned out that we ignored general.useragent.override in the main-thread too.
Just because we can have overridden values per site, we cannot use the standard approach to retrieve prefs in workers.
Attachment #8491691 - Flags: review?(khuey)
Comment on attachment 8491691 [details] [diff] [review]
useragent.patch

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

I assume that you've tested that this behaves sanely in a worker created from a JSM/JS component?

::: dom/base/Navigator.cpp
@@ +2550,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  if (!nsContentUtils::IsCallerChrome()) {
> +    const nsAdoptingString& override =
> +      mozilla::Preferences::GetString("general.useragent.override");

Is a fully qualified mozilla:: really necessary here?
Comment on attachment 8491691 [details] [diff] [review]
useragent.patch

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

::: dom/workers/Navigator.cpp
@@ +366,5 @@
> +
> +} // anonymous namespace
> +
> +void
> +WorkerNavigator::GetUserAgent(nsString& aUserAgent) const

We should cache here, rather than go to the main thread every time.
> We should cache here, rather than go to the main thread every time.

We don't need to do that because the attribute is marked as [Cached] in the WebIDL. The bindings do the magic.

I'll write a test for worker in JSM.
Posted patch id.patch (obsolete) — Splinter Review
bz, can you take a look at this change?
I need language to be cached and to have the ClearCachedLanguageValue() method.
Attachment #8491691 - Attachment is obsolete: true
Attachment #8493621 - Flags: review?(bzbarsky)
Posted patch id2.patch (obsolete) — Splinter Review
khuey, a quick review on this interdiff? A test was failing with the previous patch.
Attachment #8493729 - Flags: review?(khuey)
Comment on attachment 8493621 [details] [diff] [review]
id.patch

r=me especially if you add a comment to the IDL about why it's cached.
Attachment #8493621 - Flags: review?(bzbarsky) → review+
Last push to try before landing:

https://tbpl.mozilla.org/?tree=Try&rev=6f38ed97475d
Posted patch useragent.patch (obsolete) — Splinter Review
Attachment #8493621 - Attachment is obsolete: true
Attachment #8493729 - Attachment is obsolete: true
Attachment #8493730 - Attachment is obsolete: true
Keywords: checkin-needed
sorry had to backout this change for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=48754884&tree=Mozilla-Inbound
it turned out that on b2g desktop the platform can be an empty string.

https://tbpl.mozilla.org/?tree=Try&rev=63fd93df5ddb
Attachment #8494257 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4f72287c59ca
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
We should uplift this patch because it's similar to bug 1062920
Flags: needinfo?(ryanvm)
Happy to do so once it has approval ;)
Flags: needinfo?(ryanvm) → needinfo?(amarchesini)
Comment on attachment 8494445 [details] [diff] [review]
useragent.patch

Approval Request Comment
[Feature/regressing bug #]: 925847
[User impact if declined]: The overridden prefs for userAgent will be ignored in workers.
[Describe test coverage new/current, TBPL]: https://tbpl.mozilla.org/?tree=Try&rev=63fd93df5ddb
[Risks and why]: The patch is not complex and it has good mochitests.
[String/UUID change made/needed]: none
Attachment #8494445 - Flags: approval-mozilla-beta?
Attachment #8494445 - Flags: approval-mozilla-aurora?
Flags: needinfo?(amarchesini)
Attachment #8494445 - Flags: approval-mozilla-beta?
Attachment #8494445 - Flags: approval-mozilla-beta+
Attachment #8494445 - Flags: approval-mozilla-aurora?
Attachment #8494445 - Flags: approval-mozilla-aurora+
Depends on: 1072947
> r=me especially if you add a comment to the IDL about why it's cached.

This comment never got added.  Or rather, a comment got added, but it doesn't explain the _why_.
Flags: needinfo?(amarchesini)
Blocks: 1224944
Flags: needinfo?(amarchesini)
You need to log in before you can comment on or make changes to this bug.