310993 - HTML comment on JS if() causes erroneous results

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Neil MacLeod]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051003 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051003 Firefox/1.6a1

Lloyds TSB (major UK bank) has the following code on their internet banking site:

  if (document.StatementSearch.selectbox.value == 5 ) <!--no longer using
selectedindex, so no support for NN 4.7 -->
  {                                              
      // do something
      return false;
  }

The inclusion of the HTML comment causes the if() to always evaluate to TRUE no
matter what value is selected. The Lloyds site functions normally with IE and
the following build of FF 1.5:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4

In addition the JS console flags the line containing the if() and HTML comment
as containing a "useless expression".

Test case will follow.

Reproducible: Always

Steps to Reproduce:
1. Add an HTML comment to an if() statement in JS
2.
3.
Actual Results:  
if() is always true

Expected Results:  
execute the if() correctly

Comment 1

[Kevin Brosnan]

Related to bug 276912

Comment 2

[Neil MacLeod]

Attachment: if() always evaluates to true if followed by an HTML comment

It seems that at least one other JS regression (see bug 310927) has appeared in
recent 1.6 builds - this concerns me as these bugs need to be fixed before 1.5
is released as JS regressions will result in sites being unusable with FF.

My companies web site is broken by bug 310927, and this bug has broken another
corporate web site. Cheers, Neil.

Comment 3

[Neil MacLeod]

(In reply to comment #1)
> Related to bug 276912

Hi Kevin - I've been using Lloyds TSB (personal) online banking for about 2
years with FF without any significant issues.

However, this bug (310993) is an outright bug and it's just a coincidence I
found it on the Lloyds TSB site (I initially reported it as an error on the
Lloyds site as it had recently been updated but after regression testing it is
clear the fault lies with FF 1.6a1, I hope I haven't wasted too much of their time!)

This error will break any site that includes an html comment on the same line as
a Javascript if() statement, whereas this has not been the case prior to FF 1.6a1.

Comment 4

[Phil Ringnalda (:philor)]

Attachment: minimal testcase

Comment 5

[Phil Ringnalda (:philor)]

Confirmed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5)
Gecko/20051003 Firefox/1.4.1 ID:2005100307 (thus the blocking? flag) but I
haven't yet checked whether there are unlanded HTML comment patches that cover this.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

I bet we're treating the comment as a lonely XML comment (and thus a statement),
so this is the moral equivalent of: if (false); something (note the stray ;).

I'm not sure what we want to do here.

Comment 7

[Neil MacLeod]

(In reply to comment #6)
> I'm not sure what we want to do here.

My vote is that we revert to the original (and IMHO correct) behaviour, because
that's the way it's been up until 1.6a1 and IE (and possibly other browsers)
treat the html/xml comment as a comment (which it is) and thus evaluate the if()
condition correctly.

If this defect is left as it now is (ie. if() always evaluates to true) then FF
will needlessly break sites that it never did before.

Comment 8

[Neil MacLeod]

Blake, you're probably correct that the if() is being evaluated as s single
statement due to the comment, and the code that follows is unconditionally
executed since the following testcase will throw a JS syntaxt error suggesting
that "else" is invalid:

<script>
if (false) <!-- oops -->
{
  alert("false is true");
}
else
{
  alert("false is not true");
}
</script>

Comment 9

[Brendan Eich [:brendan]]

Yes, we are not backward-compatible here.  E4X auto-enabling counts on something
like the current heuristic, but this bug wants one that is not merely lexical,
but syntactic.  I'll see what can be done.

/be

Comment 10

[Brendan Eich [:brendan]]

Attachment: proposed change

The INHTMLCOMMENT => IN_HTML_COMMENT renaming is just to match recent TSF_
flags, whose names are too long to run together as in the old daze.

The rest is straightforward parser=>scanner feedback, using the new
TSF_START_STATEMENT flag.  Any <! sequence starting where a statement may start
is on its face useless as an E4X comment or CDATA literal, so is treated as an
HTML comment.

This means if someone does something like

    var x = <!-- dumbass
	42;

they will feel the wrath of E4X, whereas before their code "worked".  I'm fully
prepared to be disappointed again, but I'd bet no page worth worrying about has
been so badly written.

/be

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #1)
> Related to bug 276912

Not in this case.  The regression from E4X auto-detection (see bug 309242) was
recent and unrelated to any older bug to do with this site.

/be

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 198417 [details] [diff] [review]
proposed change

Phew. Can I get a t-shirt that says: "JavaScript is not SGML"?

r=mrbkap

Comment 13

[Phil Ringnalda (:philor)]

Double link to https://bugzilla.mozilla.org/show_bug.cgi?id=309242 in the
comment in jsscan.c - did you mean for one of them to go to another bug?

Comment 14

[Brendan Eich [:brendan]]

(In reply to comment #13)
> Double link to https://bugzilla.mozilla.org/show_bug.cgi?id=309242 in the
> comment in jsscan.c - did you mean for one of them to go to another bug?

D'oh, yes -- thanks for catching this.  I meant to link to this bug in the first
line.  Fixed for checkin.

/be

Comment 15

[Brendan Eich [:brendan]]

(In reply to comment #14)
> (In reply to comment #13)
> > Double link to https://bugzilla.mozilla.org/show_bug.cgi?id=309242 in the
> > comment in jsscan.c - did you mean for one of them to go to another bug?
> 
> D'oh, yes -- thanks for catching this.  I meant to link to this bug in the first
> line.

Er, in the last line (bug number ascending order).  Late, must check in soon and
sleep....

/be

Comment 16

[Brendan Eich [:brendan]]

Fixed on trunk.  Approving for branch, we can't be breaking UK banks in the name
of E4X, even if they use HTML comments in the middle of JS for no good reason.

/be

Comment 17

[Brendan Eich [:brendan]]

Fixed on branch and trunk -- thanks.

Any pages still broken are WONTFIX ;-).

/be

Comment 18

[Brendan Eich [:brendan]]

This patch caused disaster (bug 311071).

/be

Comment 19

[Brendan Eich [:brendan]]

Actually, this is fixed.  But what I wrote in comment 17, "Any pages still
broken are WONTFIX ;-)." -- I take it back.

There's just no way to allow <! to start anything other than an HTML script tag
content comment hiding hack.  See bug 311071 for the patch to make that the case.

/be

Comment 20

[Brendan Eich [:brendan]]

(In reply to comment #19)
> There's just no way to allow <! to start anything other than an HTML script tag
> content comment hiding hack.  See bug 311071 for the patch to make that the
> case.

To be precise, <! followed immediately by -- starts an HTML comment hack to end
of line, used for ten years to hide script content from script-unaware browsers.

Yeah, this means legal JS such as

  var a = 0, x = 1;
  if (a<!--x)
    alert("a is less than the logical not of --x");

has never worked.  That's the cost of the comment-hiding hack I perpetrated over
ten years ago.  People shouldn't crunch such operators together, and anyway the
combination is very unlikely.

This is all for posterity.  It probably ought to be written up on devmo.

/be

Comment 21

[Yuh-Ruey Chen]

(In reply to comment #20)
> To be precise, <! followed immediately by -- starts an HTML comment hack to end
> of line, used for ten years to hide script content from script-unaware browsers.
> 
> Yeah, this means legal JS such as
> 
>   var a = 0, x = 1;
>   if (a<!--x)
>     alert("a is less than the logical not of --x");
> 
> has never worked.  That's the cost of the comment-hiding hack I perpetrated over
> ten years ago.  People shouldn't crunch such operators together, and anyway the
> combination is very unlikely.
> 
> This is all for posterity.  It probably ought to be written up on devmo.
> 
> /be
> 

Does that apply to XHTML as well? It seems that XHTML allows comment hiding
within CDATA section:

<script type="text/javascript"><![CDATA[
   var a = 0, x = 1;
   if (a<!--x)
     alert("a is less than the logical not of --x");
]]></script>

Above code has the same error. Or is that a symptom of E4X?

Comment 22

[Brendan Eich [:brendan]]

(In reply to comment #21)
> Does that apply to XHTML as well? It seems that XHTML allows comment hiding
> within CDATA section:

SpiderMonkey allows comment hiding, it doesn't matter how the script is embedded
or loaded.  It's legal even in out of line scripts, you know, with src=foo.js --
and people do put one-line <!-- and //--> (and probably even --> without the //
to protect it) in those files!  (The unprotected --> swallowing is an IE-ism we
have emulated.  The rest goes back to the dawn of time.)

XHTML has no implicit CDATA containers, and if you have to write CDATA marked
sections, you are not worried about hiding inline script content from pre-JS
browsers.  If you are writing XHTML, you are not worried about pre-JS browsers,
or even current browsers if you care about IE.

> <script type="text/javascript"><![CDATA[
>    var a = 0, x = 1;
>    if (a<!--x)
>      alert("a is less than the logical not of --x");
> ]]></script>
> 
> Above code has the same error. Or is that a symptom of E4X?

It's the way SpiderMonkey's extended lexer works.  It has nothing to do with
anything but the HTML comment hiding hack being enabled independent of how the
script was embedded or sourced.

/be

Comment 23

[Bob Clary [:bc] (inactive)]

Checking in regress-310993.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-310993.js,v  <--  regress-310993.js
initial revision: 1.1
done