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

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect, P3)

Description

[Robert Strong (they/them - no direct email)]

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.

Comment 1

[Ralph Giles (:rillian)]

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.

Comment 2

[Ralph Giles (:rillian)]

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.

Comment 3

[Robert Strong (they/them - no direct email)]

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?

Comment 4

[Ralph Giles (:rillian)]

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?

Comment 5

[Robert Strong (they/them - no direct email)]

(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.

Comment 6

[Henri Sivonen (:hsivonen)]

(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?

Comment 7

[Ralph Giles (:rillian)]

If you want me to write a patch I'll see what I can do.

Comment 8

[Ralph Giles (:rillian)]

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 9

[Ralph Giles (:rillian)]

Attachment: Bug 1277359 - Add chrome-only navigator.cpuHasSSE2 api.

Make the host's support for SSE2 instructions available to
javascript so we can report that status in update pings.

Review commit: https://reviewboard.mozilla.org/r/84360/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84360/

Comment 10

[Ralph Giles (:rillian)]

Attachment: Report SSE2 instruction support on linux update pings.

We need to advertise this to the update server to avoid downloading
updates which won't run because of unsupported cpu features on
the recipient's hardware.

Review commit: https://reviewboard.mozilla.org/r/84362/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84362/

Comment 11

[Ralph Giles (:rillian)]

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.

Comment 12

[Robert Strong (they/them - no direct email)]

It is tested in
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/tests/unit_aus_update/urlConstruction.js

Comment 13

[Robert Strong (they/them - no direct email)]

I'll review the changes to the jsm after the supporting code is reviewed.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

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 ?

Comment 15

[Ralph Giles (:rillian)]

Comment on attachment 8799062 [details]
Bug 1277359 - Add chrome-only navigator.cpuHasSSE2 api.

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

Comment 16

[Ralph Giles (:rillian)]

Attachment: Bug 1277359 - Report SSE2 instruction support on linux update pings.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84362/diff/1-2/

Comment 17

[Ralph Giles (:rillian)]

Comment on attachment 8799062 [details]
Bug 1277359 - Add chrome-only navigator.cpuHasSSE2 api.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84360/diff/1-2/

Comment 18

[Ralph Giles (:rillian)]

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?

Comment 19

[Ralph Giles (:rillian)]

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`?

Comment 20

[Robert Strong (they/them - no direct email)]

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

Comment 21

[Robert Strong (they/them - no direct email)]

Ben, heads up regarding Linux adding whether SSE2 support is present to the update url

Comment 22

[[e-mail] (: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.

Comment 23

[Ralph Giles (:rillian)]

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 24

[Ralph Giles (:rillian)]

Comment on attachment 8799062 [details]
Bug 1277359 - Add chrome-only navigator.cpuHasSSE2 api.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84360/diff/2-3/

Comment 25

[Ralph Giles (:rillian)]

Comment on attachment 8802648 [details]
Bug 1277359 - Report SSE2 instruction support on linux update pings.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84362/diff/2-3/

Comment 26

[Pulsebot]

Pushed by [e-mail]:
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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4e8884c8a186
Fix syntax error introduced in 87fe724cfc90; r=me

Comment 28

[Robert Strong (they/them - no direct email)]

If urlConstruction.js fails please disable it on Linux and I'll fix it separately from this bug.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9ffa9dc9288e
https://hg.mozilla.org/mozilla-central/rev/87fe724cfc90
https://hg.mozilla.org/mozilla-central/rev/4e8884c8a186

Comment 30

[Boris Zbarsky [:bzbarsky]]

Why the empty "partial interface Navigator"?

Comment 31

[Ralph Giles (:rillian)]

(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.

Comment 32

[Henri Sivonen (:hsivonen)]

Is it practical to uplift this to earlier trains?

Comment 35

[Robert Strong (they/them - no direct email)]

Oops! Ignore comment #33 and comment #34. I thought this was bug 1311515.

Comment 36

[Ralph Giles (:rillian)]

We could uplift to 51 aurora. I think it's too late for 50 beta.

Comment 37

[Ralph Giles (:rillian)]

Attachment: 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.

Comment 38

[Gerry Chang [:gchang]]

Comment on attachment 8805698 [details] [diff] [review]
aurora 51 backport

Support update for linux. Take it in 51 aurora.

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/46f75e576477