Last Comment Bug 309242 - E4X should be on by default, while preserving the comment hiding hack
: E4X should be on by default, while preserving the comment hiding hack
Status: VERIFIED FIXED
: js1.6, verified1.8
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.8beta5
Assigned To: Brendan Eich [:brendan]
:
Mentors:
Depends on: 309712 563118
Blocks: 310993
  Show dependency treegraph
 
Reported: 2005-09-19 20:18 PDT by Brendan Eich [:brendan]
Modified: 2010-05-29 01:35 PDT (History)
7 users (show)
brendan: blocking1.8b5+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
bonus, optimize js_GetToken's line-dirtying a bit, by avoiding branch-tests (7.68 KB, patch)
2005-09-19 22:10 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
the right patch (12.60 KB, patch)
2005-09-19 22:15 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
interdiff for last patch, as promised (805 bytes, patch)
2005-09-19 22:40 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
better patch, handle adjacent HTML comments closed by // --> (15.92 KB, patch)
2005-09-20 13:22 PDT, Brendan Eich [:brendan]
mrbkap: review+
shaver: superreview+
Details | Diff | Splinter Review
testcase, could use some framework help (1.30 KB, text/html)
2005-09-20 14:58 PDT, Brendan Eich [:brendan]
no flags Details
branch version of what's in the trunk (17.08 KB, patch)
2005-09-20 17:09 PDT, Brendan Eich [:brendan]
brendan: review+
brendan: superreview+
mtschrep: approval1.8b5+
Details | Diff | Splinter Review

Description Brendan Eich [:brendan] 2005-09-19 20:18:18 PDT
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] 2005-09-19 20:22:44 PDT
Must-fix for 1.8b5/fx1.5 -- should be a small patch.

/be
Comment 2 Brendan Eich [:brendan] 2005-09-19 22:10:56 PDT
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
Comment 3 Brendan Eich [:brendan] 2005-09-19 22:11:28 PDT
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] 2005-09-19 22:15:30 PDT
Created attachment 196747 [details] [diff] [review]
the right patch
Comment 5 Jesse Ruderman 2005-09-19 22:18:56 PDT
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] 2005-09-19 22:29:13 PDT
(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] 2005-09-19 22:36:40 PDT
(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] 2005-09-19 22:40:31 PDT
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 Jesse Ruderman 2005-09-19 23:04:43 PDT
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] 2005-09-20 13:22:15 PDT
Created attachment 196822 [details] [diff] [review]
better patch, handle adjacent HTML comments closed by // -->
Comment 11 Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-09-20 14:04:45 PDT
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) 2005-09-20 14:09:47 PDT
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] 2005-09-20 14:49:09 PDT
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] 2005-09-20 14:58:03 PDT
Created attachment 196836 [details]
testcase, could use some framework help
Comment 15 Brendan Eich [:brendan] 2005-09-20 14:58:51 PDT
I'll attach a branch patch shortly.

/be
Comment 16 Bob Clary [:bc:] 2005-09-20 16:38:15 PDT
(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] 2005-09-20 17:09:08 PDT
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
Comment 18 Brendan Eich [:brendan] 2005-09-21 13:59:47 PDT
Fixed on the 1.8 branch now.

/be
Comment 19 Bob Clary [:bc:] 2005-10-09 17:15:47 PDT
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 21 Bob Clary [:bc:] 2006-04-09 21:20:40 PDT
verified fixed 1.8.x and trunk.

Note You need to log in before you can comment on or make changes to this bug.