[Linux] Add CPU instruction set detection to UpdateUtils.jsm

RESOLVED FIXED in Firefox 51

Status

()

Toolkit
Application Update
P3
normal
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: rstrong, Assigned: rillian)

Tracking

unspecified
mozilla52
Unspecified
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed, firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

In bug 1271761 CPU instruction set detection on Windows was added to the app update url and we want the same for Linux. This will need to be added to
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/UpdateUtils.jsm

After it is added it will automatically be used by application update.

Updated

2 years ago
Blocks: 1271762
(Assignee)

Comment 1

2 years ago
We finally saw a crash from this on linux32. We should deploy the same update block for that OS as well.

We have many independent cpuid wrappers in the tree, but the most generic is mozilla::supports_sse2() from mozilla/SSE.h. Seems like calling that would solve the problem on all x86 platforms.
Blocks: 1291650
(Assignee)

Comment 2

2 years ago
Robert, what should we do to get this deployed? It's a very small number by crash reports, but breaking people's Firefox isn't nice.
Flags: needinfo?(robert.strong.bugs)
Ralph, it would need to be added to the following file
https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/UpdateUtils.jsm

Would you be interested in adding it?
Flags: needinfo?(robert.strong.bugs) → needinfo?(giles)
(Assignee)

Comment 4

2 years ago
I don't mind doing it, but I'm unconvinced it's worthwhile. The rust dependency on SSE2 cpus shipped for linux32 in Firefox 48 alongside win32. At best we could uplift the code to beta and block updates starting with 49. What do you think?
Flags: needinfo?(giles) → needinfo?(robert.strong.bugs)
(Assignee)

Updated

2 years ago
Blocks: 1298442
(In reply to Ralph Giles (:rillian) needinfo me from comment #4)
> I don't mind doing it, but I'm unconvinced it's worthwhile. The rust
> dependency on SSE2 cpus shipped for linux32 in Firefox 48 alongside win32.
> At best we could uplift the code to beta and block updates starting with 49.
> What do you think?
I think it would be a good thing. In the Windows case it sends the highest instruction set supported so it is possible to discontinue support of SSE2 if / when that changes.
Flags: needinfo?(robert.strong.bugs)
Priority: -- → P3
(Assignee)

Updated

2 years ago
Blocks: 1274196
(In reply to Ralph Giles (:rillian) needinfo me from comment #4)
> I don't mind doing it, but I'm unconvinced it's worthwhile.

Now that this is marked as blocking bug 1274196, are you planning on writing the patch? If not, how do we get forward motion on this?
Flags: needinfo?(giles)
(Assignee)

Comment 7

2 years ago
If you want me to write a patch I'll see what I can do.
Assignee: nobody → giles
Flags: needinfo?(giles)
Blocks: 1308167
(Assignee)

Comment 8

2 years ago
Sorry, I didn't get to this this week, and I'm away all of next week. Feel free to steal this bug in the meantime.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
This is the WIP I have so far, very rough. In particular it reports only SSE2 on linux; it would probably be cleaner to replicate the full option check from UpdateUtils.jsm in the c++ implementation and use that for both archs.

Robert, how did you test this? I don't know how to trigger the update ping other than leaving it running under pcap and waiting.
Flags: needinfo?(robert.strong.bugs)
Attachment #8799063 - Attachment is patch: true
Attachment #8799063 - Attachment mime type: text/x-review-board-request → text/plain
Attachment #8799063 - Attachment is patch: false
I'll review the changes to the jsm after the supporting code is reviewed.

Comment 14

2 years ago
mozreview-review
Comment on attachment 8799062 [details]
Bug 1277359 - Add chrome-only navigator.cpuHasSSE2 api.

https://reviewboard.mozilla.org/r/84360/#review83186

comments fixed, r+

::: dom/webidl/Navigator.webidl:446
(Diff revision 1)
>    readonly attribute unsigned long long hardwareConcurrency;
>  };
> +
> +partial interface Navigator {
> +  [ChromeOnly]
> +  readonly attribute boolean cpu_sse2;

Just put the attribute to existing partial interface (remember to keep the ChromeOnly annotation)
// nsIDOMNavigator
partial interface Navigator {

using _ in attribute names in unusual.
Perhaps sse2CPU ?
Attachment #8799062 - Flags: review?(bugs) → review+
(Assignee)

Comment 15

2 years ago
mozreview-review
Comment on attachment 8799062 [details]
Bug 1277359 - Add chrome-only navigator.cpuHasSSE2 api.

https://reviewboard.mozilla.org/r/84360/#review86050
Attachment #8799063 - Flags: review?(robert.strong.bugs) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Thanks. Carrying-forward r=smaug,rstrong for the api name change.

Robert, how can I verify this is sending the correct signal in the update request?
Flags: needinfo?(robert.strong.bugs)
(Assignee)

Comment 19

2 years ago
mozreview-review-reply
Comment on attachment 8799062 [details]
Bug 1277359 - Add chrome-only navigator.cpuHasSSE2 api.

https://reviewboard.mozilla.org/r/84360/#review83186

> Just put the attribute to existing partial interface (remember to keep the ChromeOnly annotation)
> // nsIDOMNavigator
> partial interface Navigator {
> 
> using _ in attribute names in unusual.
> Perhaps sse2CPU ?

Thanks. I'd like it to start with `cpu` because we might add more later and I'd like them to sort. How about `cpuHasSSE2`?
The url can be tested in

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #12)
> It is tested in
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/tests/
> unit_aus_update/urlConstruction.js
Flags: needinfo?(robert.strong.bugs)
Ben, heads up regarding Linux adding whether SSE2 support is present to the update url
Flags: needinfo?(bhearsum)
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #21)
> Ben, heads up regarding Linux adding whether SSE2 support is present to the
> update url

Thanks! No changes needed on the server for this - we added support for all platforms when with our earlier work.
Flags: needinfo?(bhearsum)
(Assignee)

Comment 23

2 years ago
mozreview-review
Comment on attachment 8802648 [details]
Bug 1277359 - Report SSE2 instruction support on linux update pings.

https://reviewboard.mozilla.org/r/84362/#review86764

::: toolkit/modules/UpdateUtils.jsm:192
(Diff revision 2)
>      return instructionSet;
>    }
>  
> +  if (AppConstants == "linux") {
> +    let instructionSet = "unknown";
> +    if navigator.cpu_sse2 {

forgot to propagate the rename here.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 26

2 years ago
Pushed by rgiles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9ffa9dc9288e
Add chrome-only navigator.cpuHasSSE2 api. r=smaug
https://hg.mozilla.org/integration/autoland/rev/87fe724cfc90
Report SSE2 instruction support on linux update pings. r=rstrong

Comment 27

2 years ago
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4e8884c8a186
Fix syntax error introduced in 87fe724cfc90; r=me
If urlConstruction.js fails please disable it on Linux and I'll fix it separately from this bug.

Comment 29

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9ffa9dc9288e
https://hg.mozilla.org/mozilla-central/rev/87fe724cfc90
https://hg.mozilla.org/mozilla-central/rev/4e8884c8a186
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Why the empty "partial interface Navigator"?
Flags: needinfo?(giles)

Updated

2 years ago
Depends on: 1312345
(Assignee)

Updated

2 years ago
Depends on: 1312559
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #30)
> Why the empty "partial interface Navigator"?

Thanks for pointing that out. Hopefully fixed in bug 1312559.
Flags: needinfo?(giles)
Is it practical to uplift this to earlier trains?
Flags: needinfo?(giles)
Comment hidden (spam)
Comment hidden (spam)
Oops! Ignore comment #33 and comment #34. I thought this was bug 1311515.
We could uplift to 51 aurora. I think it's too late for 50 beta.
Flags: needinfo?(giles)
Created attachment 8805698 [details] [diff] [review]
aurora 51 backport

Patches apply cleanly to aurora, but there are some follow-up commits, so here's a squashed version. r=me

Approval Request Comment
[Feature/regressing bug #]: Bug 1308167

[User impact if declined]: Dropping support for non-SSE2 linux32 machines would have to wait for another release.
 
[Describe test coverage new/current, TreeHerder]: Landed on m-c last week.

[Risks and why]: I think the risk is acceptable this late in the Aurora cycle. The change extends update ping code we've already deployed for win32 to linux32, which is a small part of our user base. We should be able to mitigate any incorrect behaviour on the update server.

[String/UUID change made/needed]: None.
Attachment #8805698 - Flags: review+
Attachment #8805698 - Flags: approval-mozilla-aurora?
status-firefox51: --- → affected
Comment on attachment 8805698 [details] [diff] [review]
aurora 51 backport

Support update for linux. Take it in 51 aurora.
Attachment #8805698 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 39

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/46f75e576477
status-firefox51: affected → fixed
Depends on: 1415336
You need to log in before you can comment on or make changes to this bug.