As per https://groups.google.com/forum/#!topic/mozilla.dev.tech.js-engine/yYQyMCcMf-0/discussion , it is about time to completely remove E4X forever and ever. And ever.

We're a bit behind on that schedule. Realistically, it seems like we'd want to wait until the chrome=false setting had gotten to beta before dropping the bombs.

(In reply to Luke Wagner [:luke] from comment #1) > We're a bit behind on that schedule. Realistically, it seems like we'd want > to wait until the chrome=false setting had gotten to beta before dropping > the bombs. +1. But Gary, I like your spirit!

Any news if a replacement / transition tool / anything, will be provided by SpiderMonkey/Mozilla/Anyone? The information on https://developer.mozilla.org/en-US/docs/E4X about alternatives is really "short" :-)

Setting NEEDINFO from Naveed to see if we can make this for the current Firefox 21 window. E4X has been disabled in content and chrome now.

Mads, depends on the context. There won't be a tool to rewrite your E4X into JS, I'm sure of that. As for sprucing up the MDN docs, it'd be great to do it, but I'm not sure who has the most time to do it right now. I'd be willing, except I have a number of things in front of it (reviews, other patchwork, etc.) and don't have the time now. :-\ Gary, not that I don't want that, but it seems to me like we should wait for addon developers to start updating their extensions -- and not by just flipping the pref -- before we start targeting full removal for a release. Turned off does mean it's far less of a security concern (because only extensions that flip it on will be affected), so I'm not aware that there's particular reason to rush out removing it. Of course if incremental GC people (say) think otherwise, I'm happy to take that into account. :-) Although it cuts both ways, there, because if not enough addon authors have moved fast enough to do that, they're stuck waiting as well.

Jeff : OK, I understand. Are you aware of any groups of efforts outside of Mozilla that aims to provide a transition for E4X to "something"? We have quite a large codebase that does XML manipulation using embedded SpiderMonkeys and Rhinos, and are unsure about how to migrate (and to what) with as little problems/effort as possibly.

I'm not aware of outside efforts. We've talked to various groups that we'd previously known were using E4X, and we've been encouraging them to start switching over on their ends, and they've been doing so. More broadly, the removal has been discussed in the js-engine newsgroup for awhile, too. If you're not following mozilla.dev.tech.js-engine, you definitely should be, to hear about the latest notable SpiderMonkey API changes. (Although, I should note that the API changes that relate to rooting specifically -- making APIs take handles rather than raw pointers, and such -- haven't been discussed/announced every time they've happened, because that would just be a whole lot of announcing with little information content.)

Jeff : Thanks for your information. Do you have any other pointers (besides mozilla.dev.tech.js-engine) for groups using E4X? We would really like to learn what they do with respect to migrating to "something else". Thanks in advance. Mads

I'm going to upload a bunch of patches, separated to make reviewing easier. But I'm going to fold them together before landing, because I'm not sure if the intermediate states will be valid.

> Setting NEEDINFO from Naveed to see if we can make this for the current > Firefox 21 window. E4X has been disabled in content and chrome now. I've talked with dmandelin and Naveed; e4x is ok to be removed in FF21!

This patch: - Removes the setting of JS_HAS_XML_SUPPORT from configure.in. - Removes all the code that is guarded by JS_HAS_XML_SUPPORT, and *no other code*. There's plenty more to come in follow-up patches -- save your "you can remove/change X now" comments for later, please :) - Does a tiny bit of reformatting in some places around the removed code (e.g. fiddling with braces). It removes over 11,000 lines of code.

There's an XXX comment at the top of the patch that I'd like some feedback on. I tried removing that conjunct and jit-tests and jstests both passed.

At the top of this patch there's a commented-out assertion that I added because I thought it would succeed, but it fails regularly during the tests. Any suggestions about that? My favourite part of this patch was removing the opcodes whose only function was to help e4x decompilation.

The remaining version/compile option stuff is over-engineered with e4x gone. I want to keep the changes in this patch simple, so I'll look at that in a follow-up bug.

Comment on attachment 707953 [details] [diff] [review] Remove e4x GC stuff. Review of attachment 707953 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/TestingFunctions.cpp @@ +902,5 @@ > " 3: Collect when the window paints (browser only)\n" > " 4: Verify pre write barriers between instructions\n" > " 5: Verify pre write barriers between paints\n" > +" 6: Verify stack rooting\n" > +" 7: Verify stack rooting (yes, it's the same as 6)\n" Good idea! :-) ::: js/src/jscntxt.h @@ -810,5 @@ > - * Whether exact stack scanning is enabled for this runtime. This is > - * currently only used for dynamic root analysis. Exact scanning starts out > - * enabled, and is disabled if e4x has been used. > - */ > - bool gcExactScanningEnabled; \o/

I should mention: I did a try run on Mac that was green with these patches. I'll do an all-platforms run again before landing, once the reviews are addressed.

Comment on attachment 707954 [details] [diff] [review] Remove e4x Unicode stuff. Taking this one.

Comment on attachment 707954 [details] [diff] [review] Remove e4x Unicode stuff. This seems okay. But you adjust make_unicode.py and re-run it. This should reduce the table size. :)

Comment on attachment 707926 [details] [diff] [review] Remove all e4x stuff that is outside of SpiderMonkey. Review of attachment 707926 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsJSEnvironment.cpp @@ -1068,5 @@ > else > newDefaultJSOptions &= ~JSOPTION_WERROR; > > ::JS_SetOptions(context->mContext, > - newDefaultJSOptions & (JSRUNOPTION_MASK | JSOPTION_ALLOW_XML)); I remember adding this grossness, and thinking, "It's OK, we really are going to delete it."

Comment on attachment 707928 [details] [diff] [review] Remove all traces of JS_HAS_XML_SUPPORT from SpiderMonkey. Review of attachment 707928 [details] [diff] [review]: ----------------------------------------------------------------- r=me. ::: js/src/frontend/Parser.cpp @@ +5594,5 @@ > return NULL; > JS_ASSERT(tokenStream.currentToken().t_op == JSOP_NAME); > node->setOp(JSOP_NAME); > > + if ((!afterDoubleDot) && !pc->inDeclDestructuring) { Memo to myself: afterDoubleDot can't happen anymore, make sure it gets removed later in the stack. ::: js/src/jsobjinlines.h @@ +1797,2 @@ > return false; > } It's a beautiful thing. Memo to myself: this is obsolete too. ::: js/src/jsopcode.cpp @@ +2630,1 @@ > #define inXML JS_FALSE This too. ::: js/src/jsxml.cpp @@ +7,5 @@ > > #include <stddef.h> > #include "jsversion.h" > > size_t sE4XObjectsCreated = 0; And this file. ::: js/src/jsxml.h @@ +8,4 @@ > #define jsxml_h___ > > #include "jspubtd.h" > #include "jsobj.h" and this

Comment on attachment 707929 [details] [diff] [review] A jsinterp.cpp clean-up. Review of attachment 707929 [details] [diff] [review]: ----------------------------------------------------------------- Stealing review. Feel free to inline this everywhere it's used and do away with the macro. I don't think a comment is really necessary at all; ymmv.

Comment on attachment 707934 [details] [diff] [review] Remove e4x parse nodes. Review of attachment 707934 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/ParseNode.cpp @@ +177,5 @@ > stack->pushUnlessNull(pn->pn_kid); > break; > case PN_NULLARY: > + // XXX: with e4x gone, is the |!pn->isUsed()| still necessary? > + return /*!pn->isUsed() &&*/ !pn->isDefn(); Wait... can a nullary node be a definition? That seems crazy to me. Try returning true here, and if it pasts tests, r=me to land that. PN_NULLARY nodes do use the pn_u.name.atom slot sometimes, but they should never be uses or definitions. (Honestly I don't know if recycling parse nodes buys us anything worth having; maybe we'll tear all this out someday.) ::: js/src/frontend/ParseNode.h @@ -366,5 @@ > * pn_count: 1 + N (where N is number of args) > * ctor is a MEMBER expr > * PNK_DELETE unary pn_kid: MEMBER expr > * PNK_DOT, name pn_expr: MEMBER expr to left of . > - * PNK_DBLDOT pn_atom: name to right of . No, don't just remove that line. What we want it to say is: > * PNK_DOT name pn_expr: MEMBER expr to left of . > * pn_atom: name to right of . Note no comma after PNK_DOT.

Comment on attachment 707938 [details] [diff] [review] Some parser clean-ups. Review of attachment 707938 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/Parser.cpp @@ +5564,5 @@ > JS_ASSERT(tokenStream.currentToken().t_op == JSOP_NAME); > node->setOp(JSOP_NAME); > > + if (!pc->inDeclDestructuring && !NoteNameUse(node, this)) > + return NULL; Lately I've noticed some folks writing it this way if (condition) { if (!DoStuff()) return NULL; } and I think that's fine, in case you care. Totally up to you.

Comment on attachment 707942 [details] [diff] [review] Remove e4x opcodes. Review of attachment 707942 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BytecodeEmitter.cpp @@ +2027,5 @@ > right = NullaryNode::create(PNK_STRING, bce->parser); > if (!right) > return false; > + //JS_ASSERT(!IsIdentifier(pn->pn_atom)); // njn: hmm > + right->setOp(JSOP_STRING); Whaaaaaaaat. I'm guessing this was to do with the decompiler. Or else the opcode is completely ignored later on!

Comment on attachment 707947 [details] [diff] [review] Remove e4x from the JSAPI. Review of attachment 707947 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsapi.h @@ +3188,5 @@ > > #define JSOPTION_ION JS_BIT(20) /* IonMonkey */ > > /* Options which reflect compile-time properties of scripts. */ > +#define JSCOMPILEOPTION_MASK 0 Memo to myself: look for this to be removed too. ::: js/src/jscntxt.h @@ +1268,5 @@ > * become invalid. > */ > namespace VersionFlags { > static const unsigned MASK = 0x0FFF; /* see JSVersion in jspubtd.h */ > +static const unsigned FULL_MASK = 0x0FFF; and this @@ +1303,2 @@ > JS_ASSERT((copts & JSCOMPILEOPTION_MASK) == copts); > return copts; and this @@ +1306,5 @@ > > static inline JSVersion > OptionFlagsToVersion(unsigned options, JSVersion version) > { > + return version; and this ::: js/src/jsiter.cpp @@ +151,3 @@ > return false; > } > } Remove the curly braces around 'return false;'?

Comment on attachment 707949 [details] [diff] [review] Remove jsxml.{h,cpp}. Review of attachment 707949 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsprototypes.h @@ -56,5 @@ > - macro(Proxy, 34, js_InitProxyClass) \ > - macro(AnyName, 35, js_InitNullClass) \ > - macro(WeakMap, 36, js_InitWeakMapClass) \ > - macro(Map, 37, js_InitMapClass) \ > - macro(Set, 38, js_InitSetClass) \ I spent an embarrassingly long time staring at this list trying to figure out what the fourth thing was that got removed. :) Got it. Cool.

Comment on attachment 707926 [details] [diff] [review] Remove all e4x stuff that is outside of SpiderMonkey. Review of attachment 707926 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsFrameMessageManager.cpp @@ +1132,2 @@ > JS_SetOptions(cx, JS_GetOptions(cx) | > + JSOPTION_PRIVATE_IS_NSISUPPORTS); One line, please. ::: dom/base/nsGlobalWindow.cpp @@ -1936,2 @@ > JSObject* outer = NewOuterWindowProxy(cx, aNewInner->FastGetGlobalJSObject(), > isChrome); Inline the IsChromeWindow() call? ::: dom/base/nsJSEnvironment.cpp @@ +1059,5 @@ > else > newDefaultJSOptions &= ~JSOPTION_WERROR; > > ::JS_SetOptions(context->mContext, > + newDefaultJSOptions & JSRUNOPTION_MASK); One line

Comment on attachment 707952 [details] [diff] [review] Remove JOF_XMLNAME. Review of attachment 707952 [details] [diff] [review]: ----------------------------------------------------------------- decompiler_tco++

Comment on attachment 707955 [details] [diff] [review] Remove remaining e4x bits and pieces. Review of attachment 707955 [details] [diff] [review]: ----------------------------------------------------------------- Nice work, njn. Lightening the load is always a pleasure. ::: js/src/jsinterp.cpp @@ +2136,3 @@ > * There must be an object value below the id, which will not be popped > + * (XXX with e4x, this was necessary for interning the id for XML; is it > + * still necessary now that e4x is gone?) This opcode is emitted for code like: a[obj]++; The standard requires us to call obj.toString() only once. (Technically all that's needed is a TOSTRING opcode--converting the resulting string to a jsid is side-effect-free, so we can do it twice without observable effect. But there's no reason to want to.) ::: js/src/vm/Shape-inl.h @@ +320,5 @@ > if (!self->getUserId(cx, &id)) > return false; > > /* > + * |with (it) color;| ends up here. Huh. That can't be right. It must mean: |with (it) color='red';| ...Right? (could set a breakpoint here and see if it's actually reached for that code...)

Comment on attachment 707928 [details] [diff] [review] Remove all traces of JS_HAS_XML_SUPPORT from SpiderMonkey. Review of attachment 707928 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/TestingFunctions.cpp @@ +156,2 @@ > if (!JS_SetProperty(cx, info, "e4x", &value)) > return false; Do we still need this? ::: js/src/frontend/BytecodeCompiler.cpp @@ +157,5 @@ > bce.objectList.add(funbox); > } > } > > ParseNode *pn; Consider declaring this variable in the body of the loop

Did you miss ValueIsSpecial/VersionFlagsToOptions?

> Did you miss ValueIsSpecial/VersionFlagsToOptions? Waldo specifically asked that I leave all the Special stuff in. It's currently unused(?) but might be needed in the future if Harmony-proposed private names (whatever they are) are implemented. I'll deal with the version stuff in a follow-up. See comment 22.

> > case PN_NULLARY: > > + // XXX: with e4x gone, is the |!pn->isUsed()| still necessary? > > + return /*!pn->isUsed() &&*/ !pn->isDefn(); > > Wait... can a nullary node be a definition? That seems crazy to me. > > Try returning true here, and if it pasts tests, r=me to land that. I'll do it in a follow-up, just to be careful. (Don't worry, I'm writing down all my follow-ups!) > (Honestly I don't know if recycling parse nodes buys us anything worth > having; maybe we'll tear all this out someday.) I've done measurements that show we can easily have many 10s of MiBs of parse nodes. And they're insidious: they're short-lived, so they can cause problems (e.g. OOM) but they don't show up much in about:memory. So please be careful if you do this.

> ::: js/src/frontend/BytecodeEmitter.cpp > @@ +2027,5 @@ > > right = NullaryNode::create(PNK_STRING, bce->parser); > > if (!right) > > return false; > > + //JS_ASSERT(!IsIdentifier(pn->pn_atom)); // njn: hmm > > + right->setOp(JSOP_STRING); > > Whaaaaaaaat. > > I'm guessing this was to do with the decompiler. Or else the opcode is > completely ignored later on! I'll take that to mean I should just remove the commented-out assertion.

> decompiler_tco++ "Total Cost of Ownership"?

I filed bug 836949 for the follow-ups.

Thanks for the all reviews, jorendorff. I've made all the changes and started a try server run: https://tbpl.mozilla.org/?tree=Try&rev=991fbf59ec25 Once that's done we can nuke this sucker from orbit.

Here's a try server run that uses |-p all| instead of |-p none|, sigh: https://tbpl.mozilla.org/?tree=Try&rev=ca608715cd06

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a7f7309da69d - either this or bug 834090 broke pretty much everything.

Relanded with a clobber in https://hg.mozilla.org/integration/mozilla-inbound/rev/c929583ba8ae

Please can we adjust the CLOBBER file too?

(In reply to Ed Morley [:edmorley UTC+0] from comment #58) > Please can we adjust the CLOBBER file too? https://hg.mozilla.org/integration/mozilla-inbound/rev/c6e5bf0eb51a

Great :-)

https://hg.mozilla.org/mozilla-central/rev/c929583ba8ae https://hg.mozilla.org/mozilla-central/rev/c6e5bf0eb51a

I am little bit late, but anyway. Great! https://www.youtube.com/watch?v=WAQbRFZU7rE

From the patch, I assume the (useful) "for each...in" construct stayed in?

It did for now. ES6 may introduce (and SpiderMonkey has already implemented) a for-of construct with roughly similar semantics, which in the long run may overtake for-each. But it's over a year til that's something we could even consider requiring developers to switch to (until all supported, stable releases support it -- and we'd want to add in time until ES6 finalizes as well). So it's not worth worrying about right now.

For the fate of "for each", see bug 804834 and bugs mentioned there.

I've added this bug to the compatibility doc. Please correct the info if I'm wrong. https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_21

This has been documented in various places, at this point people who depend on E4X are probably aware or screwed.

(In reply to Tom Schuster [:evilpie] from comment #67) > This has been documented in various places, at this point people who depend > on E4X are probably aware or screwed. Possibly aware *and* screwed ... ?

I have been depending heavily on E4X, in various projects. I found JXON to be an almost direct replacement. You need to make a few small changes, e.g. foo.@href -> foo["@href"], but there are other things where JXON can even be better than E4X, esp. when you take my version of it from bug 759422 comment 4. With this version, the JXON-based code ended up being better than the E4X-based one.

(In reply to Ben Bucksch (:BenB) from comment #69) > I have been depending heavily on E4X, in various projects. I found JXON to > be an almost direct replacement. You need to make a few small changes, e.g. > foo.@href -> foo["@href"], but there are other things where JXON can even be > better than E4X, esp. when you take my version of it from bug 759422 comment > 4. With this version, the JXON-based code ended up being better than the > E4X-based one. Thanks. We did look at JXON, but the namespace support seems limited (and, we use namespaces *a lot*) and also our issue is not so much representing the XML, as manipulating it. We have a lot of serverside javascript that uses E4X to manipulate XML, and there seems to be no easy way to "translate" this into using JXON or JSON/BadgerFish or similar. We will manage, and this bug should not degenerate into a discussion about this - but we did feel a bit "left behind" at some point ;-)

> JXON, but the namespace support seems limited There's no namespace support at all, but I added it in my local copy. I'll update the public version.

(In reply to Ben Bucksch (:BenB) from comment #71) > > JXON, but the namespace support seems limited > > There's no namespace support at all, but I added it in my local copy. I'll > update the public version. Thank you! That would be great, and a step towards a solution based on JXON for us. Thanks.

As promised: http://download.beonex.com/component/JXON.js http://download.beonex.com/component/JXON.txt

From http://www.theguardian.com/world/2013/oct/04/tor-attacks-nsa-users-online-anonymity: "According to the training presentation provided by Snowden, EgotisticalGiraffe exploits a type confusion vulnerability in E4X, which is an XML extension for Javascript. This vulnerability exists in Firefox 11.0 – 16.0.2, as well as Firefox 10.0 ESR – the Firefox version used until recently in the Tor browser bundle. According to another document, the vulnerability exploited by EgotisticalGiraffe was inadvertently fixed when Mozilla removed the E4X library with the vulnerability, and when Tor added that Firefox version into the Tor browser bundle, but NSA were confident that they would be able to find a replacement Firefox exploit that worked against version 17.0 ESR."