HTML comment on JS if() causes erroneous results

RESOLVED FIXED in mozilla1.8beta5

Status

()

Core
JavaScript Engine
P1
normal
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Neil MacLeod, Assigned: brendan)

Tracking

({fixed1.8, js1.6})

Trunk
mozilla1.8beta5
fixed1.8, js1.6
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b5 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

12 years ago
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

12 years ago
Related to bug 276912
(Reporter)

Comment 2

12 years ago
Created attachment 198365 [details]
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.
(Reporter)

Comment 3

12 years ago
(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.
Created attachment 198373 [details]
minimal testcase
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.
Assignee: nobody → general
Component: General → JavaScript Engine
Flags: blocking1.8b5?
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → Trunk
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.
(Reporter)

Comment 7

12 years ago
(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.
(Reporter)

Comment 8

12 years ago
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>
(Assignee)

Updated

12 years ago
Assignee: general → brendan
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 9

12 years ago
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
Status: NEW → ASSIGNED
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.8beta5
(Assignee)

Updated

12 years ago
Depends on: 309242
(Assignee)

Comment 10

12 years ago
Created attachment 198417 [details] [diff] [review]
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
Attachment #198417 - Flags: review?(mrbkap)
Attachment #198417 - Flags: approval1.8b5?
(Assignee)

Comment 11

12 years ago
(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 on attachment 198417 [details] [diff] [review]
proposed change

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

r=mrbkap
Attachment #198417 - Flags: review?(mrbkap) → review+
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?
(Assignee)

Comment 14

12 years ago
(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
(Assignee)

Comment 15

12 years ago
(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
(Assignee)

Comment 16

12 years ago
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
Flags: blocking1.8b5? → blocking1.8b5+

Updated

12 years ago
Attachment #198417 - Flags: approval1.8b5? → approval1.8b5+
(Assignee)

Comment 17

12 years ago
Fixed on branch and trunk -- thanks.

Any pages still broken are WONTFIX ;-).

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8, js1.6
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Blocks: 311071
(Assignee)

Comment 18

12 years ago
This patch caused disaster (bug 311071).

/be
No longer blocks: 311071
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

12 years ago
Blocks: 311071
(Assignee)

Comment 19

12 years ago
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
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 20

12 years ago
(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

12 years ago
(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?
(Assignee)

Comment 22

12 years ago
(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

12 years ago
Checking in regress-310993.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-310993.js,v  <--  regress-310993.js
initial revision: 1.1
done
Flags: testcase+
You need to log in before you can comment on or make changes to this bug.