Closed Bug 1882386 Opened 5 months ago Closed 5 months ago

FF 123.0 broke my GWT app

Categories

(Core :: JavaScript Engine: JIT, defect)

Firefox 123
Desktop
Windows 10
defect

Tracking

()

RESOLVED FIXED
125 Branch
Tracking Status
firefox-esr115 --- unaffected
firefox123 + fixed
firefox124 + fixed
firefox125 + fixed

People

(Reporter: horia.muntean, Assigned: anba)

References

(Regression)

Details

(Keywords: regression)

Attachments

(8 files)

Attached image 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.

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.

Component: Untriaged → General
Product: Firefox → Fenix
OS: Unspecified → Windows 10
Product: Fenix → Firefox
Hardware: Unspecified → Desktop
Attached image why-false.png

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.

Attached image 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

(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.

Attached image for-loop.png

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)

Attached image ff.png

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).

Component: General → JavaScript Engine
Product: Firefox → Core

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

Keywords: regression
Regressed by: 1873042
Component: JavaScript Engine → JavaScript Engine: JIT

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

For more information, please visit BugBot documentation.

Flags: needinfo?(andrebargull)
Flags: needinfo?(andrebargull)
Regressed by: 1864323
No longer regressed by: 1873042

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

And I reproduced it with a plain html file:

Thanks for the reduced test case!

Assignee: nobody → andrebargull
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

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

Pushed by andre.bargull@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/df746d46aeaa
Correctly handle empty search string case when inlining String.p.lastIndexOf. r=jandem

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

Flags: needinfo?(andrebargull)

(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.

Flags: needinfo?(andrebargull)

(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!

Attachment #9388734 - Flags: approval-mozilla-beta?

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.
Attachment #9388736 - Flags: approval-mozilla-release?

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.
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → 125 Branch
Attachment #9388734 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9388736 - Flags: approval-mozilla-release? → approval-mozilla-release+

Hi,

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

Thanks!

(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!

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: