1768346 - Bug 1767966 breaks https://pc-play.games.dmm.com/play/monmusutd/ with javascript error

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Toshihiro Yamada]

On https://pc-play.games.dmm.com/play/monmusutd/ (need DMM account), after I start with click at title but game stops there.

At least I reproduced it on Win11.

In Developer Console, an error appeared
Failed to call function ReceiveResponse of class WebGLApiConnector
Calling function ReceiveResponse with 'in' parameter is not supported.

Seems the error's origin is in wasm code, so I can't get any information.

Regression Range (in autoland tree)
Last OK:09b25e7ee28da7855f25cbf06ccc6452794f27f4
First NG:e59acdccc25bfcbbcadff3d4c0783a0a974bcf77

Apparently, regression from Bug 1767966

Comment 1

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

Set release status flags based on info from the regressing bug 1767966

Comment 2

[André Bargull [:anba]]

Hmm, unfortunately I can't debug this issue, because the game isn't available in my region:

このゲームはお住まいの地域からはご利用いただけません
お住まいの地域からは以下のゲームがご利用いただけます。

which translates as:

This game cannot be used from your area
The following games can be used from your area.

For the six games which are available for me, I got the following results:

Game	Status
アイ・アム・マジカミ	Works for me
文豪とアルケミスト	Chrome-only
神姫PROJECT	Chrome-only
御城プロジェクト:RE～CASTLE DEFENSE～	Works for me
FLOWER KNIGHT GIRL	Works for me
千年戦争アイギス	Doesn't work: "DOM exception: SecurityError: The operation is insecure."

So I'm not quite sure how to proceed best. We could

• either wait for another regression on a different web page and try to debug it there,
• or we back out some of the patches, maybe this gives a hint which patch exactly is at fault here,
• or someone with access to that specific game tries to debug the error.

Comment 3

[André Bargull [:anba]]

Hey Jan, what do you think is the best way to investigate this issue further?

Comment 4

[Jan de Mooij [:jandem]]

Toshihiro, would you be willing to test a few Windows Try builds for us?

We could then narrow it down more. I'm a bit suspicious of part 11 because that seems to be the most complicated change.

Comment 5

[Jan de Mooij [:jandem]]

Maybe I can test this with a VPN. I'll try that in a bit...

Comment 6

[Toshihiro Yamada]

(In reply to Jan de Mooij [:jandem] from comment #4)

Toshihiro, would you be willing to test a few Windows Try builds for us?

We could then narrow it down more. I'm a bit suspicious of part 11 because that seems to be the most complicated change.

Yes, I would like to do it.

Comment 7

[Jan de Mooij [:jandem]]

I was able to reproduce this with a VPN connected to Tokyo. I'm now waiting for some Try builds to bisect the patch stack.

A debug build doesn't fail any assertions.

Comment 8

[Jan de Mooij [:jandem]]

I bisected it to:

part 2: OK
part 6: Fails

I'll now trigger Try builds for parts 3-5...

Comment 9

[Jan de Mooij [:jandem]]

part 3 fails, so that seems to be the culprit.

Does that help, André? I could try to narrow it down more with more try pushes...

Comment 10

[Toshihiro Yamada]

I track down the route to throw the exception and I found the exception is appeared in a function "UpdateTestSuccessors" called at line 1073　(When commented out, game starts normally).

Comment 11

[Toshihiro Yamada]

Correct:
line 1073 of IonAnalysis.cpp

Comment 12

[André Bargull [:anba]]

Does that help, André? I could try to narrow it down more with more try pushes...

I think I still need some hints. Even after extensive staring at part 3, I still don't have a clue where things go awry.

Looking at code coverage to spot any code paths which aren't tested, I see that the IsTestInputMaybeToBool code paths aren't covered. Applies both for the triangle and the diamond pattern, because in IsTestInputMaybeToBool we never return true. But when I created some local tests to execute these code paths, I couldn't spot any obvious issues.

Comment 13

[Toshihiro Yamada]

More track down.
By changing line 767 of IonAnalysis.cpp to "return false", the problem has gone.

So, does UpdateTestSuccessors return "true" to the code should be returned "false" by some lack of condition check?

Comment 14

[André Bargull [:anba]]

I track down the route to throw the exception and I found the exception is appeared in a function "UpdateTestSuccessors" called at line 1073

I'm confused. Won't that lead to keeping an edge from initialBlock to phiBlock, which seems wrong, given that we later remove phiBlock?

Comment 15

[André Bargull [:anba]]

By changing line 767 of IonAnalysis.cpp to "return false", the problem has gone.

Returning false from that function will indicate to the caller that we've encountered an out-of-memory situation. This will eventually prevent JIT compilation and as a side-effect the error won't happen.

Comment 16

[Jan de Mooij [:jandem]]

I can track this down tomorrow.

Comment 17

[Toshihiro Yamada]

(In reply to André Bargull [:anba] [back from break; working through mail backlog] from comment #14)

I track down the route to throw the exception and I found the exception is appeared in a function "UpdateTestSuccessors" called at line 1073

I'm confused. Won't that lead to keeping an edge from initialBlock to phiBlock, which seems wrong, given that we later remove phiBlock?

(In reply to André Bargull [:anba] [back from break; working through mail backlog] from comment #15)

By changing line 767 of IonAnalysis.cpp to "return false", the problem has gone.

Returning false from that function will indicate to the caller that we've encountered an out-of-memory situation. This will eventually prevent JIT compilation and as a side-effect the error won't happen.

Thanks to the info and sorry to bother you.
I tried more and changing line 1121 line to "return false" also vanishes exception.
So, as you say, simply, when the code that throws exception wasn't be JIT compiled, no exceptions are thrown.
(Probably, not JIT compiled before Bug 1767966, too)

Comment 18

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #16)

I can track this down tomorrow.

Just thinking out load: LIRGenerator::visitCompare handles MCompare::Compare_RefOrNull, but this is after CanEmitCompareAtUses. And LIRGenerator::visitCompare doesn't handle MCompare::Compare_RefOrNull. This doesn't seem correct at first glance.

Comment 19

[Jan de Mooij [:jandem]]

Attachment: Shell test

Here's a Wasm shell testcase. It fails with --wasm-compiler=ion --no-threads but passes with Wasm Baseline.

I think the problem is here:

  if (phiBlock == trueBranch) {
    if (!UpdateTestSuccessors(graph.alloc(), initialBlock, initialTest->input(),
                              finalTest->ifTrue(), initialTest->ifFalse(),
                              testBlock)) {
      return false;
    }

In this case initialtest->input() != trueResult but we incorrectly use the test input.

Comment 20

[Jan de Mooij [:jandem]]

Attachment: Simpler shell test

Reduced it a bit more.

Comment 21

[André Bargull [:anba]]

Thanks a ton, I'd never figured this out myself!

Comment 22

[André Bargull [:anba]]

Attachment: Bug 1768346: Don't fold test block if the phi-operand doesn't match the initial test input. r=jandem!

Comment 23

[Toshihiro Yamada]

Confirmed the patch fixes the problem in my own build.

Comment 24

[André Bargull [:anba]]

(In reply to Toshihiro Yamada from comment #23)

Confirmed the patch fixes the problem in my own build.

Thanks for verifying the patch fixes the issue! :-)

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ce7ffebb6b7
Don't fold test block if the phi-operand doesn't match the initial test input. r=jandem

Comment 26

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/5ce7ffebb6b7