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

VERIFIED FIXED in mozilla1.8beta5

Status

()

Core
JavaScript Engine
P1
normal
VERIFIED FIXED
12 years ago
7 years ago

People

(Reporter: brendan, Assigned: brendan)

Tracking

({js1.6, verified1.8})

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

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

Comment 1

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

/be
Status: NEW → ASSIGNED
Flags: blocking1.8b5+
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta5
(Assignee)

Comment 2

12 years ago
Created attachment 196746 [details] [diff] [review]
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
Attachment #196746 - Flags: superreview?(shaver)
Attachment #196746 - Flags: review?(mrbkap)
(Assignee)

Comment 3

12 years ago
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
Attachment #196746 - Attachment is obsolete: true
Attachment #196746 - Flags: superreview?(shaver)
Attachment #196746 - Flags: review?(mrbkap)
(Assignee)

Comment 4

12 years ago
Created attachment 196747 [details] [diff] [review]
the right patch
Attachment #196747 - Flags: superreview?(shaver)
Attachment #196747 - Flags: review?(mrbkap)

Comment 5

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

Comment 6

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

Comment 7

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

Comment 8

12 years ago
Created attachment 196749 [details] [diff] [review]
interdiff for last patch, as promised

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

/be

Comment 9

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

Updated

12 years ago
Flags: testcase?
(Assignee)

Comment 10

12 years ago
Created attachment 196822 [details] [diff] [review]
better patch, handle adjacent HTML comments closed by // -->
Attachment #196747 - Attachment is obsolete: true
Attachment #196749 - Attachment is obsolete: true
Attachment #196822 - Flags: superreview?(shaver)
Attachment #196822 - Flags: review?(mrbkap)
(Assignee)

Updated

12 years ago
Attachment #196747 - Flags: superreview?(shaver)
Attachment #196747 - Flags: review?(mrbkap)
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.
Attachment #196822 - Flags: superreview?(shaver) → superreview+
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
Attachment #196822 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 13

12 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Summary: E4X should be on by default, whlie preserving the comment hiding hack → E4X should be on by default, while preserving the comment hiding hack
(Assignee)

Comment 14

12 years ago
Created attachment 196836 [details]
testcase, could use some framework help
(Assignee)

Comment 15

12 years ago
I'll attach a branch patch shortly.

/be
Flags: testcase? → testcase+

Comment 16

12 years ago
(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.
Flags: testcase+ → testcase?
(Assignee)

Comment 17

12 years ago
Created attachment 196854 [details] [diff] [review]
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
Attachment #196854 - Flags: superreview+
Attachment #196854 - Flags: review+
Attachment #196854 - Flags: approval1.8b5?

Updated

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

Comment 18

12 years ago
Fixed on the 1.8 branch now.

/be
Keywords: fixed1.8

Updated

12 years ago
Depends on: 309712
(Assignee)

Updated

12 years ago
Blocks: 310993

Comment 19

12 years ago
Checking in regress-309242.js;
/cvsroot/mozilla/js/tests/js1_6/Regress/regress-309242.js,v  <--  regress-309242.js
initial revision: 1.1
done
Flags: testcase? → testcase+

Comment 20

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

12 years ago
verified fixed 1.8.x and trunk.
Status: RESOLVED → VERIFIED
Keywords: fixed1.8 → verified1.8
Depends on: 563118
You need to log in before you can comment on or make changes to this bug.