1748700 - wasm-via-Ion: fix incorrect 64-bit compare-select merging introduced in bug 1716580

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect, P1)

Description

[Manuel Martin]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:97.0) Gecko/20100101 Firefox/97.0

Steps to reproduce:

• Go to hubs.mozilla.com
• Enter the room
• Spawn a 3D model using the bottom toolbar "Place" button
• Click on the 3D object and try to move it around

Actual results:

The 3D object doesn't move

Expected results:

You should be able to move the object around.

This works fine in FF and Chrome in all OSs and it also works fine in FF Nightly Not Mac OSs.

The object movement is handled by the physics system (AmmoJS). I've tracked down the error to a call to: https://github.com/InfiniteLee/three-ammo/blob/db3ad1104b258d963ca66165f9fbed4013faa32a/src/world.js#L51-L54 For some reason that makes the worker tick to stop and the simulation is halted https://github.com/InfiniteLee/three-ammo/blob/db3ad1104b258d963ca66165f9fbed4013faa32a/src/ammo.worker.js#L74

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::Widget: Cocoa' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 2

[Stephen A Pohl [:spohl]]

Since this reproduces in nightly but not release, could you run mozregression[1] to see when this started happening? If you have never run mozregression before, simply run these three commands in a Terminal window:

sudo easy_install pip
sudo python3 -m pip install -U mozregression --ignore-installed
mozregression --good 2017-01-01

A number of Firefox versions will open in succession to narrow down when this started occurring. Simply type "good" or "bad" in Terminal based on whether or not a build reproduces the bug. Once finished, please post the output from the last run. It should give a last good and first bad revision as well as a link to look at the changesets in that range. Thank you!

[1] https://mozilla.github.io/mozregression/

Comment 3

[Manuel Martin]

This is the output of the last run:

69:27.68 INFO: Downloading build from: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/JkXUELeRT76NcOKYGXFyRQ/runs/0/artifacts/public%2Fbuild%2Ftarget.dmg
===== Downloaded 100% =====
69:34.63 INFO: Running autoland build built on 2021-12-17 14:08:44.368000, revision cfda85b5
69:51.65 INFO: Launching /private/var/folders/0s/gngl1zqd3r77lh7m7fpk6jc40000gn/T/tmpwiuxiy4b/Firefox Nightly.app/Contents/MacOS/firefox
69:51.65 INFO: Application command: /private/var/folders/0s/gngl1zqd3r77lh7m7fpk6jc40000gn/T/tmpwiuxiy4b/Firefox Nightly.app/Contents/MacOS/firefox -foreground -profile /var/folders/0s/gngl1zqd3r77lh7m7fpk6jc40000gn/T/tmpdlx71ak2.mozrunner
69:51.66 INFO: application_buildid: 20211217135618
69:51.66 INFO: application_changeset: cfda85b56f5ec28ea9f904b1978e7b0198a84be0
69:51.66 INFO: application_name: Firefox
69:51.66 INFO: application_repository: https://hg.mozilla.org/integration/autoland
69:51.66 INFO: application_version: 97.0a1
Was this integration build good, bad, or broken? (type 'good', 'bad', 'skip', 'retry', 'back' or 'exit' and press Enter): good
70:34.93 INFO: Narrowed integration regression window from [71af0ba0, d3f99095] (3 builds) to [cfda85b5, d3f99095] (2 builds) (~1 steps left)
70:34.93 INFO: No more integration revisions, bisection finished.
70:34.93 INFO: Last good revision: cfda85b56f5ec28ea9f904b1978e7b0198a84be0
70:34.93 INFO: First bad revision: d3f99095b5a726a28b7c111ff9f6c0caad7701bb
70:34.93 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=cfda85b56f5ec28ea9f904b1978e7b0198a84be0&tochange=d3f99095b5a726a28b7c111ff9f6c0caad7701bb

Thanks!

Comment 4

[Stephen A Pohl [:spohl]]

(In reply to Manuel Martin from comment #3)

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=cfda85b56f5ec28ea9f904b1978e7b0198a84be0&tochange=d3f99095b5a726a28b7c111ff9f6c0caad7701bb

Thank you!

Comment 5

[Lars T Hansen [:lth]]

I think we'll want some attention on this ASAP with an uplift if the patch does not make the FF97 train.

Comment 6

[Julian Seward [:jseward]]

Reproduced. It's a bit confusing though:

• I suspect reliably not-reproducing or reproducing it, as the bug 1716580
  changeset is un-applied or re-applied respectively, depends on setting
  javascript.options.wasm_caching to false. For good measure I also set
  javascript.options.wasm_baselinejit to false.

• To be more precise about moving of the 3D object: immediately after
  selecting one of the many offered objects, a box with three animated loops
  is drawn. Moving the box always works. The box soon "times out" and is
  replaced by a picture of the chosen object, and it's that picture that
  cannot be moved. I tested using the goldfish object.

Comment 7

[Manuel Martin]

Just a clarification regarding your second comment "replaced by a picture of the chosen object". Once the object loader cube disappears you should not see a picture, you should see and actual 3D object. This has a kinematic physics body attached with a constraint attached to the mouse pointer so it moves when you drag it around but as the physics system tick stops right after adding the body to the world, there is no simulation.

Comment 8

[Julian Seward [:jseward]]

Further STR clarification: not all 3D objects cause the problem. For example,
the "Stereo Tape Recorder" works fine. However, the Goldfish does reliably
cause the problem (that is, become non-movable once it is drawn).

Setting javascript.options.wasm_optimizingjit to false causes the problem to
disappear, unsurprisingly, given the regressing patch.

Comment 9

[Julian Seward [:jseward]]

It also fails in the same way (viz, only with Ion) also on aarch64-linux.

Comment 10

[Julian Seward [:jseward]]

Attachment: Bug 1748700 - wasm-via-Ion: fix incorrect 64-bit compare-select merging introduced in bug 1716580. r=lth.

Prior to bug 1716580, Ion could in some cases merge 32-bit wasm compares and
32-bit wasm into just two machine instructions. Bug 1716580 expanded that to
include 64-bit operands on some targets. Unfortunately the resulting code was
incorrect for the case where the comparison is of unsigned-64-bit values, and
this was not adequately covered by testing.

This patch:

• fixes the problem, which is a missing test in JSOpToCondition().

• enhances existing test cases for the transformation so as to check for
  correct handling of signedness in the comparisons.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e9ce5266ac40
wasm-via-Ion: fix incorrect 64-bit compare-select merging introduced in bug 1716580.  r=lth.

Comment 12

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/e9ce5266ac40

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 14

[Julian Seward [:jseward]]

Comment on attachment 9258480 [details]
Bug 1748700 - wasm-via-Ion: fix incorrect 64-bit compare-select merging introduced in bug 1716580. r=lth.

Beta/Release Uplift Approval Request

• User impact if declined: Possible wrong-code generation for some wasm inputs, leading to misbehaviour of wasm. With possible crashing of the associated content process -- at least, it is not obvious to me how to make an argument that such crashing couldn't occur.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): * it's a 1-liner fix for an obscure corner case relating to 64-bit arithmetic in wasm, which is something of a rarity given that wasm is currently mostly a 32-bit world.
• a standalone test case exists. It passes for x64 and arm64.
• I tested the fix in browser builds on both x64 and arm64, against the original problem case as described above in comments 0 through 8.
• String changes made/needed:

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9258480 [details]
Bug 1748700 - wasm-via-Ion: fix incorrect 64-bit compare-select merging introduced in bug 1716580. r=lth.

Approved for 97.0b3.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b0789e0f3df7