Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#19316 closed task (fixed)

Make sure our updates are dealing with SSE requirement properly

Reported by: gk Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: ff52-esr, TorBrowserTeam201704R, tbb-7.0-must-alpha
Cc: mcs, brade, boklm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

https://bugzilla.mozilla.org/show_bug.cgi?id=1271761 landed on ESR45 and it is very likely that Firefox won't work on machines without SSE support anymore in the next ESR (52). We should make sure we handle this properly with our updater. We want to be sure users without SSE support won't get an update then, for instance.

Child Tickets

Change History (38)

comment:1 Changed 3 years ago by mcs

Similar to the minSupportedOSVersion update manifest attribute that we added for the Mac OS 10.6 issue (see https://trac.torproject.org/projects/tor/ticket/13047#comment:3), we should add support for a new attribute such as: minInstructionSet="SSE2"

comment:2 Changed 3 years ago by yawning

We want to be sure users without SSE support won't get an update then, for instance.

Should they get a large clearly visible warning that their platform won't be supported for much longer? (In browser UI ideally since enough people probably don't read the release notes).

Anyone using something that doesn't support SSE2 is guaranteed to be on 32 bit Windows or Linux, and if we ever need to do it, detecting SSE2 support is trivial (CPUID with EAX set to 1, look at bit 26)...

comment:3 Changed 3 years ago by gk

Keywords: tbb-7.0-must added

comment:4 Changed 3 years ago by gk

Keywords: TorBrowserTeam201703 added

Getting those tickets on our March radar as well.

comment:5 Changed 3 years ago by cypherpunks

Are you sure want to make Tor Browser phoning home about CPU features?

comment:6 in reply to:  5 Changed 3 years ago by boklm

Replying to cypherpunks:

Are you sure want to make Tor Browser phoning home about CPU features?

If we add an update manifest attribute similar to minSupportedOSVersion (as suggested by mcs), we are not phoning home with the CPU features, but checking locally that the feature is available.

comment:7 in reply to:  1 ; Changed 3 years ago by boklm

Replying to mcs:

Similar to the minSupportedOSVersion update manifest attribute that we added for the Mac OS 10.6 issue (see https://trac.torproject.org/projects/tor/ticket/13047#comment:3), we should add support for a new attribute such as: minInstructionSet="SSE2"

I attached a first version of a patch to do that, but I did not test it yet, so it may not work.

I think there will still be an issue with people upgrading to the new ESR52 version from Tor Browser <= 6.5.1 (skipping the version that added support for the minInstructionSet attribute), as their browser will not know this attribute. If we want to avoid this, we will need to change the update_2 from the updates URL to update_3 (similar to what we did in #13047).

comment:8 in reply to:  7 Changed 3 years ago by gk

Status: newneeds_revision

Replying to boklm:

Replying to mcs:

Similar to the minSupportedOSVersion update manifest attribute that we added for the Mac OS 10.6 issue (see https://trac.torproject.org/projects/tor/ticket/13047#comment:3), we should add support for a new attribute such as: minInstructionSet="SSE2"

I attached a first version of a patch to do that, but I did not test it yet, so it may not work.

I think there will still be an issue with people upgrading to the new ESR52 version from Tor Browser <= 6.5.1 (skipping the version that added support for the minInstructionSet attribute), as their browser will not know this attribute. If we want to avoid this, we will need to change the update_2 from the updates URL to update_3 (similar to what we did in #13047).

The plan sounds good to me. I think we should be less strict here, though: it seems SSE2 is only required for Windows builds with ESR 52. The Linux bug is https://bugzilla.mozilla.org/show_bug.cgi?id=1274196 which will land in Firefox 53. I am not sure about OS X. The tracking bug for the update on Mozilla's side is: https://bugzilla.mozilla.org/show_bug.cgi?id=1274196 and there is a way to detect SSE2 support for Linux using UpdateUtils code included in Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1277359.

comment:9 Changed 3 years ago by gk

It seems like all macOS Intel machines are SSE2 capable, so we are fine in that regard.

comment:10 in reply to:  7 ; Changed 3 years ago by mcs

Replying to boklm:

Replying to mcs:

Similar to the minSupportedOSVersion update manifest attribute that we added for the Mac OS 10.6 issue (see https://trac.torproject.org/projects/tor/ticket/13047#comment:3), we should add support for a new attribute such as: minInstructionSet="SSE2"

I attached a first version of a patch to do that, but I did not test it yet, so it may not work.

It seems like we just need one attribute in the update manifest, e.g., minSupportedInstructionSet.

Also, I think your patch is missing a !. Maybe just do something like:

if (!this.unsupported && update.hasAttribute("minSupportedInstructionSet")) {
  let minInstructionSet = update.getAttribute("minSupportedInstructionSet");
  if (['MMX', 'SSE', 'SSE2', 'SSE3',
       'SSE4A', 'SSE4_1', 'SSE4_2'].indexOf(minInstructionSet) >= 0) {
  try {
    this.unsupported = !Services.sysinfo.getProperty("has" + minInstructionSet);
  } catch (e) {}
}

(untested).

I think there will still be an issue with people upgrading to the new ESR52 version from Tor Browser <= 6.5.1 (skipping the version that added support for the minInstructionSet attribute), as their browser will not know this attribute. If we want to avoid this, we will need to change the update_2 from the updates URL to update_3 (similar to what we did in #13047).

Do we really need to change to update3? We are not changing the request URL. We should add support for the new instruction set attribute to TB 6.5.2 and then advertise to older Windows browsers an update to 6.5.2. Then, after that update is done, the 6.5.2 browser will recognize the new attribute and make the appropriate instruction set check. Mozilla does this kind of thing once in a while; they call it a step-wise update or flag day or something ;)

comment:11 in reply to:  10 Changed 3 years ago by boklm

Replying to mcs:

Replying to boklm:

Replying to mcs:

Similar to the minSupportedOSVersion update manifest attribute that we added for the Mac OS 10.6 issue (see https://trac.torproject.org/projects/tor/ticket/13047#comment:3), we should add support for a new attribute such as: minInstructionSet="SSE2"

I attached a first version of a patch to do that, but I did not test it yet, so it may not work.

It seems like we just need one attribute in the update manifest, e.g., minSupportedInstructionSet.

Also, I think your patch is missing a !. Maybe just do something like:

if (!this.unsupported && update.hasAttribute("minSupportedInstructionSet")) {
  let minInstructionSet = update.getAttribute("minSupportedInstructionSet");
  if (['MMX', 'SSE', 'SSE2', 'SSE3',
       'SSE4A', 'SSE4_1', 'SSE4_2'].indexOf(minInstructionSet) >= 0) {
  try {
    this.unsupported = !Services.sysinfo.getProperty("has" + minInstructionSet);
  } catch (e) {}
}

(untested).

This looks better, thanks.

I think there will still be an issue with people upgrading to the new ESR52 version from Tor Browser <= 6.5.1 (skipping the version that added support for the minInstructionSet attribute), as their browser will not know this attribute. If we want to avoid this, we will need to change the update_2 from the updates URL to update_3 (similar to what we did in #13047).

Do we really need to change to update3? We are not changing the request URL. We should add support for the new instruction set attribute to TB 6.5.2 and then advertise to older Windows browsers an update to 6.5.2. Then, after that update is done, the 6.5.2 browser will recognize the new attribute and make the appropriate instruction set check. Mozilla does this kind of thing once in a while; they call it a step-wise update or flag day or something ;)

I think we have no easy way to continue advertising 6.5.2 to older Windows browsers after 7.0 is released, with what we use to generate the update responses files. We can add support for that, but it will probably require keeping a copy of the 6.5.2 mar files when generating update response files for many of the future releases. I think an easier way to do that is changing the update url to update_3 in 6.5.2, and not advertising 7.0 in update_2. We can also add update_2 -> update_3 redirects for the Linux and OSX URLs so that Linux and OSX browsers are updated to the latest version directly.

comment:12 Changed 3 years ago by boklm

I made a build with version 2 of the patch:
https://people.torproject.org/~boklm/builds/bug19316/

I did not yet find a computer without SSE3 (or how to emulate one) to test it.

comment:13 Changed 3 years ago by gk

Keywords: TorBrowserTeam201704 added; TorBrowserTeam201703 removed

Moving tickets over to April

comment:14 Changed 3 years ago by gk

Keywords: tbb-7.0-must-alpha added; tbb-7.0-must removed

Getting this on our radar for alpha release in less than two weeks.

comment:15 Changed 3 years ago by gk

Status: needs_revisionneeds_information

It seems to me there is still a patch for the client side missing for this bug?

comment:16 in reply to:  12 Changed 3 years ago by boklm

Status: needs_informationneeds_review

Replying to boklm:

I made a build with version 2 of the patch:
https://people.torproject.org/~boklm/builds/bug19316/

I did not yet find a computer without SSE3 (or how to emulate one) to test it.

I have no computer without SSE2 support (and could not find how to emulate one), so I tested with SSE4A (which is not supported on my computer since it seems to be AMD only).

If I set this url as app.update.url.override: https://people.torproject.org/~boklm/bug19316/SSE4A.xml
Then the update is not started, with a message saying "You Tor Browser is out of date, but the latest version is not supported on your system".

If I set this url as app.update.url.override: https://people.torproject.org/~boklm/bug19316/SSE2.xml
Then the updating process is starting.

So the patch seems to be working as expected.

comment:17 Changed 3 years ago by boklm

Keywords: TorBrowserTeam201704R added; TorBrowserTeam201704 removed

comment:18 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Thanks. Applied as commit 3e531451125720e28f3b7fd113af19b5d02aad18 to tor-browser-52.0.2esr-7.0-2.

comment:19 Changed 3 years ago by boklm

Should we get that into 6.5.2 too, or is that too late?

It seems 6.5.2 will be the last release working without SSE2.

An other option is to make a 6.5.3 release with only that change, with the drawback that the update to 7.0 will have to be done in 2 steps.

comment:20 Changed 3 years ago by boklm

I attached 0001-fixup-Bug-4234-Use-the-Firefox-Update-Process-for-To-update-URL.patch, which change the update URL from update_2 to update_3.

This will allow us to keep advertising 6.5.2 in update_2 after 7.0 is available, so that 6.5.1 users who did not update to 6.5.2 and don't have SSE are not updated directly to 7.0.

comment:21 in reply to:  19 Changed 3 years ago by gk

Replying to boklm:

Should we get that into 6.5.2 too, or is that too late?

It seems 6.5.2 will be the last release working without SSE2.

An other option is to make a 6.5.3 release with only that change, with the drawback that the update to 7.0 will have to be done in 2 steps.

Yes, let's do a rebuild with a buildN tag (N > 1).

comment:22 in reply to:  20 Changed 3 years ago by gk

Replying to boklm:

I attached 0001-fixup-Bug-4234-Use-the-Firefox-Update-Process-for-To-update-URL.patch, which change the update URL from update_2 to update_3.

This will allow us to keep advertising 6.5.2 in update_2 after 7.0 is available, so that 6.5.1 users who did not update to 6.5.2 and don't have SSE are not updated directly to 7.0.

I think this makes sense to me. mcs/brade what do you think? (Note in this whole discussion Windows users are the only ones that are affected here)

comment:23 Changed 3 years ago by brade

The patch looks fine and this approach makes sense to us.

comment:24 Changed 3 years ago by boklm

When we publish the 6.5.2 release, we should upload the update manifests to both update_2 and update_3.

When we publish the 7.0 release, we should upload the new update manifests to update_3 only, keeping the 6.5.2 manifests in update_2. We should also add some update_2/release/{Linux,OSX} -> update_3/release/{Linux,OSX} redirects as only Windows users are affected.

We should also remember to keep the 6.5.2 mar files (at least the Windows ones) on cdn.torproject.org for a while.

comment:25 Changed 3 years ago by gk

Thanks. This is commit db3f92ed23e12a2a9b9eac5be773d475897edfca and commit 15cf08adee2d81031b12769e75edb02eab85042b on tor-browser-45.9.0esr-6.5-1. I guess we need your second patch on the esr52 branch as well (even though we are a bit too late for the smart approach in comment:24)?

comment:26 Changed 3 years ago by cypherpunks

tbb-nightly works great without SSE2 (except WebGL, maybe), so there is no need to hurry up with this until Mozilla decides to break esr52 entirely in subsequent updates.

comment:27 in reply to:  25 Changed 3 years ago by boklm

Replying to gk:

Thanks. This is commit db3f92ed23e12a2a9b9eac5be773d475897edfca and commit 15cf08adee2d81031b12769e75edb02eab85042b on tor-browser-45.9.0esr-6.5-1. I guess we need your second patch on the esr52 branch as well (even though we are a bit too late for the smart approach in comment:24)?

Yes, I think we want to use update_3 URLs in the esr52 branch too, for consistency with the stable branch.

comment:28 Changed 3 years ago by gk

Okay, that's commit 5e44400548a9fd480e6715771c75dad75796f1e7 on tor-browser-52.0.2esr-7.0-2.

comment:29 in reply to:  26 Changed 3 years ago by gk

Replying to cypherpunks:

tbb-nightly works great without SSE2 (except WebGL, maybe), so there is no need to hurry up with this until Mozilla decides to break esr52 entirely in subsequent updates.

I don't think we should take the risk that this is happening with secruity updates because Mozilla is assuming only SSE2 capable machines will run Firefox ESR 52 on Windows.

comment:30 Changed 3 years ago by boklm

Resolution: fixed
Status: closedreopened

In my branch bug_19316_update-responses I added a patch to add support for minSupportedInstructionSet in the config.yml file:
https://gitweb.torproject.org/user/boklm/tor-browser-bundle.git/commit/?h=bug_19316_update-responses&id=70961a33301ca42b1fe3eb9eee87ea16bef4fea2

We don't need it for this release yet, but we will need it when releasing 7.0.

comment:31 Changed 3 years ago by boklm

Status: reopenedneeds_review

comment:32 in reply to:  30 ; Changed 3 years ago by gk

Replying to boklm:

In my branch bug_19316_update-responses I added a patch to add support for minSupportedInstructionSet in the config.yml file:
https://gitweb.torproject.org/user/boklm/tor-browser-bundle.git/commit/?h=bug_19316_update-responses&id=70961a33301ca42b1fe3eb9eee87ea16bef4fea2

We don't need it for this release yet, but we will need it when releasing 7.0.

I already added

        win32:
            minSupportedInstructionSet: SSE2

to the config.yml file for 7.0a3. I guess we can make use of that by just committing your patch to master and using that to create the update responses files? Or do we need to be on the exact tag for that?

comment:33 in reply to:  32 Changed 3 years ago by boklm

Replying to gk:

Replying to boklm:

In my branch bug_19316_update-responses I added a patch to add support for minSupportedInstructionSet in the config.yml file:
https://gitweb.torproject.org/user/boklm/tor-browser-bundle.git/commit/?h=bug_19316_update-responses&id=70961a33301ca42b1fe3eb9eee87ea16bef4fea2

We don't need it for this release yet, but we will need it when releasing 7.0.

I already added

        win32:
            minSupportedInstructionSet: SSE2

to the config.yml file for 7.0a3. I guess we can make use of that by just committing your patch to master and using that to create the update responses files? Or do we need to be on the exact tag for that?

Yes, I think we can do that.

comment:34 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Sounds good. I committed your changes to master (commit 70961a33301ca42b1fe3eb9eee87ea16bef4fea2).

comment:35 in reply to:  24 Changed 3 years ago by boklm

Replying to boklm:

When we publish the 6.5.2 release, we should upload the update manifests to both update_2 and update_3.

This is done.

When we publish the 7.0 release, we should upload the new update manifests to update_3 only, keeping the 6.5.2 manifests in update_2. We should also add some update_2/release/{Linux,OSX} -> update_3/release/{Linux,OSX} redirects as only Windows users are affected.

I opened #22003 for that.

We should also remember to keep the 6.5.2 mar files (at least the Windows ones) on cdn.torproject.org for a while.

I created the file /srv/cdn-master.torproject.org/htdocs/aus1/torbrowser/6.5.2-do-not-remove.txt on staticiforme to remember that.

Note: See TracTickets for help on using tickets.