176125 - Page rendering shows "true" (or "false", or even blank) after JavaScript submit "refused" with alert...

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Serge Gautherie (:sgautherie)]

User-Agent:       Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.2b) Gecko/20021016
Build Identifier: Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.2b) Gecko/20021016

I give the login URL; the problem is the same inside the site at least.


Reproducible: Always

Steps to Reproduce:
1. Go to specified URL.
2. Click on "Valider".
3. Acknowledge the dialog box (which says that your number is wrong)

Actual Results:  
*View Frame Info says that the URL is "javascript:if
(testform01(document.form01) == true) {document.form01.submit();}"

*View Frame Source is blank.

*Mozilla shows "true" instead of the (previous) frame.


Expected Results:  
Supposedly:
The page should not have changed;
And I should be able to "correct" the wrong number in the fields.


I think this bug did not exist before v1.2b !?

Workaround:
Press "Back" to return to the correct frame.

Comment 1

[Boris Zbarsky [:bzbarsky]]

to JS engine.... What should the return value of that "if" be?  If it's supposed
to be "true" or "false" then the behavior is correct...

Comment 2

[Serge Gautherie (:sgautherie)]

"Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2b) Gecko/20021016"

Same on W2K.

*****

In reply to AC#1:
I don't want to try and go into details;
But there must be a bug either in the page (unlikely ?) or in Mozilla (.!.),
because I do doubt anyone would want a page which displays "true" as its body.

As it looks like to me, the "javascript:..." is "also" taken as a URL when it
should have been evaluated only.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Wrong.  It _should_ also be taken as a url if it does not evaluate to void. 
Many pages depend on that behavior.

So if if() statements should have void return value this is a Mozilla bug. 
Otherwise the page is buggy.  It's that simple.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: testcase

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK.  That seems very wrong....  Over to JS engine (I thought I did that the
first time!).

Comment 6

[Phil Schwartau]

The behavior of this changed between RC1 and RC2 of JS1.5.
Here is a variation of Boris' testcase for the JS shell:

js> function test1() {return 'Hi there!';}
js> function test2() {test1(); return false;}


In RC1 (03 Mar 2000), no return value on either |if| clause:
js> if(test1());   
js> if(test2());   


In RC2 (10 Aug 2000) and onwards, a return value on |if (test2());|
js> if(test1());   
js> if(test2());   
Hi there!


From RC2 onwards, it doesn't matter whether test2() returns |true|,
|false|, or has no return value at all. You always get the return value
of test1() when you do |if (test2());| interactively in the JS shell.

Comment 7

[Phil Schwartau]

Note: JS1.5 RC2 also had this behavior:

js> function test() {'Hi there!';}
js> if (test());
Hi there!


i.e. it wasn't necessary to have a nested function as in the
examples above. This simple manifestation of the bug is gone
(as of RC3), but we still have the nested one given above -

Comment 8

[Brendan Eich [:brendan]]

Attachment: proposed fix

This broke due to inline function calls (see inline_call and inline_return in
js_Interpret), but the fix is to build on the useless expression elimination
logic in jsemit.c, so that even if we have to emit an expression statement just
in case eval or similar is calling (eval('if (cond) true; else false') returns
the last popped expression statement value), we can avoid generating the
result-setting JSOP_POPV in favor of JSOP_POP if we're compiling a function
(whether light- or heavyweight).

Testing needed.  I single out shaver for review duties.  This should go into
1.2final, it's a nice small fix.

/be

Comment 9

[Brendan Eich [:brendan]]

Have fix, taking bug.

/be

Comment 10

[Mike Shaver (:shaver emeritus)]

Comment on attachment 103952 [details] [diff] [review]
proposed fix

Makes sense.  How did we not notice this before?  Shame unto me, at least.

r=shaver.

Comment 11

[Phil Schwartau]

The patch fixes the problem, and causes no test regressions.
Ran the entire testsuite on it in the debug/opt shells on WinNT -

Comment 12

[Brendan Eich [:brendan]]

Attachment: slightly better name: wantval (plus, bigger diff -u for more context)

This is what I'll check in, with a= from drivers, for 1.2final.

/be

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 104039 [details] [diff] [review]
slightly better name: wantval (plus, bigger diff -u for more context)

Carrying over shaver's r=.

/be

Comment 14

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 104039 [details] [diff] [review]
slightly better name: wantval (plus, bigger diff -u for more context)

a=dbaron for trunk checkin

Comment 15

[Brendan Eich [:brendan]]

Attachment: proposed 1.0 branch patch

Trunk patch is in.

I'd like to get this in for the 1.0 branch and anything releasing off of it
soon (1.0.2, commercial products based on 1.0.2).

/be

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 104040 [details] [diff] [review]
proposed 1.0 branch patch

shaver still loves this patch!

/be

Comment 17

[tor]

Comment on attachment 104040 [details] [diff] [review]
proposed 1.0 branch patch

a=tor for branch checkin

Comment 18

[Brendan Eich [:brendan]]

Fixed on the 1.0 branch, too.

I still hope to upgrade that branch to a more final 1.5 version of SpiderMonkey,
but that'll happen after 1.0.2.

/be

Comment 19

[Phil Schwartau]

Marking Verified Fixed. 

In the JS shell built from both trunk and -rMOZILLA_1_0_BRANCH,
the bug has been fixed, as observed above in Comment #11.

Comment 20

[Serge Gautherie (:sgautherie)]

"Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.2) Gecko/20021126"

Bug fixed on the test case of my initial report :-)
(Very fast and good job done !)

Comment 21

[Serge Gautherie (:sgautherie)]

Learning how to use BugZilla:
Could I mark this bug as "Closed" (see comment 20), or should it remain V.D.
untill some condition is met ?
Thanks.

Comment 22

[Boris Zbarsky [:bzbarsky]]

It should remain.

Comment 23

[Phil Schwartau]

Serge: thanks. We leave it in the "Verified Fixed" state for the 
time being. Bugs remain in this state until the product ships,
at which time they are marked CLOSED.

I found that explanation at http://www.mozilla.org/bugs

Comment 24

[Serge Gautherie (:sgautherie)]

I too read that definition: by clicking on Status :->

You mean, in this bug case, that V.F. remains because the "Target Milestone" is
v1.0.2 (future) rather than v1.2 (already shipped) !?

Thanks for your explanation.

Comment 25

[Phil Schwartau]

Only certain people have the "right" to mark bugs closed. By looking
through Bugzilla, you will see that bugs remain in a Verified state
for a long time before that happens. It's not just this bug; it is
a Bugzilla policy -

Comment 26

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/Regress/regress-176125.js,v  <--  regress-176125.js
initial revision: 1.1