Bug 1767966 breaks https://pc-play.games.dmm.com/play/monmusutd/ with javascript error
Categories
(Core :: JavaScript Engine: JIT, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr91 | --- | unaffected |
firefox100 | --- | unaffected |
firefox101 | --- | unaffected |
firefox102 | --- | fixed |
People
(Reporter: yamadat501, Assigned: anba)
References
(Blocks 2 open bugs, Regression, )
Details
Attachments
(2 files, 1 obsolete file)
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•2 years ago
|
||
Set release status flags based on info from the regressing bug 1767966
Assignee | ||
Comment 2•2 years ago
|
||
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.
Assignee | ||
Comment 3•2 years ago
|
||
Hey Jan, what do you think is the best way to investigate this issue further?
Comment 4•2 years ago
|
||
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•2 years ago
|
||
Maybe I can test this with a VPN. I'll try that in a bit...
Reporter | ||
Comment 6•2 years ago
|
||
(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•2 years ago
|
||
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.
Updated•2 years ago
|
Updated•2 years ago
|
Comment 8•2 years ago
|
||
I bisected it to:
part 2: OK
part 6: Fails
I'll now trigger Try builds for parts 3-5...
Comment 9•2 years ago
|
||
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...
Reporter | ||
Comment 10•2 years ago
|
||
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).
Reporter | ||
Comment 11•2 years ago
|
||
Correct:
line 1073 of IonAnalysis.cpp
Assignee | ||
Comment 12•2 years ago
•
|
||
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.
Reporter | ||
Comment 13•2 years ago
|
||
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?
Assignee | ||
Comment 14•2 years ago
|
||
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
?
Assignee | ||
Comment 15•2 years ago
|
||
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•2 years ago
|
||
I can track this down tomorrow.
Reporter | ||
Comment 17•2 years ago
|
||
(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
tophiBlock
, which seems wrong, given that we later removephiBlock
?
(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)
Assignee | ||
Comment 18•2 years ago
|
||
(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•2 years ago
|
||
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.
Assignee | ||
Comment 21•2 years ago
|
||
Thanks a ton, I'd never figured this out myself!
Assignee | ||
Comment 22•2 years ago
|
||
Updated•2 years ago
|
Reporter | ||
Comment 23•2 years ago
|
||
Confirmed the patch fixes the problem in my own build.
Assignee | ||
Comment 24•2 years ago
|
||
(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•2 years ago
|
||
Pushed by andre.bargull@gmail.com: 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•2 years ago
|
||
bugherder |
Description
•