1882386 - FF 123.0 broke my GWT app

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[horia.muntean]

Attachment: number-format.png

Steps to reproduce:

I have a GWT (Google Web Toolkit) app that suddenly breaks after upgrading to FF 123.0; it worked with no problems until this version.

Actual results:

The NumberFormat code started to yield strage results breaking all numeric fields validations.

Expected results:

Work without issues. All other browsers - Edge, Opera, Chrome, Safari work with no issues.

I will attach a print-screen from a debug session where one can see the same function (java_lang_String_$endsWith__Ljava_lang_String_2Ljava_lang_String_2Z) being called twice with the same arguments ("10", "") but giving two different results (one call gotPositiveSuffix=true, gotNegativeSuffix=false).

The app itself is not available to the public but I can provide more private info if needed.

Comment 1

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

The Bugbug bot thinks this bug should belong to the 'Fenix::General' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[horia.muntean]

Attachment: why-false.png

Comment 3

[horia.muntean]

I have added some log lines to "function java_lang_String_$endsWith__Ljava_lang_String_2Ljava_lang_String_2Z" which is the one that seems to produce the problem and I am posting the results below in an image (why-false.png). One can see that when the function is called with args "500" and "" it returns false (that is not correct); this is strange because if I run the return expression in the console I get true (which is correct).

For the reference, the modified function is

function java_lang_String_$endsWith__Ljava_lang_String_2Ljava_lang_String_2Z(this$static, suffix){
   console.log(typeof this$static);
    console.log(this$static);
    console.log(suffix);
    console.log(this$static.lastIndexOf(suffix) != -1 && this$static.lastIndexOf(suffix) == this$static.length - suffix.length);

  return this$static.lastIndexOf(suffix) != -1 && this$static.lastIndexOf(suffix) == this$static.length - suffix.length;
}

I did try to see how it behave in a separate file and it returns correct results; so how is it possible that the same function yields different results with the same parameters when run in different context.

Comment 4

[jackyzy823]

Attachment: image.png

Sorry, i couldn't reproduce on Firefox 123.0

---

Step I'm using

1. Create a demo application via ./gwt-2.11.0/webAppCreator -out test org.example.test
2. Edit src/org/example/client/test.java , add import com.google.gwt.i18n.client.NumberFormat; and in onModuleLoad function add double val = NumberFormat.getDecimalFormat().parse("10"); nameField.setText("GWT User: " + NumberFormat.getDecimalFormat().format(val)); after final TextBox nameField = new TextBox();
3. Edit build.xml add
   <arg line="-optimize"/> <arg value="0"/> <arg line="-saveSource"/> <arg line="-XmethodNameDisplayMode"/> <arg value="FULL"/> <arg line="-style"/> <arg value="DETAILED"/> in gwtc section ,just after <arg value="war"/>
4. Package with ant war
5. Deploy generated war file into Tomcat 9.
6. Visit the url and debug test-0.js

Comment 5

[horia.muntean]

(In reply to jackyzy823 from comment #4)

Created attachment 9388052 [details]
image.png

Sorry, i couldn't reproduce on Firefox 123.0

---

Step I'm using

1. Create a demo application via ./gwt-2.11.0/webAppCreator -out test org.example.test
2. Edit src/org/example/client/test.java , add import com.google.gwt.i18n.client.NumberFormat; and in onModuleLoad function add double val = NumberFormat.getDecimalFormat().parse("10"); nameField.setText("GWT User: " + NumberFormat.getDecimalFormat().format(val)); after final TextBox nameField = new TextBox();
3. Edit build.xml add
   <arg line="-optimize"/> <arg value="0"/> <arg line="-saveSource"/> <arg line="-XmethodNameDisplayMode"/> <arg value="FULL"/> <arg line="-style"/> <arg value="DETAILED"/> in gwtc section ,just after <arg value="war"/>
4. Package with ant war
5. Deploy generated war file into Tomcat 9.
6. Visit the url and debug test-0.js

I tried your suggestion also with gwt-2.5.1 and can't reproduce it either in such a simple project; the generated javascript for the function is quite different in gwt-2.11.0 vs gwt-2.5.1 but is the same in the app with the problem and this simple one generated with gwt-2.5.1 yet in one environment it returns a different result when called with the same input;

Any suggestion about what can I do to advance with this issue would be appreciated.
The same project worked for years in all major browsers, only this last version of Firefox has this issue.

Comment 6

[horia.muntean]

Attachment: for-loop.png

Comment 7

[horia.muntean]

I was able to reproduce it with gwt-2.5.1 !
Follow the steps from above but call parse in a loop - probably some kind of optimization kicks in ?

    for (int i = 0; i < 1000; i++) {
      double val = NumberFormat.getDecimalFormat().parse("10"); 
      nameField.setText("GWT User: " + NumberFormat.getDecimalFormat().format(val));
    }

What I get in the browser console iss

This page is in Quirks Mode. Page layout may be impacted. For Standards Mode use “<!DOCTYPE html>”. blank
GET
http://127.0.0.1:3002/favicon.ico
[HTTP/1.1 404 Not Found 3ms]

This page is in Quirks Mode. Page layout may be impacted. For Standards Mode use “<!DOCTYPE html>”. 060F808CA325307F2BEBAC7BC2106E19.cache.html
Uncaught TypeError: stackMessage is undefined
    handleError http://127.0.0.1:3002/___vscode_livepreview_injected_script:233
    <anonymous> http://127.0.0.1:3002/___vscode_livepreview_injected_script:14
    EventListener.handleEvent* http://127.0.0.1:3002/___vscode_livepreview_injected_script:14
___vscode_livepreview_injected_script:233:20
Uncaught 
Object { java_lang_Throwable_detailMessage: "10 does not have either positive or negative affixes" }
060F808CA325307F2BEBAC7BC2106E19.cache.html:446:7
    com_google_gwt_core_client_impl_Impl_entry__Lcom_google_gwt_core_client_JavaScriptObject_2Lcom_google_gwt_core_client_JavaScriptObject_2 http://127.0.0.1:3002/war/test/060F808CA325307F2BEBAC7BC2106E19.cache.html:446
    gwtOnLoad http://127.0.0.1:3002/war/test/060F808CA325307F2BEBAC7BC2106E19.cache.html:7244
    maybeStartModule http://127.0.0.1:3002/war/test/test.nocache.js:40
    onScriptLoad http://127.0.0.1:3002/war/test/test.nocache.js:278
    <anonymous> http://127.0.0.1:3002/war/test/060F808CA325307F2BEBAC7BC2106E19.cache.html:7252

I attached an screen shot (for-loop.png)

Comment 8

[horia.muntean]

Attachment: ff.png

Comment 9

[horia.muntean]

And I reproduced it with a plain html file:

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <title>String EndsWith Function Test</title>
    <script>
        function endsWith(text, suffix){
            return text.lastIndexOf(suffix) != -1 && text.lastIndexOf(suffix) == text.length - suffix.length;
        }

        function callFunctionInLoop() {
            for (let i = 0; i < 1000; i++) {
                var res = endsWith("500", "");
                console.log(res);
            }
        }
    </script>
</head>
<body>

<button onclick="callFunctionInLoop()">Test EndsWith Function</button>

</body>
</html>

If one presses "Test EndsWith Function" button for the first time, one will get 11 true returns and 989 false returns !!! Next button pushes will make the function return only false.

Attached a screen shot (ff.png).

Comment 10

[Tom S [:evilpie]]

4:43.04 INFO: Last good revision: 1cd3cffeda1bbc916f13235acd07d9c70e6c5aca
4:43.04 INFO: First bad revision: a45a5a1a1c7f7ccf50275176de827f2a9f1c9776
4:43.04 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=1cd3cffeda1bbc916f13235acd07d9c70e6c5aca&tochange=a45a5a1a1c7f7ccf50275176de827f2a9f1c9776

Comment 11

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

:anba, since you are the author of the regressor, bug 1873042, could you take a look?

For more information, please visit BugBot documentation.

Comment 12

[André Bargull [:anba]]

(In reply to horia.muntean from comment #9)

And I reproduced it with a plain html file:

Thanks for the reduced test case!

Comment 13

[André Bargull [:anba]]

Attachment: Bug 1882386: Correctly handle empty search string case when inlining String.p.lastIndexOf. r=jandem!

Comment 14

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

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df746d46aeaa
Correctly handle empty search string case when inlining String.p.lastIndexOf. r=jandem

Comment 16

[Pascal Chevrel:pascalc]

André, would that be a safe uplift for our planned dot release on Tuesday or should we target 124? Thanks

Comment 17

[André Bargull [:anba]]

(In reply to Pascal Chevrel:pascalc from comment #16)

André, would that be a safe uplift for our planned dot release on Tuesday or should we target 124? Thanks

Yes, I think this is safe enough for the dot release.

Comment 18

[Pascal Chevrel:pascalc]

(In reply to André Bargull [:anba] from comment #17)

(In reply to Pascal Chevrel:pascalc from comment #16)

André, would that be a safe uplift for our planned dot release on Tuesday or should we target 124? Thanks

Yes, I think this is safe enough for the dot release.

Could you ask an uplift for beta and release please? We build the dot release on Monday. Thanks!

Comment 19

[André Bargull [:anba]]

Attachment: Bug 1882386: Correctly handle empty search string case when inlining String.p.lastIndexOf. r=jandem!

Original Revision: https://phabricator.services.mozilla.com/D203248

Comment 20

[Phabricator Automation]

Uplift Approval Request

• String changes made/needed: None
• Explanation of risk level: Low risk because it only changes the return value for the JIT code path of "lastIndexOf" when the search string is the empty string.
• Risk associated with taking this patch: Low risk
• Fix verified in Nightly: no
• Needs manual QE test: no
• Is Android affected?: yes
• Code covered by automated testing: yes
• Steps to reproduce for manual QE testing: None
• User impact if declined: Wrong result for String.prototype.lastIndexOf after JIT-inlining when the search string is the empty string.

Comment 21

[André Bargull [:anba]]

Attachment: Bug 1882386: Correctly handle empty search string case when inlining String.p.lastIndexOf. r=jandem!

Original Revision: https://phabricator.services.mozilla.com/D203248

Comment 22

[Phabricator Automation]

Uplift Approval Request

• Risk associated with taking this patch: Low risk
• Needs manual QE test: no
• Fix verified in Nightly: no
• Explanation of risk level: Low risk because it only changes the return value for the JIT code path of "lastIndexOf" when the search string is the empty string.
• String changes made/needed: None
• Code covered by automated testing: yes
• Steps to reproduce for manual QE testing: None
• Is Android affected?: yes
• User impact if declined: Wrong result for String.prototype.lastIndexOf after JIT-inlining when the search string is the empty string.

Comment 23

[Sandor Molnar[:smolnar]]

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

Comment 24

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/42f38e140473

Comment 25

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-release/rev/f4b373e01f53

Comment 26

[horia.muntean]

Hi,

I can confirm FF 123.0.1 fixed the issue, the GWT app works fine now.

Thanks!

Comment 27

[André Bargull [:anba]]

(In reply to horia.muntean from comment #26)

Hi,

I can confirm FF 123.0.1 fixed the issue, the GWT app works fine now.

Thanks!

Thank you for confirming the update fixed this issue!