Closed Bug 309242 Opened 15 years ago Closed 15 years ago

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

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.8beta5

People

(Reporter: brendan, Assigned: brendan)

References

Details

(Keywords: js1.6, verified1.8)

Attachments

(3 files, 3 obsolete files)

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.
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
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)
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)
Attached patch the right patch (obsolete) — Splinter Review
Attachment #196747 - Flags: superreview?(shaver)
Attachment #196747 - Flags: review?(mrbkap)
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.
(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
(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
Just review with this applied in your head, or in your tree!

/be
No, I don't know any real-world examples of sites that use the "comment hiding
hack" in unusual ways.
Flags: testcase?
Attachment #196747 - Attachment is obsolete: true
Attachment #196749 - Attachment is obsolete: true
Attachment #196822 - Flags: superreview?(shaver)
Attachment #196822 - Flags: review?(mrbkap)
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+
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
Closed: 15 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
I'll attach a branch patch shortly.

/be
Flags: testcase? → testcase+
(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?
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?
Attachment #196854 - Flags: approval1.8b5? → approval1.8b5+
Fixed on the 1.8 branch now.

/be
Keywords: fixed1.8
Depends on: 309712
Blocks: 310993
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+
verified fixed 1.8.x and trunk.
Status: RESOLVED → VERIFIED
Keywords: fixed1.8verified1.8
Depends on: 563118
You need to log in before you can comment on or make changes to this bug.