309242 - E4X should be on by default, while preserving the comment hiding hack

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

From correspondence with David.

/be
-----
Brendan Eich wrote:

> Thinking it through again, I see an earlier mistake that led to the
> version-selection requirement that does us no good in hiding from old
> browsers: again, the reason e4x=1 is required to enable E4X in full is to
> avoid breaking the HTML "comment hiding hack".
>
> But any E4X comment literal usage is unlikely to be the first thing (excluding
> whitespace) in a script tag. 

Good point! A script-hiding comment is useful only if it is the first token in
the script.  I suppose it is possible to construct examples where an XML literal
begins with a comment that is the first literal. But in common usage, any such
XML literal would be assigned to a variable first.  I think it would be safe to
disambiguate in this way.

Comment 1

[Brendan Eich [:brendan]]

Must-fix for 1.8b5/fx1.5 -- should be a small patch.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: bonus, optimize js_GetToken's line-dirtying a bit, by avoiding branch-tests

This reduces JSOPTION_XML to an option enabling scanning of <!-- as an XML
comment, instead of a "skip to end of line" marker for the old Netscape-2-era
HTML comment hiding hack.  That's it.  :: and @ are scanned, #if
JS_HAS_XML_SUPPORT and without any run-time option/version check.

mrbkap, this required pushing your automagic JSOPTION_XML enable/disable code
down into ParseXMLSource (which was a fix anyway, since it made 'l = new
XMLList(...)' auto-enable, as well as 'x = new XML(...)').

This motivated an overdue (or just-in-time!) change to track an E4X spec-fix as
it goes to ISO: ECMA-357 evaluates @a or *::b or * to undefined if the XML name
is not found in the scope chain!  See 11.1 3(a)(i).  The change to throw a
ReferenceError affects js.msg and js_FindXMLProperty's uses and definition.

I had made this a strict warning upon discovering it when implementing
ECMA-357. It's really a bug when you consider that, e.g., given no @b in the
scope chain, @b = 42 fails (as it should), but @b not on the left-hand side of
an assignment silently evaluates to undefined.

Bob, the testsuite may not cover the weird old behavior -- can you adjust if it
does, or add tests if it doesn't?  Thanks.

/be

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 196746 [details] [diff] [review]
bonus, optimize js_GetToken's line-dirtying a bit, by avoiding branch-tests

Argh, forgot to copy up new patch.

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: the right patch

Comment 5

[Jesse Ruderman]

This might break sites that use the "comment hiding hack" in unusual ways.  For
example, if a single script tag contains two <!-- --> blocks, Firefox currently
executes the scripts inside both blocks.

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #5)
> This might break sites that use the "comment hiding hack" in unusual ways.  For
> example, if a single script tag contains two <!-- --> blocks, Firefox currently
> executes the scripts inside both blocks.

Do you know of any real-world examples?

We could eliminate TSF_DIRTYINPUT and use only TSF_DIRTYLINE, but then valid E4X
code such as

    var a_very_very_very_very_very_very_very_long_name =
        <!-- an XML comment that did not fit on the above line -->;

would miscompile.

/be

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)

No, I have it!  We just need to clear TSF_DIRTYINPUT after a --> that matches a
non-E4X <!--, and we're back in the game!  Ah, compatibility.  Interdiff coming
up to avoid whole-patch redo (for now).

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: interdiff for last patch, as promised

Just review with this applied in your head, or in your tree!

/be

Comment 9

[Jesse Ruderman]

No, I don't know any real-world examples of sites that use the "comment hiding
hack" in unusual ways.

Comment 10

[Brendan Eich [:brendan]]

Attachment: better patch, handle adjacent HTML comments closed by // -->

Comment 11

[Mike Shaver (:shaver emeritus)]

Comment on attachment 196822 [details] [diff] [review]
better patch, handle adjacent HTML comments closed by // -->

sr=shaver if you switch to setting oldopts from JS_SetOptions' return value,
instead of directly peeking in cx.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 196822 [details] [diff] [review]
better patch, handle adjacent HTML comments closed by // -->

>Index: jsxml.c
> XML(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)

XMLList needs the same treatment.

r=mrbkap

Comment 13

[Brendan Eich [:brendan]]

mrbkap: thanks, sorry I doubted you without reading XMLList (but pushing the
auto-enable code down to the common sub-subroutine would have been righteous
even before this bug ;-).

Fixed on trunk.

/be

Comment 14

[Brendan Eich [:brendan]]

Attachment: testcase, could use some framework help

Comment 15

[Brendan Eich [:brendan]]

I'll attach a branch patch shortly.

/be

Comment 16

[Bob Clary [:bc] (inactive)]

(In reply to comment #14)
testcase+ if _you check the test case in_, testcase? if you need work on it and
need it to be checked in.

I need to gomb and set up an area for browser only tests which do not have a
place in the shell oriented test library before this can go it. Soonest.

Comment 17

[Brendan Eich [:brendan]]

Attachment: branch version of what's in the trunk

This is all a big deal for Firefox 1.5 and JavaScript 1.6 / E4X.  David
Flanagan (cc'd on this bug, author of the O'Reilly JS book) will be documenting
the way this all works.  It's baking on the trunk, but it should go into the
branch for b5, probably not at the last minute (or not when I'm in Estonia next
week, unless someone else does the honors).

/be

Comment 18

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch now.

/be

Comment 19

[Bob Clary [:bc] (inactive)]

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

Comment 20

[Bob Clary [:bc] (inactive)]

<http://test.bclary.com/tests/mozilla.org/js/js-test-driver-quirks.html?test=js1_6/Regress/regress-309242.js;language=type;text/javascript;version=1.5> passes in 1.8/trunk

<http://test.bclary.com/tests/mozilla.org/js/js-test-driver-quirks.html?test=js1_6/Regress/regress-309242.js;language=type;text/javascript;version=1.6> fails in 1.8/trunk.

Does version 1.6 automatically turn on e4x?

Comment 21

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.x and trunk.