Closed Bug 1415336 Opened 2 years ago Closed 2 years ago

Linux CPU instruction detection code doesn't work correctly

Categories

(Toolkit :: Application Update, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- wontfix
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- verified

People

(Reporter: standard8, Assigned: rstrong)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Flags: needinfo?(robert.strong.bugs)
Flags: needinfo?(giles)
We could put cpuHasSSE2 on ChromeUtils or ThreadSafeChromeUtils or something instead.
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?
Flags: needinfo?(giles)
> 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...
This should probably fix things, but I have no idea how to test it...
(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?
Flags: needinfo?(ryanvm)
Not sure offhand what hardware SV has available for testing. Rares?
Flags: needinfo?(ryanvm) → needinfo?(rares.bologa)
(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. :(
Flags: needinfo?(rares.bologa)
bz: not sure what we can do here...
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
Attached patch patch rev1 (obsolete) — Splinter Review
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.
Assignee: nobody → robert.strong.bugs
Status: NEW → ASSIGNED
Flags: needinfo?(robert.strong.bugs)
Attachment #8932690 - Flags: feedback?(bhearsum)
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).
With that patch, navigator.cpuHasSSE2 can be removed, right?
Comment on attachment 8932690 [details] [diff] [review]
patch rev1

No issues here, thanks for checking!
Attachment #8932690 - Flags: feedback?(bhearsum) → feedback+
(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
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
Attached patch patch rev2Splinter Review
Try run
https://treeherder.mozilla.org/#/jobs?repo=try&revision=50c4258ce2ba88afe84820bab498d1d57095fd76

I had to include arm for the tests to pass.
Attachment #8926182 - Attachment is obsolete: true
Attachment #8932690 - Attachment is obsolete: true
Attachment #8933166 - Flags: review?(mhowell)
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?
Attachment #8933166 - Flags: review?(mhowell) → review+
Is there a reason to believe testing on a vm emulating pentium2 or some other old cpu wouldn't work?
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...
I think bug 1308167 is for 32 bit Firefox on 32 bit Linux.
Not sure what to make of comment 21.  32 bit firefox on 32 bit linux is what I've been testing.
Sorry, I misread the url in comment #20.

It appears that the rules in bug 1308167 weren't setup correctly.
(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.
(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.
Pushed by rstrong@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/1038ecda6a1d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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.
Verified that nightly on my laptop sends ISET:SSE4_2 while beta uses ISET:NA.

Do we want to uplift this to 58?
Flags: needinfo?(robert.strong.bugs)
Is there a followup for comment 15?
(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
Flags: needinfo?(robert.strong.bugs)
(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.
You need to log in before you can comment on or make changes to this bug.