ARM64: failure of a wasm module on m.twitch.tv stream pages

VERIFIED FIXED in Firefox 66

Status

()

defect
P2
normal
VERIFIED FIXED
5 months ago
3 months ago

People

(Reporter: twisniewski, Assigned: lth)

Tracking

({regression})

Trunk
mozilla67
ARM64
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 disabled, firefox64 disabled, firefox65 disabled, firefox66 verified, firefox67 verified)

Details

(Whiteboard: [arm64:m3])

Attachments

(2 attachments, 1 obsolete attachment)

As reported on webcompat.com, m.twitch.tv stream pages do not compile or run a wasm module properly on arm64 builds, resulting in a blank video. Arm32 builds seem fine, and disabling javascript.options.wasm_baselinejit works around the error as well (presumably by disabling wasm altogether).

The only clues I currently have are that their script wasmworker.min.js is logging a mystery number to console, in this code-fragment:

function D(a) {
  if (e.onAbort) e.onAbort(a);
  void 0 !== a ? (e.print(a), e.printErr(a), a=JSON.stringify(a))
                     : a="";
  ma = !0;
  throw "abort(" + a + "). Build with -s ASSERTIONS=1 for more info.";
}

On my Android 8 phone, this ends up logging the number 25.

Reporter

Comment 1

5 months ago

A console.trace shows this is how function D above is reached (I'm only getting the number 22 or 25 as the "error", not sure why it varies):

console.trace() 22 wasmworker.min.js:160:204
	D https://cvp.twitch.tv/2.8.3/wasmworker.min.js:160
	<anonymous> https://cvp.twitch.tv/2.8.3/wasmworker.min.wasm:11197
	<anonymous> https://cvp.twitch.tv/2.8.3/wasmworker.min.wasm:211476
	<anonymous> https://cvp.twitch.tv/2.8.3/wasmworker.min.wasm:634433
	<anonymous> https://cvp.twitch.tv/2.8.3/wasmworker.min.wasm:410958
	dynCall_viii_870873056 https://cvp.twitch.tv/2.8.3/wasmworker.min.js line 82 > Function:4
	write https://cvp.twitch.tv/2.8.3/wasmworker.min.js:122
	toWireType https://cvp.twitch.tv/2.8.3/wasmworker.min.js:122
	<anonymous> https://cvp.twitch.tv/2.8.3/wasmworker.min.js:126
	b https://cvp.twitch.tv/2.8.3/wasmworker.min.js:124
	WebMediaPlayer https://cvp.twitch.tv/2.8.3/wasmworker.min.js line 69 > Function:4
	d https://cvp.twitch.tv/2.8.3/wasmworker.min.js:34
	b https://cvp.twitch.tv/2.8.3/wasmworker.min.js:33
	onRuntimeInitialized https://cvp.twitch.tv/2.8.3/wasmworker.min.js:33
	a https://cvp.twitch.tv/2.8.3/wasmworker.min.js:159
	kd https://cvp.twitch.tv/2.8.3/wasmworker.min.js:160
	jd https://cvp.twitch.tv/2.8.3/wasmworker.min.js:158
	c https://cvp.twitch.tv/2.8.3/wasmworker.min.js:60

But unfortunately I'm not sure where to go from here. I've tried redirecting the requests to their 2.8.0 and 2.7.0 versions of the JS and wasm modules, but only received slightly different error numbers in response (it's still failing similarly).

Reporter

Updated

5 months ago
Summary: ARM64: plausible compilation failure of a wasm module for m.twitch.tv stream pages → ARM64: failure of a wasm module on m.twitch.tv stream pages
OS: Unspecified → Android
Hardware: Unspecified → ARM64
Whiteboard: [arm64:m3]
Assignee

Comment 2

5 months ago

I can repro the problem locally on my Nexus 5X. There is a single wasm, about 760K, emscripten-compiled, with an elaborate song-and-dance preceding every call_indirect, so likely compiled C++. Lots of strings, one prominent one is "WebMediaPlayer".

Assignee

Comment 3

5 months ago

I should say, I can repro the symptom (no video) but I'm not seeing any error code being reported. What we have here is js + wasm running in a worker, and I've been unable so far to use our web debugging tools effectively to get at the problem at all.

Thomas, can you give me detailed STR (including how to setup tools) so that I can try to repro the printing of this error?

Another thing I'd like to know is whether you're logged in as a registered user or you're using twitch anonymously (as I am).

Assignee

Updated

5 months ago
Flags: needinfo?(twisniewski)

Lars: on your device, does 'javascript.options.wasm = false' allows the video to play?

Reporter

Comment 5

5 months ago

The STR for me are just:

  1. run the current arm64 nightly build specifically offered in the Play Store on your device (I'm using a Samsung Galaxy S7 Edge).
  2. open the remote devtools on a tab so it will definitely capture the console log.
  3. visit any Twitch stream page on that tab, for instance https://m.twitch.tv/stray228
  4. the page should load, the "tap to unmute" indicator should briefly appear and vanish over the video area, but the video never plays.
  5. a console message should be logged by the script wasmworker.min.js, which just spits out a number like 22 or 25.

Note that I have to be running that arm64 build from the Play Store. If I use a build from mozregression with app=fennec, it runs a 32-bit build that doesn't exhibit this problem.

Flags: needinfo?(twisniewski)
Assignee

Comment 6

5 months ago

(In reply to Luke Wagner [:luke] from comment #4)

Lars: on your device, does 'javascript.options.wasm = false' allows the video to play?

Yes.

Assignee

Comment 7

5 months ago

(In reply to Thomas Wisniewski [:twisniewski] (PTO Dec 24-26, 31-2nd) from comment #5)

  1. run the current arm64 nightly build specifically offered in the Play Store on your device (I'm using a Samsung Galaxy S7 Edge).

OK. I have a different phone but it's a Play Store nightly build. I have verified it runs 64-bit. I connect to it over USB from my work computer.

  1. open the remote devtools on a tab so it will definitely capture the console log.

By this, do you mean the about:debugging page in Nightly or something else? Do you mean the JS console or something else? Please be very specific, and imagine you're talking to somebody who basically never debugs JS or wasm at this level...

I do see JS console output from the app in the about:debugging window (or in the WebIDE when attaching from a DevEd build), I just have not seen the error code you describe. And I don't understand how you obtained the backtrace in comment 1.

  1. visit any Twitch stream page on that tab, for instance https://m.twitch.tv/stray228
  2. the page should load, the "tap to unmute" indicator should briefly appear and vanish over the video area, but the video never plays.

Yes, exactly what I see.

  1. a console message should be logged by the script wasmworker.min.js, which just spits out a number like 22 or 25.

But not this, unfortunately :(

(Obviously this is likely a wasm error, but without a toehold on something on this end there's not much I can do yet.)

Flags: needinfo?(twisniewski)
Reporter

Comment 8

5 months ago

Sorry for any confusion, lth. I just meant the JS/web console. This is what I'm seeing, for the record:

{"message":"Polyfilling worker: //polyfill.twitchsvc.net/v2/polyfill.min.js?unknown=polyfill&features=fetch%2CSet%2CMap%2CObject.assign%2CURL%2CUserTiming%2CString.prototype.startsWith%2CArray.prototype.includes%2CString.prototype.includes%2CObject.values%2CObject.entries%2CArray.from%2CIntl%2CPromise%2CrequestAnimationFrame%2CIntl.~locale.en&flags=gated","level":"INFO","time":1548347976985} df17272481eacc8b4ff279340de7deaa82b6ca2d.worker.js:1:89151

{"level":"INFO","message":"Instantiating client and connecting to mongraal","time":1548347977069} df17272481eacc8b4ff279340de7deaa82b6ca2d.worker.js:1:89151

{"error":{},"message":"Error with initial connection to chat service","level":"ERROR","time":1548347978118} df17272481eacc8b4ff279340de7deaa82b6ca2d.worker.js:1:89289
error12NextJS

Object { event: "benchmark_custom_event", properties: {…}, level: "DEBUG", time: 1548347978226 } consoleLogger.js:44

Object { event: "benchmark_custom_event", properties: {…}, level: "DEBUG", time: 1548347978903 } consoleLogger.js:44

Object { event: "benchmark_custom_event", properties: {…}, level: "DEBUG", time: 1548347979016 } consoleLogger.js:44

Object { event: "benchmark_complete_transition", properties: {…}, level: "DEBUG", time: 1548347979024 } consoleLogger.js:44

failed to resolve properties promise Response { type: "cors", url: "https://api.twitch.tv/kraken/user", redirected: false, status: 401, ok: false, statusText: "Unauthorized", headers: 
Headers, body: ReadableStream, bodyUsed: false } player.7b11ba5d6fbe02003c6e.js:42:385654

25 wasmworker.min.js:160:242

25 wasmworker.min.js:160:253

I see the following analogues entries using adb logcat as well:

01-24 11:39:36.989  7402  8133 I GeckoConsole: {"message":"Polyfilling worker: //polyfill.twitchsvc.net/v2/polyfill.min.js?unknown=polyfill&features=fetch%2CSet%2CMap%2CObject.assign%2CURL%2CUserTiming%2CString.prototype.startsWith%2CArray.prototype.includes%2CString.prototype.includes%2CObject.values%2CObject.entries%2CArray.from%2CIntl%2CPromise%2CrequestAnimationFrame%2CIntl.~locale.en&flags=gated","level":"INFO","time":1548347976985}
01-24 11:39:37.077  7402  8133 I GeckoConsole: {"level":"INFO","message":"Instantiating client and connecting to mongraal","time":1548347977069}
01-24 11:39:38.121  7402  8133 E GeckoConsole: [JavaScript Error: "{"error":{},"message":"Error with initial connection to chat service","level":"ERROR","time":1548347978118}"]
01-24 11:39:38.228  7402  8133 I GeckoConsole: [object Object]
01-24 11:39:38.903  7402  8133 I GeckoConsole: [object Object]
01-24 11:39:39.016  7402  8133 I GeckoConsole: [object Object]
01-24 11:39:39.025  7402  8133 I GeckoConsole: [object Object]
01-24 11:39:39.260  7402  8133 W GeckoConsole: [JavaScript Warning: "failed to resolve properties promise [object Response]"]
01-24 11:39:40.153  7402  8133 I GeckoConsole: 25
01-24 11:39:40.154  7402  8133 W GeckoConsole: [JavaScript Warning: "25"]
01-24 11:39:46.882  7402  8133 W GeckoEventDispatcher: No listener for Session:DataWritten

In case you've never used either tool, there are some docs: remote devtools, adb logcat. I'm on IRC in #webcompat, and would be happy to help a bit more directly, if you'd like.

Beyond that, I wonder if it's specific to one's Android version? I'm running a Samsung Galaxy S7 Edge with Android 8.0.0 and Samsung Experience 9.0, for instance.

Flags: needinfo?(twisniewski)

One idea for getting a toe hold is to add instrumentation to, say, prologue/epilogue that prints "entering"/"leaving"+funcIndex and compare the output logs of ARM32 vs. x64 vs. ARM64. If we're lucky, there will be a point where ARM64 diverges from the other two and then we could dig in further if need be with finer-grained logging.

Assignee

Comment 10

5 months ago

Yeah, that's been my thought as well. (Relative debugging is such a powerful idea and yet so poorly supported, but then i guess it's a hard idea to generalize.)

For what it's worth, I'm perfectly willing to run any test-builds and providing logging output, if no one else can reproduce this.

Assignee

Comment 12

5 months ago

On the overview page (the page listing the available streams for a game) I see this in the logcat, which is worrisome, it suggests something is broken with the script that we're trying to run in that worker:

01-25 09:20:42.926  3956  3982 E GeckoConsole: [JavaScript Error: "SyntaxError: missing ( before formal parameters" {file: "https://cvp.twitch.tv/2.8.4/wasmworker.min.js" line: 3 column: 29 source: "return function dynCall_viii_-1342046396(a1, a2, a3) {"}]

Then when I select a stream I get these messages:

01-25 09:21:56.212  3956  3956 D GeckoToolbar: onTabChanged: TITLE
01-25 09:21:56.214  3956  3956 D GeckoBrowserApp: BrowserApp.onTabChanged: 1: TITLE
01-25 09:21:56.236  3956  3982 I GeckoConsole: ON PAINT WHEN WAITED FOR
01-25 09:21:56.334  3956  3982 I GeckoConsole: {"message":"Polyfilling worker: //polyfill.twitchsvc.net/v2/polyfill.min.js?unknown=polyfill&features=fetch%2CSet%2CMap%2CObject.assign%2CURL%2CUserTiming%2CString.prototype.startsWith%2CArray.prototype.includes%2CString.prototype.includes%2CObject.values%2CObject.entries%2CArray.from%2CIntl%2CPromise%2CrequestAnimationFrame%2CIntl.~locale.en&flags=gated","level":"INFO","time":1548404516330}
01-25 09:21:56.360  3956  3982 E GeckoConsole: [JavaScript Error: "TypeError: doc.querySelector(...) is null" {file: "resource://gre/modules/SimpleServiceDiscovery.jsm" line: 362}]
01-25 09:21:56.364  3956  3982 E GeckoConsole: [JavaScript Error: "TypeError: doc.querySelector(...) is null" {file: "resource://gre/modules/SimpleServiceDiscovery.jsm" line: 362}]
01-25 09:21:56.431  3956  3982 I GeckoConsole: {"level":"INFO","message":"Instantiating client and connecting to tfue","time":1548404516418}
01-25 09:21:58.375  3956  3982 W GeckoEventDispatcher: No listener for Session:DataWritten
01-25 09:21:59.312  3956  3982 E GeckoConsole: [JavaScript Error: "{"error":{},"message":"Error with initial connection to chat service","level":"ERROR","time":1548404518411}"]
01-25 09:21:59.326  3956  3982 I GeckoConsole: [object Object]
01-25 09:21:59.395  3956  3982 I chatty  : uid=10136(org.mozilla.fennec_aurora) Gecko identical 1 line
01-25 09:21:59.403  3956  3982 I GeckoConsole: [object Object]
01-25 09:21:59.618  3956  3982 W GeckoConsole: [JavaScript Warning: "failed to resolve properties promise [object Response]"]

and then nothing more. These are the last lines you see before you see your magic error code, but I never get that one.

The system is running Android 8.1.0.

Assignee

Comment 13

5 months ago

Alright, maybe least a place to start: The following message shows up repeatedly. In particular it shows up if I reload the "stray228" page listed above. It only shows up in logcat, not in the console in the devtools, which is worrisome but suggests that worker debugging is still not working well.

01-25 10:09:37.601 11989 12008 E GeckoConsole:
[JavaScript Error: "SyntaxError: missing ( before formal parameters"
{file: "https://cvp.twitch.tv/2.8.4/wasmworker.min.js" line: 3 column: 29 
 source: "return function dynCall_viii_-1403165420(a1, a2, a3) {"}]

This is from the bit in the emscripten run-time library that generates JS code for calls into wasm [source forthcoming as soon as I can get it out of the debugger], and the negative number is the reason why this fails. This looks like a generated ID that should have been interpreted as a positive number, in this case 2891801876 aka 0xAC5D6514, but was not.

Wasm doesn't normally care about the signed/unsigned distinction but the wasm<->JS stub layer does, so maybe there's something there.

Maybe worth trying a debug build, in case we hit some assertion failure?

Assignee

Comment 15

5 months ago

Indeed, I have a few clues to follow up here. My next step is to try to repro in a build off of try / inbound, to ensure that I don't have to go via the play store at least, and if so a debug build should be possible.

Assignee

Comment 16

5 months ago

As a data point, a debug build does run and has the same symptoms, which is nice.

Looking at "return function dynCall_viii_-1403165420(a1, a2, a3) {", -1403165420 is an i32 with the high bit set, so signedness bug seems like a likely culprit? Maybe you can break in debugger at point of syntax error, look up the callstack and find which Number flowed into the string generation and how that Number was produced?

Assignee

Comment 18

5 months ago

Disabling the JS jits (but leaving wasm enabled) also makes the site work. This in a release Nightly, I'll try to repro in my local debug build. Be sure to disable Ion before disabling Baseline, or you're likely to run into bug 1523564.

Breaking at the syntax error only lets me know that I'm in a syntax parse stack, at least so far (on one of 118 threads...). Disabling off-thread compilation and off-thread parsing doesn't seem to change that.

Debugging is a little precarious and not terribly well documented but more or less seems to work, though a no-opt debug build may be required for anything meaningful.

Assignee

Updated

5 months ago
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Priority: -- → P2
Assignee

Updated

5 months ago
Depends on: 1522458
Assignee

Comment 19

5 months ago

Applying Benjamin's patch from bug 1522458 makes the bug go away, suggesting very strongly that this is an optimized-stubs problem.

Assignee

Comment 20

5 months ago

For my problem, though, only the change to WasmInstance.cpp appears to matter. This is the one that optimizes callImport.

Assignee

Comment 21

5 months ago

Summarizing the story so far:

  • Looks like a stubs problem when taking the optimized path for calling out to js
  • Ion is not a factor, this is reproducible with baseline only and the report predates Ion-enablement on arm64
  • Arm64 is a factor, so far
  • Could be a wasm stubs problem (what benjamin is looking at)
  • Could be a JS baseline problem that's specific to arm64
  • Even --baseline-eager may not be sufficient to trigger it for our test suite since, IIUC, there must be multiple calls through the import table to trigger the optimization and hence the bug, and even then the bug may be representation-dependent
    or value-dependent
  • The syntax error mentioned in comment 13 does not always occur, mostly it occurs during reloads, so it's not clear that it is the root cause, it could just be a symptom that is sometimes present
Assignee

Updated

5 months ago
Depends on: 1523941

Bug 1522458 is about the Ion to wasm fast path; Lars said the patch there doesn't fix the issue here, so updating dependencies.

No longer depends on: 1522458

Setting javascript.options.baselinejit=false makes both https://webassembly.org/demo/Tanks/ and https://www.twitch.tv/ work.

Alternatively you can set:

javascript.options.baselinejit.threshold=1000000000
javascript.options.ion.threshold=1000000000

Assignee

Comment 25

4 months ago

Oh is tanks not working? I didn't know that.

Yes, setting the thresholds very high is tantamount to disabling the JITs, so that makes sense.

Comment 20 is the important one here. There is something on this optimized call-out path that is wonky. I've been sitting still waiting for Benjamin to land his debug code so that I could more easily trace the execution, will ping him again. Benjamin?

Flags: needinfo?(bbouvier)
Assignee

Comment 26

4 months ago

I believe there is a minor bug in FillArgumentArray where code incorrectly canonicalizes Float32 along the non-toValue path (compare this to the Double path) but fixing this does not fix the problem:

diff --git a/js/src/wasm/WasmStubs.cpp b/js/src/wasm/WasmStubs.cpp
--- a/js/src/wasm/WasmStubs.cpp
+++ b/js/src/wasm/WasmStubs.cpp
@@ -1080,10 +1080,7 @@ static void FillArgumentArray(MacroAssem
             masm.storeDouble(fpscratch, dst);
           } else {
             // Preserve the NaN pattern in the input.
-            ScratchFloat32Scope fpscratch(masm);
-            masm.moveFloat32(srcReg, fpscratch);
-            masm.canonicalizeFloat(fpscratch);
-            masm.storeFloat32(fpscratch, dst);
+            masm.storeFloat32(srcReg, dst);
           }
         }
         break;

(In reply to Lars T Hansen [:lth] from comment #25)

Comment 20 is the important one here. There is something on this optimized call-out path that is wonky. I've been sitting still waiting for Benjamin to land his debug code so that I could more easily trace the execution, will ping him again. Benjamin?

I got the final review for this today, and it needs a few changes before landing, but that will happen soonish. Watch bug 1523993.

Flags: needinfo?(bbouvier)

Comment 28

4 months ago

The above evidence seems to point at either an entry or exit stub; it seems like we could narrow in pretty quickly by:

  1. confirming issue fixed if all optimized entry/exist stubs disabled (so we go through C++ both dirs)
  2. selectively enabling stubs based on env-var [low,high] func index bounds
  3. bisecting on that to find the individual import/exit stub at fault
Assignee

Comment 29

4 months ago

Per comment 20, it is known that this is the jitExit stub. Yes, bisecting the search space will probably be a good approach, I'm dedicating the day to this.

(Environment variables are not a thing on Android and there are various other restrictions that make it hard to configure the app at runtime because it's sandboxed, but something will work out.)

Assignee

Comment 30

4 months ago

Import #28, which has signature (I32 I32 I32 I32 I32 I32 I32 I32 I32 I32) -> Void.

The imported function takes 10 arguments as well (exactly) and is either a dynamically generated dyncall thing or __embind_register_value_object_field.

Assignee

Comment 31

4 months ago

I can get to that function in a debugger on desktop by loading m.twitch.tv, but I haven't yet been able to identify it or look at what's going on around it; if there's a way of printing script source from the debugger I haven't found it yet. So that'll be the next step.

Assignee

Comment 32

4 months ago

Well.

function f(a,b,c,d,e,f,g,h,i,j) {
    print(`${a} ${b} ${c} ${d} ${e} ${f} ${g} ${h} ${i} ${j}`);
}

var bin = wasmTextToBinary(
    `(module
       (import $f "m" "f" (func (param i32) (param i32) (param i32) (param i32) (param i32)
                                (param i32) (param i32) (param i32) (param i32) (param i32)))
       (func (export "test") (param $a i32) (param $b i32) (param $c i32) (param $d i32) (param $e i32)
                             (param $f i32) (param $g i32) (param $h i32) (param $i i32) (param $j i32)
         (call $f (get_local $a) (get_local $b) (get_local $c) (get_local $d) (get_local $e)
                  (get_local $f) (get_local $g) (get_local $h) (get_local $i) (get_local $j))))`);

var mod = new WebAssembly.Module(bin);
var ins = new WebAssembly.Instance(mod, {m:{f}});

for ( var i=0; i < 100; i++ ) {
    ins.exports.test(i, i+1, i+2, i+3, i+4, i+5, i+6, i+7, i+8, i+9);
}

Run with --no-ion --no-threads:

0 1 2 3 4 5 6 7 8 9
1 2 3 4 5 6 7 8 9 10
2 3 4 5 6 7 8 9 10 11
3 4 5 6 7 8 9 10 11 12
4 5 6 7 8 9 10 11 12 13
5 6 7 8 9 10 11 12 13 14
6 7 8 9 10 11 12 13 14 15
7 8 9 10 11 12 13 14 15 16
8 9 10 11 12 13 14 15 16 17
9 10 11 12 13 14 15 16 17 18
10 11 12 13 14 15 16 17 18 19
11 12 13 14 15 16 17 18 -2114690644 19
12 13 14 15 16 17 18 19 -2114690644 20
13 14 15 16 17 18 19 20 -2114690644 21
14 15 16 17 18 19 20 21 -2114690644 22
...
Assignee

Comment 33

4 months ago

(That's on the arm64 simulator btw.)

Assignee

Comment 34

4 months ago

Because:

diff --git a/js/src/wasm/WasmStubs.cpp b/js/src/wasm/WasmStubs.cpp
--- a/js/src/wasm/WasmStubs.cpp
+++ b/js/src/wasm/WasmStubs.cpp
@@ -1421,7 +1421,7 @@ static bool GenerateImportJitExit(MacroA
   argOffset += sizeof(Value);
 
   // 5. Fill the arguments
-  unsigned offsetToCallerStackArgs = jitFramePushed + sizeof(Frame);
+  unsigned offsetToCallerStackArgs = jitFramePushed + sizeof(Frame) + frameAlignExtra;
   FillArgumentArray(masm, fi.funcType().args(), argOffset,
                     offsetToCallerStackArgs, scratch, ToValue(true));
   argOffset += fi.funcType().args().length() * sizeof(Value);
Assignee

Comment 35

4 months ago

OK, that fixes this problem on twitch.tv. Patch coming.

Assignee

Comment 36

4 months ago

In the optimized stub, we forgot to account for the frameAlignExtra
word when computing the offset from the SP at which stack args are
read. The test case finds the problem on the ARM64 simulator, and
does not need any special parameters, just to run long enough for a JS
JIT to kick in.

Also a drive-by fix for an incorrect NaN canonicalization along the
non-toValue path, cf the Double case right above it. This code
changed recently when I introduced the ScratchFloat32Scope, but the
bug predates that.

Nice! Does it fix the Unity problem as well?

Assignee

Comment 38

4 months ago

(In reply to Luke Wagner [:luke] from comment #37)

Nice! Does it fix the Unity problem as well?

Forgot to check. In the past that demo has not worked on Android at all (for WebGL reasons? I forget). Will check tomorrow.

Comment 39

4 months ago
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d720cbe1187
Correct the offset for reading stack args on ARM64.  r=bbouvier
Assignee

Comment 40

4 months ago

(In reply to Lars T Hansen [:lth] from comment #38)

(In reply to Luke Wagner [:luke] from comment #37)

Nice! Does it fix the Unity problem as well?

Forgot to check. In the past that demo has not worked on Android at all (for WebGL reasons? I forget). Will check tomorrow.

Unity content does indeed not run on Android for WebGL reasons, so we'll want Anthony to test on Windows-on-ARM64 once there's a Nightly release that has this bugfix, Monday should be safe.

Flags: needinfo?(ajones)

Comment 41

4 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

Both twitch.tv and the web assembly demo work splendidly.

Flags: needinfo?(ajones)
Assignee

Comment 43

4 months ago

(Reposted content deleted)

Attachment #9046286 - Attachment is obsolete: true

Working splendidly sounds good! Do you want to request uplift to beta?

Flags: needinfo?(lhansen)
Assignee

Comment 45

4 months ago

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: None
  • User impact if declined: A fair amount of C/C++ code compiled to webassembly will likely not work on ARM64 (Android and Win10 alike)
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: Go to m.twitch.tv on an ARM64 device. Select a game. On the game's page, select a channel of a live video stream. If the stream plays then the bug is fixed.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Very local fix affecting only programs that would unquestionably have been broken previously (on ARM64).
  • String changes made/needed:
Flags: needinfo?(lhansen)
Attachment #9047293 - Flags: approval-mozilla-beta?
Comment on attachment 9047293 [details] [diff] [review]
bug1521939-arm64-stackargs-beta.patch

Fix for WASM/ARM64 issues that break Twitch.
This is such a specific fix that I'm OK with uplifting this for beta 13, then verifying after.
Attachment #9047293 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

Comment 48

4 months ago

Reproduced the issue on Firefox Beta 66.0b12 on Lenovo Yoga C630-13Q50 with Windows 10 Home (v18030).

The issue is verified - fixed on the latest Nightly 67.0a1 (20190303215808) on the above-mentioned setup.
Will verify on Beta13 too once it will be available.

Comment 49

3 months ago

Verified - Fixed on latest Firefox Beta 66.0b14 on Lenovo Yoga C630-13Q50 with Windows 10 Home (v18030).

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.