1415336 - Linux CPU instruction detection code doesn't work correctly

Status: RESOLVED FIXED

(Toolkit :: Application Update, enhancement)

Description

[Mark Banner (:standard8)]

The code that was added in bug 1277359 has various issues and it means that on Linux, the instruction set will always be reported as "NA" rather than "unknown" or "SSE2".

I picked this up via lint whilst poking starting to look at improving our detection of globals for jsms (bug 1369722).

http://searchfox.org/mozilla-central/rev/ed212c79cfe86357e9a5740082b9364e7f6e526f/toolkit/modules/UpdateUtils.jsm#211-217

```
  if (AppConstants == "linux") {
    let instructionSet = "unknown";
    if (navigator.cpuHasSSE2) {
      instructionSet = "SSE2";
    }
    return instructionSet;
  }
```

AppConstants is an object - the if should be checking AppConstants.platform.

Additionally, navigator is not available in jsm files, so that check would fail if it managed to get to it.

Comment 1

[Boris Zbarsky [:bzbarsky]]

We could put cpuHasSSE2 on ChromeUtils or ThreadSafeChromeUtils or something instead.

Comment 2

[Ralph Giles (:rillian)]

Oops. I'm not a js dev and will need some help fixing this.

Does this mean we've been blocking updates to linux32 users for the past two years?

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Does this mean we've been blocking updates to linux32 users for the past two years?

Well, we didn't land bug 1277359 until less than a year ago...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix Linux32 SSE2 detection in UpdateUtils to actually work

This should probably fix things, but I have no idea how to test it...

Comment 5

[Mark Banner (:standard8)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #4)
> This should probably fix things, but I have no idea how to test it...

Maybe QA have some capability to test it?

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Not sure offhand what hardware SV has available for testing. Rares?

Comment 7

[RaresB]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #6)
> Not sure offhand what hardware SV has available for testing. Rares?

Unfortunately we don't have such hardware in SV. SSE2 was found on Pentium 4 and those are much too old for our infrastructure. :(

Comment 8

[Mark Banner (:standard8)]

bz: not sure what we can do here...

Comment 9

[Boris Zbarsky [:bzbarsky]]

The obvious options are:

1)  Keep shipping the known-broken code.
2)  Take this fix without testing, like we took the original code, and hope.
3)  Keep trying to find some test hardware.
4)  Both 2 and 3.

Comment 10

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

Attachment: patch rev1

Telemetry gets this data from system-info and I verified this works on Win 10, OS X, and Ubuntu 14.04.5 desktop i386.

We should be able to deploy a system add-on to fix this on Linux for earlier Firefox versions.

Ben, do you have a problem with the CPU value as shown below:
SSE4_2
SSE4_1
SSE4A
SSSE3
SSE3
SSE2
SSE
MMX

I'd like to future proof this hence why SSE4_2, SSE4_1, and SSE4A are possible values.

Comment 11

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

Comment on attachment 8932690 [details] [diff] [review]
patch rev1

Changed the test to fail if it doesn't get a value and pushed it to try.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ca88b2f24690064bf1358203bc9310b6eff1f4b

Comment 12

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

I'll take a look at telemetry for 52.x tomorrow to see if it has reports for max SSE2 instruction set CPU's (it reports all supported instruction sets so SSE3, etc. will also contain SSE2).

Comment 13

[Boris Zbarsky [:bzbarsky]]

With that patch, navigator.cpuHasSSE2 can be removed, right?

Comment 14

[[e-mail] (:bhearsum)]

Comment on attachment 8932690 [details] [diff] [review]
patch rev1

No issues here, thanks for checking!

Comment 15

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

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #13)
> With that patch, navigator.cpuHasSSE2 can be removed, right?
Correct

Comment 16

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

Using the 1% sample of 52.0.2 from the longitudinal datasource restricted to only Linux I see the following

ARMv7:     9
SSE2 : 2,338
SSE  :     2
MMX  :     0

Comment 17

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

Attachment: patch rev2

Try run
https://treeherder.mozilla.org/#/jobs?repo=try&revision=50c4258ce2ba88afe84820bab498d1d57095fd76

I had to include arm for the tests to pass.

Comment 18

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8933166 [details] [diff] [review]
patch rev2

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

r=me, with one probably unimportant question:

::: toolkit/modules/tests/xpcshell/test_UpdateUtils_url.js
@@ +146,3 @@
>    }
>  
> +  return "error";

I doubt it matters, but this got changed from "unknown" when the ARM properties were added, and I can't see why?

Comment 19

[Julien Cristau [:jcristau]]

Is there a reason to believe testing on a vm emulating pentium2 or some other old cpu wouldn't work?

Comment 20

[Julien Cristau [:jcristau]]

I just tried firefox 52.0.2 on a pentium3 vm.  update log seems to confirm "NA" is reported:

*** AUS:SVC Checker:onLoad - request completed downloading document
*** AUS:SVC Checker:getUpdateURL - update URL: https://aus5.mozilla.org/update/6/Firefox/52.0.2/20170323105023/Linux_x86-gcc3/en-US/release/Linux%203.16.0-4-686-pae%20(GTK%203.14.5%2Clibpulse%205.0.0)/NA/default/default/update.xml
*** AUS:SVC Checker:onLoad - number of updates available: 1
*** AUS:SVC UpdateManager:_loadXMLFileIntoArray: XML file does not exist
*** AUS:SVC getCanApplyUpdates - testing write access /home/test/firefox/update.test
*** AUS:SVC getCanApplyUpdates - able to apply updates
*** AUS:SVC UpdateService:_selectAndInstallUpdate - prompting because silent install is disabled

And, despite the lack of SSE2, I'm being offered an update to 57.0 or 57.0.1.  Which presumably won't work?

Incidentally I see this in the browser console:

1512035651603	Toolkit.Telemetry	ERROR	TelemetrySend::sendPersistedPings - failed to send ping 8b63ac66-faa5-401e-94f0-2ebacd82b4a2: SyntaxError: The URI is malformed. Stack trace: open()@resource://gre/modules/ServiceRequest.jsm:42 < TelemetrySendImpl._doPing()@resource://gre/modules/TelemetrySend.jsm:912 < TelemetrySendImpl._sendPersistedPings</<()@resource://gre/modules/TelemetrySend.jsm:828 < Handler.prototype.process()@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:932 < this.PromiseWalker.walkerLoop()@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813 < this.PromiseWalker.scheduleWalkerLoop/<()@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747

which seems kind of concerning for telemetry...

Comment 21

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

I think bug 1308167 is for 32 bit Firefox on 32 bit Linux.

Comment 22

[Julien Cristau [:jcristau]]

Not sure what to make of comment 21.  32 bit firefox on 32 bit linux is what I've been testing.

Comment 23

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

Sorry, I misread the url in comment #20.

It appears that the rules in bug 1308167 weren't setup correctly.

Comment 24

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

(In reply to Matt Howell [:mhowell] from comment #18)
> Comment on attachment 8933166 [details] [diff] [review]
> patch rev2
> 
> Review of attachment 8933166 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, with one probably unimportant question:
> 
> ::: toolkit/modules/tests/xpcshell/test_UpdateUtils_url.js
> @@ +146,3 @@
> >    }
> >  
> > +  return "error";
> 
> I doubt it matters, but this got changed from "unknown" when the ARM
> properties were added, and I can't see why?
I changed it locally per comment #11 before ARM support was added.
It returns "error" so if a value isn't returned the test will fail.

Comment 25

[Molly Howell (she/her) (mostly inactive)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #24)
> I changed it locally per comment #11 before ARM support was added.
> It returns "error" so if a value isn't returned the test will fail.

Ah, I see, that makes sense. Thanks.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1038ecda6a1d
use system-info to detect latest CPU instruction set for the update url on all platforms. r=mhowell

Comment 28

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/1038ecda6a1d

Comment 29

[Julien Cristau [:jcristau]]

Following up on comment 20, after restarting firefox in that pentium3 VM I get a message in the terminal saying "This browser version requires a processor with the SSE2 instruction set extension.
You may be able to obtain a version that does not require SSE2 from your Linux distribution." so no surprise there.

Comment 30

[Julien Cristau [:jcristau]]

Verified that nightly on my laptop sends ISET:SSE4_2 while beta uses ISET:NA.

Do we want to uplift this to 58?

Comment 31

[Boris Zbarsky [:bzbarsky]]

Is there a followup for comment 15?

Comment 32

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

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #31)
> Is there a followup for comment 15?
Filed bug 1423079

Comment 33

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

(In reply to Julien Cristau [:jcristau] from comment #30)
> Verified that nightly on my laptop sends ISET:SSE4_2 while beta uses ISET:NA.
> 
> Do we want to uplift this to 58?
There are no plans that I know of to remove support for another CPU instruction set at present so I don't think this needs to be uplifted.