Last Comment Bug 788293 - Remove E4X from SpiderMonkey
: Remove E4X from SpiderMonkey
Status: RESOLVED FIXED
[js:t]
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal with 3 votes (vote)
: mozilla21
Assigned To: Nicholas Nethercote [:njn]
:
Mentors:
Depends on: 1083470 778851 788290 833208 834090
Blocks: 836949
  Show dependency treegraph
 
Reported: 2012-09-04 14:35 PDT by Gary Kwong [:gkw] [:nth10sd]
Modified: 2014-10-15 14:05 PDT (History)
31 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
21+


Attachments
Disable JS_HAS_XML_SUPPORT in the browser. (711 bytes, patch)
2013-01-29 18:03 PST, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Review
Remove all e4x stuff that is outside of SpiderMonkey. (22.55 KB, patch)
2013-01-29 18:04 PST, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Review
Remove all traces of JS_HAS_XML_SUPPORT from SpiderMonkey. (423.93 KB, patch)
2013-01-29 18:05 PST, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Review
A jsinterp.cpp clean-up. (1.29 KB, patch)
2013-01-29 18:06 PST, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Review
Remove e4x parse nodes. (21.43 KB, patch)
2013-01-29 18:19 PST, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Review
Remove e4x tokens. (15.65 KB, patch)
2013-01-29 18:20 PST, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Review
Some parser clean-ups. (6.52 KB, patch)
2013-01-29 18:21 PST, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Review
Remove e4x from reflection. (14.77 KB, patch)
2013-01-29 18:22 PST, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Review
Remove e4x opcodes. (27.63 KB, patch)
2013-01-29 18:24 PST, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Review
Inline GetPropertyGenericMaybeCallXML(), which is now trivial. (2.37 KB, patch)
2013-01-29 18:25 PST, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Review
Remove getFunctionNamespace(). (1.02 KB, patch)
2013-01-29 18:26 PST, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Review
Remove e4x from the JSAPI. (24.42 KB, patch)
2013-01-29 18:27 PST, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Review
CommonPropertyNames clean-ups. (8.31 KB, patch)
2013-01-29 18:27 PST, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Review
Remove jsxml.{h,cpp}. (12.79 KB, patch)
2013-01-29 18:28 PST, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Review
Remove e4x classes support. (3.55 KB, patch)
2013-01-29 18:28 PST, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Review
Remove e4x error messages. (13.20 KB, patch)
2013-01-29 18:29 PST, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Review
Remove JOF_XMLNAME. (9.92 KB, patch)
2013-01-29 18:30 PST, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Review
Remove e4x GC stuff. (10.48 KB, patch)
2013-01-29 18:31 PST, Nicholas Nethercote [:njn]
terrence: review+
Details | Diff | Review
Remove e4x Unicode stuff. (2.81 KB, patch)
2013-01-29 18:31 PST, Nicholas Nethercote [:njn]
evilpies: review+
Details | Diff | Review
Remove remaining e4x bits and pieces. (13.40 KB, patch)
2013-01-29 18:32 PST, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Review

Description Gary Kwong [:gkw] [:nth10sd] 2012-09-04 14:35:49 PDT
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.
Comment 1 Luke Wagner [:luke] 2012-09-04 14:40:50 PDT
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.
Comment 2 David Mandelin [:dmandelin] 2012-09-04 18:43:10 PDT
(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!
Comment 3 Mads Bondo Dydensborg 2013-01-03 00:12:07 PST
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" :-)
Comment 4 Gary Kwong [:gkw] [:nth10sd] 2013-01-09 15:15:10 PST
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.
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2013-01-09 16:30:33 PST
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.
Comment 6 Mads Bondo Dydensborg 2013-01-10 03:57:23 PST
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.
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2013-01-22 14:47:18 PST
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.)
Comment 8 Mads Bondo Dydensborg 2013-01-22 23:35:44 PST
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
Comment 9 Nicholas Nethercote [:njn] 2013-01-29 16:19:03 PST
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.
Comment 10 Nicholas Nethercote [:njn] 2013-01-29 16:19:54 PST
> 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!
Comment 11 Nicholas Nethercote [:njn] 2013-01-29 18:03:37 PST
Created attachment 707925 [details] [diff] [review]
Disable JS_HAS_XML_SUPPORT in the browser.
Comment 12 Nicholas Nethercote [:njn] 2013-01-29 18:04:33 PST
Created attachment 707926 [details] [diff] [review]
Remove all e4x stuff that is outside of SpiderMonkey.
Comment 13 Nicholas Nethercote [:njn] 2013-01-29 18:05:27 PST
Created attachment 707928 [details] [diff] [review]
Remove all traces of JS_HAS_XML_SUPPORT from SpiderMonkey.

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.
Comment 14 Nicholas Nethercote [:njn] 2013-01-29 18:06:18 PST
Created attachment 707929 [details] [diff] [review]
A jsinterp.cpp clean-up.
Comment 15 Nicholas Nethercote [:njn] 2013-01-29 18:19:13 PST
Created attachment 707934 [details] [diff] [review]
Remove e4x parse nodes.

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.
Comment 16 Nicholas Nethercote [:njn] 2013-01-29 18:20:31 PST
Created attachment 707936 [details] [diff] [review]
Remove e4x tokens.
Comment 17 Nicholas Nethercote [:njn] 2013-01-29 18:21:40 PST
Created attachment 707938 [details] [diff] [review]
Some parser clean-ups.
Comment 18 Nicholas Nethercote [:njn] 2013-01-29 18:22:18 PST
Created attachment 707939 [details] [diff] [review]
Remove e4x from reflection.
Comment 19 Nicholas Nethercote [:njn] 2013-01-29 18:24:45 PST
Created attachment 707942 [details] [diff] [review]
Remove e4x opcodes.

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.
Comment 20 Nicholas Nethercote [:njn] 2013-01-29 18:25:22 PST
Created attachment 707944 [details] [diff] [review]
Inline GetPropertyGenericMaybeCallXML(), which is now trivial.
Comment 21 Nicholas Nethercote [:njn] 2013-01-29 18:26:11 PST
Created attachment 707946 [details] [diff] [review]
Remove getFunctionNamespace().
Comment 22 Nicholas Nethercote [:njn] 2013-01-29 18:27:23 PST
Created attachment 707947 [details] [diff] [review]
Remove e4x from the JSAPI.

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 23 Nicholas Nethercote [:njn] 2013-01-29 18:27:56 PST
Created attachment 707948 [details] [diff] [review]
CommonPropertyNames clean-ups.
Comment 24 Nicholas Nethercote [:njn] 2013-01-29 18:28:30 PST
Created attachment 707949 [details] [diff] [review]
Remove jsxml.{h,cpp}.
Comment 25 Nicholas Nethercote [:njn] 2013-01-29 18:28:58 PST
Created attachment 707950 [details] [diff] [review]
Remove e4x classes support.
Comment 26 Nicholas Nethercote [:njn] 2013-01-29 18:29:45 PST
Created attachment 707951 [details] [diff] [review]
Remove e4x error messages.
Comment 27 Nicholas Nethercote [:njn] 2013-01-29 18:30:20 PST
Created attachment 707952 [details] [diff] [review]
Remove JOF_XMLNAME.
Comment 28 Nicholas Nethercote [:njn] 2013-01-29 18:31:02 PST
Created attachment 707953 [details] [diff] [review]
Remove e4x GC stuff.
Comment 29 Nicholas Nethercote [:njn] 2013-01-29 18:31:33 PST
Created attachment 707954 [details] [diff] [review]
Remove e4x Unicode stuff.
Comment 30 Nicholas Nethercote [:njn] 2013-01-29 18:32:00 PST
Created attachment 707955 [details] [diff] [review]
Remove remaining e4x bits and pieces.
Comment 31 Terrence Cole [:terrence] 2013-01-29 18:54:40 PST
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/
Comment 32 Nicholas Nethercote [:njn] 2013-01-29 20:20:47 PST
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 33 Tom Schuster [:evilpie] 2013-01-30 09:51:01 PST
Comment on attachment 707954 [details] [diff] [review]
Remove e4x Unicode stuff.

Taking this one.
Comment 34 Tom Schuster [:evilpie] 2013-01-30 09:52:16 PST
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 35 Jason Orendorff [:jorendorff] 2013-01-30 16:43:35 PST
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 36 Jason Orendorff [:jorendorff] 2013-01-31 09:55:33 PST
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 37 Jason Orendorff [:jorendorff] 2013-01-31 10:03:12 PST
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 38 Jason Orendorff [:jorendorff] 2013-01-31 10:12:05 PST
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 39 Jason Orendorff [:jorendorff] 2013-01-31 10:18:59 PST
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 40 Jason Orendorff [:jorendorff] 2013-01-31 10:44:25 PST
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 41 Jason Orendorff [:jorendorff] 2013-01-31 10:53:09 PST
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 42 Jason Orendorff [:jorendorff] 2013-01-31 10:59:48 PST
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 43 :Ms2ger 2013-01-31 11:10:50 PST
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 44 Jason Orendorff [:jorendorff] 2013-01-31 11:13:18 PST
Comment on attachment 707952 [details] [diff] [review]
Remove JOF_XMLNAME.

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

decompiler_tco++
Comment 45 Jason Orendorff [:jorendorff] 2013-01-31 11:22:19 PST
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 46 :Ms2ger 2013-01-31 11:24:36 PST
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
Comment 47 :Ms2ger 2013-01-31 11:39:05 PST
Did you miss ValueIsSpecial/VersionFlagsToOptions?
Comment 48 Nicholas Nethercote [:njn] 2013-01-31 13:35:00 PST
> 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.
Comment 49 Nicholas Nethercote [:njn] 2013-01-31 14:54:03 PST
> >        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.
Comment 50 Nicholas Nethercote [:njn] 2013-01-31 14:57:03 PST
> ::: 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.
Comment 51 Nicholas Nethercote [:njn] 2013-01-31 14:59:41 PST
> decompiler_tco++

"Total Cost of Ownership"?
Comment 52 Nicholas Nethercote [:njn] 2013-01-31 15:46:51 PST
I filed bug 836949 for the follow-ups.
Comment 53 Nicholas Nethercote [:njn] 2013-01-31 15:48:44 PST
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.
Comment 54 Nicholas Nethercote [:njn] 2013-01-31 16:05:35 PST
Here's a try server run that uses |-p all| instead of |-p none|, sigh:

https://tbpl.mozilla.org/?tree=Try&rev=ca608715cd06
Comment 55 Nicholas Nethercote [:njn] 2013-01-31 21:07:58 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/66882ea6f8c7
Comment 56 Phil Ringnalda (:philor) 2013-01-31 22:35:35 PST
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a7f7309da69d - either this or bug 834090 broke pretty much everything.
Comment 57 Phil Ringnalda (:philor) 2013-01-31 23:46:05 PST
Relanded with a clobber in https://hg.mozilla.org/integration/mozilla-inbound/rev/c929583ba8ae
Comment 58 Ed Morley [:emorley] 2013-02-01 01:51:22 PST
Please can we adjust the CLOBBER file too?
Comment 59 :Ms2ger 2013-02-01 01:58:37 PST
(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
Comment 60 Ed Morley [:emorley] 2013-02-01 02:53:22 PST
Great :-)
Comment 62 Tom Schuster [:evilpie] 2013-02-04 08:18:44 PST
I am little bit late, but anyway. Great!
https://www.youtube.com/watch?v=WAQbRFZU7rE
Comment 63 Ben Bucksch (:BenB) 2013-02-04 10:33:26 PST
From the patch, I assume the (useful) "for each...in" construct stayed in?
Comment 64 Jeff Walden [:Waldo] (remove +bmo to email) 2013-02-04 11:01:34 PST
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.
Comment 65 Jesse Ruderman 2013-02-04 16:46:25 PST
For the fate of "for each", see bug 804834 and bugs mentioned there.
Comment 66 Kohei Yoshino [:kohei] 2013-02-23 23:31:46 PST
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
Comment 67 Tom Schuster [:evilpie] 2013-03-22 15:16:58 PDT
This has been documented in various places, at this point people who depend on E4X are probably aware or screwed.
Comment 68 Mads Bondo Dydensborg 2013-03-25 02:57:25 PDT
(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 ... ?
Comment 69 Ben Bucksch (:BenB) 2013-03-25 03:42:38 PDT
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.
Comment 70 Mads Bondo Dydensborg 2013-03-25 05:42:43 PDT
(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 ;-)
Comment 71 Ben Bucksch (:BenB) 2013-03-25 05:52:24 PDT
> 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.
Comment 72 Mads Bondo Dydensborg 2013-03-25 05:55:05 PDT
(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.
Comment 74 Nicholas Nethercote [:njn] 2013-10-05 18:40:43 PDT
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."

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