753885 - Add JSOPTION_ALLOW_XML (make E4X support optional per-context)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

The existing option JSOPTION_XML does precisely these things:
- turn on E4X even in JS language versions that don't normally have it
- treat <!-- as beginning an E4X comment, rather than ignoring the rest
  of that line (an old Netscape-era hack, de facto part of the Web)

The option we want is one that lets you disable E4X entirely. So in this bug we will split JSOPTION_XML into two parts:

    JSOPTION_ALLOW_XML  - you get no E4X at all without setting this
    JSOPTION_MOAR_XML   - what the existing JSOPTION_XML option does

I'll go through and add JSOPTION_ALLOW_XML in all the places where Gecko sets up JSContexts, so this will be no net change in behavior, except in the shell where E4X will now be disabled by default. You can turn it on by doing `options('allow_xml')`.

I'll also insert `options('allow_xml');` in js/src/tests/shell.js and the corresponding place for jit-test, so the tests will be basically unaffected.

Comment 1

[Jason Orendorff [:jorendorff]]

The first two "WIP 1" patches in bug 753542 actually belong in this bug.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: v2, part 1 - Rename JSOPTION_XML -> JSOPTION_MOAR_XML and other cleanups

This shouldn't affect behavior at all.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: v2, part 2 - Add JSOPTION_ALLOW_XML

Bumps the XDR version number because it changes the version field of scripts that allow XML (and I want this to matter slightly at runtime, see part 3).

All these patches contain some inelegant bits. However the pathetic comment in AutoVersionAPI isn't my fault:
>+        // For backward compatibility, AutoVersionAPI clobbers the
>+        // JSOPTION_MOAR_XML bit in cx, but not the JSOPTION_ALLOW_XML bit.

I think it's a bug for AutoVersionAPI to clobber the MOAR_XML bit, but I'd rather not change it now.

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: v2, part 3 - expose XML global constructors only if JSOPTION_ALLOW_XML is set

The goal is to make XML support preffable. This means if the pref is on, XML and XMLList and the rest have to be present as globals. But if the pref is off, we should try to make it look as close to !JS_HAS_XML_SUPPORT if possible, which means those names shouldn't exist.

Here is my ugly hack to do that.

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: v2.1, part 2 - Add JSOPTION_ALLOW_XML

The only change here is to make JSOPTION_ALLOW_XML on-by-default for xpcshell.

I don't like doing this. But there's an xpcshell-test involving mozJSSubScriptLoader that needs XML to be enabled very early. I stared at it for a while but couldn't figure out how to rewrite the test to make it work. This was a one-liner. Another option is to hack the xpcshell test-runner to add -e 'options("allow_xml");' on the command line. I'll be happy to do that if desired.

Comment 6

[Jason Orendorff [:jorendorff]]

Seriously, if all deleting E4X accomplished was getting rid of the mad, mad behavior of JS_SetOptions, it would be worth it.

Comment 7

[Nicholas Nethercote [inactive]]

(In reply to Jason Orendorff [:jorendorff] from comment #6)
> Seriously, if all deleting E4X accomplished was getting rid of the mad, mad
> behavior of JS_SetOptions, it would be worth it.

My understanding is that deleting E4X will, happily, accomplish a lot more than that.

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 624870 [details] [diff] [review]
v2, part 1 - Rename JSOPTION_XML -> JSOPTION_MOAR_XML and other cleanups

Review of attachment 624870 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/Parser.cpp
@@ +6538,4 @@
>          reportErrorNumber(NULL, JSREPORT_ERROR, JSMSG_SYNTAX_ERROR);
>          return NULL;
>      }
>      return (tt == TOK_AT) ? attributeIdentifier() : qualifiedIdentifier();

I'd kind of like it if you inverted this so that you had |if (allowsXML()) { ... } reportErrorNumber(...); ...|.  Fewer negatives reads nicer.

@@ +6992,5 @@
>              return NULL;
>          break;
>  
>        case TOK_XMLPI: {
> +        JS_ASSERT(allowsXML());

You know, this allowsXML() mini-abstraction is so much nicer than what was were doing before; wish anyone had thought of it before.

::: js/src/jsapi-tests/testVersion.cpp
@@ +188,5 @@
>      callbackData = this;
>  
>      /* Enable XML and compile a script to activate. */
>      enableXML();
>      const char toActivateChars[] =

You should make this static while you're in context here.

::: js/src/jsapi.h
@@ +2722,5 @@
>  #define JSOPTION_ATLINE         JS_BIT(5)       /* //@line number ["filename"]
>                                                     option supported for the
>                                                     XUL preprocessor and kindred
>                                                     beasts. */
> +#define JSOPTION_MOAR_XML       JS_BIT(6)       /* EMCAScript for XML support:

EMCAScript!  Looks like even ECMA (Ecma) (so it goes) disavows this stuff.  :-D

I admit to wondering if this isn't shades of COMPILE_N_GO, pc2line, and other cutesies.  Guess it doesn't matter.

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 624887 [details] [diff] [review]
v2, part 3 - expose XML global constructors only if JSOPTION_ALLOW_XML is set

Review of attachment 624887 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/tests/basic/bug753885-2.js
@@ +10,5 @@
> +var globals = Object.getOwnPropertyNames(this);
> +for (var name of xmlnames)
> +    assertEq(globals.indexOf(name), -1);
> +
> +var g = evalcx('');

newGlobal("new-compartment"), please.

@@ +14,5 @@
> +var g = evalcx('');
> +for (var name of xmlnames)
> +    assertEq(name in g, false);
> +
> +// Turn it back on and check that the XML classes magically appear.

This is so wrong it's not even funny, but whatever.  :-\

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 624951 [details] [diff] [review]
v2.1, part 2 - Add JSOPTION_ALLOW_XML

Review of attachment 624951 [details] [diff] [review]:
-----------------------------------------------------------------

Some of these options really should be scoped to the global, not to the context, or script, or parser, or whatever -- when you create the global, you say whether the E4X bindings are allowed, whether E4X syntax is allowed, and so on.  If they are allowed, then individual scripts' allowing E4X syntax would be a permissible frob to change underneath that.  What names get resolved/enumerated on the global is quintessential global-object configuration, not something that should depend on the phase of the context options moon.  We do have flags (well, flag right now) in a reserved slot on global objects that could store this.  Perhaps it's not worth it for this temporary hackage.  We should keep this in mind for any future configuration options that are global-scoped that we might ever consider.

::: js/src/jit-test/tests/basic/bug753885-1.js
@@ +1,5 @@
> +// XML syntax is not available unless JSOPTION_ALLOW_XML is enabled.
> +
> +load(libdir + "asserts.js");
> +
> +var exprs = ["<a/>", "x..y", "x.@id", "x.(@id === '13')", "x.*", "x.function::length"];

Add "function::parseInt" and "a::b" to cover the qualified-name-in-expression cases.

::: js/src/jsapi-tests/testVersion.cpp
@@ +266,5 @@
> +BEGIN_TEST(testVersion_AllowXML)
> +{
> +    JSBool ok;
> +
> +    const char *code = "var m = <x/>;";

static const char code[]

::: js/src/jsapi.cpp
@@ -139,5 @@
>  #endif
>      JSVersion   newVersion;
>  
>    public:
> -    explicit AutoVersionAPI(JSContext *cx, JSVersion newVersion)

Why this change?

::: js/src/jsapi.h
@@ +2722,5 @@
>  #define JSOPTION_ATLINE         JS_BIT(5)       /* //@line number ["filename"]
>                                                     option supported for the
>                                                     XUL preprocessor and kindred
>                                                     beasts. */
> +#define JSOPTION_ALLOW_XML      JS_BIT(6)       /* EMCAScript for XML support

EMCAScript!

@@ +2723,5 @@
>                                                     option supported for the
>                                                     XUL preprocessor and kindred
>                                                     beasts. */
> +#define JSOPTION_ALLOW_XML      JS_BIT(6)       /* EMCAScript for XML support
> +                                                   (deprecated) */

These two descriptions aren't all that clear.  (Not that it matters, perhaps, but still, we're going to have to deal with this code for at least several months in any case.)  They should very clearly speak to the presence of the E4X global bindings, E4X syntax in general, and the comment-hiding hack in particular, stating which controls what.  What's here is fairly vague, and I don't think the next person to look at this who hasn't reviewed any patches here is going to understanding it without asking us to explain ourselves.  :-\

::: js/src/vm/Xdr.h
@@ +56,5 @@
>   * aborts deserialization if there is a mismatch between the current
>   * and saved versions. If deserialization fails, the data should be
>   * invalidated if possible.
>   */
> +static const uint32_t XDR_BYTECODE_VERSION = uint32_t(0xb973c0de - 117);

Not that it matters, and this doubtless bitrotted, but you went 115->117 here.

Comment 11

[Nicholas Nethercote [inactive]]

> Some of these options really should be scoped to the global, not to the
> context, or script, or parser, or whatever -- when you create the global,
> you say whether the E4X bindings are allowed, whether E4X syntax is allowed,
> and so on.  If they are allowed, then individual scripts' allowing E4X
> syntax would be a permissible frob to change underneath that.  What names
> get resolved/enumerated on the global is quintessential global-object
> configuration, not something that should depend on the phase of the context
> options moon.  We do have flags (well, flag right now) in a reserved slot on
> global objects that could store this.  Perhaps it's not worth it for this
> temporary hackage.  We should keep this in mind for any future configuration
> options that are global-scoped that we might ever consider.

Assuming I've understood this comment correctly... the Parser is not a bad spot for global flags, at least, for ones that must be visible during parsing and bytecode compilation.  It's visible pretty much everywhere in the front-end.

Comment 12

[Jeff Walden [:Waldo]]

For the frontend queries, it makes sense to store that info in the script/parser/etc.  I'm not disputing that.

What I'm suggesting is that the JSContext is the wrong place to store info about whether the E4X global bindings exist on a global object.  That should be an aspect of the global object, not something determined by out-of-band information embedded in a (mutable, in this respect) JSContext.

But since all this is going away in the moderately near term anyway, it may well not be worth doing in the cosmically proper sense.  Thus the r+ all around (although I'm not sure, if this instead were living really long, that I'd actually r- the changes).

Comment 13

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #8)
> I admit to wondering if this isn't shades of COMPILE_N_GO, pc2line, and
> other cutesies.  Guess it doesn't matter.

Oh, it totally is. It's crazy.

(comment #12)
> What I'm suggesting is that the JSContext is the wrong place to store info about
> whether the E4X global bindings exist on a global object.

I think you might be right; but then non-compile-and-go scripts can be run against multiple globals. So IIUC the interpreter would have to be able to cope with E4X opcodes running against globals that can't have the XML machinery. I didn't want to get into it. The hacky way was less involved--and of course the end is to delete it all...

Thanks for the reviews. I will land this today.

Comment 14

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #10)
> > +    const char *code = "var m = <x/>;";
> 
> static const char code[]

I went ahead and made this change. But does it make any difference?

> >  
> >    public:
> > -    explicit AutoVersionAPI(JSContext *cx, JSVersion newVersion)
> 
> Why this change?

En passant. 'explicit' has no effect on a constructor unless it can be called with a single argument.

> > +#define JSOPTION_ALLOW_XML      JS_BIT(6)       /* EMCAScript for XML support
> 
> EMCAScript!

I know! lol

> > +#define JSOPTION_ALLOW_XML      JS_BIT(6)       /* EMCAScript for XML support
> > +                                                   (deprecated) */
> 
> These two descriptions aren't all that clear.  (Not that it matters,
> perhaps, but still, we're going to have to deal with this code for at least
> several months in any case.)  They should very clearly speak to the presence
> of the E4X global bindings, E4X syntax in general, and the comment-hiding
> hack in particular, stating which controls what.

OK. I changed the comment on JSOPTION_ALLOW_XML to specificaly say that it controls E4X syntax and globals.

The comment on JSOPTION_MOAR_XML already specifically mentions that it controls the comment-hiding hack.

Comment 15

[Jeff Walden [:Waldo]]

(In reply to Jason Orendorff [:jorendorff] from comment #14)
> > > +    const char *code = "var m = <x/>;";
> > 
> > static const char code[]
> 
> I went ahead and made this change. But does it make any difference?

A little: <http://glandium.org/blog/?p=2361>.  It's mostly cultivating good habits so bad habits don't sneak in by mistake, tho, in this case.

Comment 16

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/11c7f20bf242
https://hg.mozilla.org/integration/mozilla-inbound/rev/9be14c2b115e
https://hg.mozilla.org/integration/mozilla-inbound/rev/a780bb0de695

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/11c7f20bf242
https://hg.mozilla.org/mozilla-central/rev/9be14c2b115e
https://hg.mozilla.org/mozilla-central/rev/a780bb0de695