Remove E4X from SpiderMonkey

RESOLVED FIXED in mozilla21

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: gkw, Assigned: njn)

Tracking

(Depends on: 1 bug, {dev-doc-complete})

Trunk
mozilla21
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 21+)

Details

(Whiteboard: [js:t])

Attachments

(20 attachments)

711 bytes, patch
jorendorff
: review+
Details | Diff | Splinter Review
22.55 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
423.93 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
1.29 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
21.43 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
15.65 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
6.52 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
14.77 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
27.63 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
2.37 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
1.02 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
24.42 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
8.31 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
12.79 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
3.55 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
13.20 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
9.92 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
10.48 KB, patch
terrence
: review+
Details | Diff | Splinter Review
2.81 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
13.40 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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

5 years ago
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!
(Reporter)

Updated

5 years ago
Depends on: 778851, 788290
Whiteboard: [js:t]
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" :-)
(Reporter)

Comment 4

5 years ago
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.
Flags: needinfo?(nihsanullah)
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.
(Assignee)

Updated

5 years ago
Depends on: 833208
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.)
(Assignee)

Updated

5 years ago
Depends on: 833668
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
(Assignee)

Updated

5 years ago
QA Contact: general → n.nethercote
(Assignee)

Updated

5 years ago
No longer depends on: 833668
(Assignee)

Comment 9

5 years ago
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.
(Assignee)

Comment 10

5 years ago
> 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!
Flags: needinfo?(nihsanullah)
(Assignee)

Updated

5 years ago
Depends on: 834090
(Assignee)

Comment 11

5 years ago
Created attachment 707925 [details] [diff] [review]
Disable JS_HAS_XML_SUPPORT in the browser.
Attachment #707925 - Flags: review?(jorendorff)
(Assignee)

Updated

5 years ago
Assignee: general → n.nethercote
(Assignee)

Comment 12

5 years ago
Created attachment 707926 [details] [diff] [review]
Remove all e4x stuff that is outside of SpiderMonkey.
Attachment #707926 - Flags: review?(jorendorff)
(Assignee)

Comment 13

5 years ago
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.
Attachment #707928 - Flags: review?(jorendorff)
(Assignee)

Comment 14

5 years ago
Created attachment 707929 [details] [diff] [review]
A jsinterp.cpp clean-up.
Attachment #707929 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 15

5 years ago
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.
Attachment #707934 - Flags: review?(jorendorff)
(Assignee)

Comment 16

5 years ago
Created attachment 707936 [details] [diff] [review]
Remove e4x tokens.
Attachment #707936 - Flags: review?(jorendorff)
(Assignee)

Comment 17

5 years ago
Created attachment 707938 [details] [diff] [review]
Some parser clean-ups.
Attachment #707938 - Flags: review?(jorendorff)
(Assignee)

Comment 18

5 years ago
Created attachment 707939 [details] [diff] [review]
Remove e4x from reflection.
Attachment #707939 - Flags: review?(jorendorff)
(Assignee)

Comment 19

5 years ago
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.
Attachment #707942 - Flags: review?(jorendorff)
(Assignee)

Comment 20

5 years ago
Created attachment 707944 [details] [diff] [review]
Inline GetPropertyGenericMaybeCallXML(), which is now trivial.
Attachment #707944 - Flags: review?(jorendorff)
(Assignee)

Comment 21

5 years ago
Created attachment 707946 [details] [diff] [review]
Remove getFunctionNamespace().
Attachment #707946 - Flags: review?(jorendorff)
(Assignee)

Comment 22

5 years ago
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.
Attachment #707947 - Flags: review?(jorendorff)
(Assignee)

Comment 23

5 years ago
Created attachment 707948 [details] [diff] [review]
CommonPropertyNames clean-ups.
Attachment #707948 - Flags: review?(jorendorff)
(Assignee)

Comment 24

5 years ago
Created attachment 707949 [details] [diff] [review]
Remove jsxml.{h,cpp}.
Attachment #707949 - Flags: review?(jorendorff)
(Assignee)

Comment 25

5 years ago
Created attachment 707950 [details] [diff] [review]
Remove e4x classes support.
Attachment #707950 - Flags: review?(jorendorff)
(Assignee)

Comment 26

5 years ago
Created attachment 707951 [details] [diff] [review]
Remove e4x error messages.
Attachment #707951 - Flags: review?(jorendorff)
(Assignee)

Comment 27

5 years ago
Created attachment 707952 [details] [diff] [review]
Remove JOF_XMLNAME.
Attachment #707952 - Flags: review?(terrence)
(Assignee)

Comment 28

5 years ago
Created attachment 707953 [details] [diff] [review]
Remove e4x GC stuff.
Attachment #707953 - Flags: review?(terrence)
(Assignee)

Comment 29

5 years ago
Created attachment 707954 [details] [diff] [review]
Remove e4x Unicode stuff.
Attachment #707954 - Flags: review?(jorendorff)
(Assignee)

Comment 30

5 years ago
Created attachment 707955 [details] [diff] [review]
Remove remaining e4x bits and pieces.
Attachment #707955 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

5 years ago
Attachment #707952 - Flags: review?(terrence) → review?(jorendorff)
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/
Attachment #707953 - Flags: review?(terrence) → review+
(Assignee)

Comment 32

5 years ago
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.
Attachment #707954 - Flags: review?(jorendorff) → review?(evilpies)
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. :)
Attachment #707954 - Flags: review?(evilpies) → review+
Attachment #707925 - Flags: review?(jorendorff) → review+
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."
Attachment #707926 - Flags: review?(jorendorff) → review+
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
Attachment #707928 - Flags: review?(jorendorff) → review+
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.
Attachment #707929 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #707934 - Flags: review?(jorendorff) → review+
Attachment #707936 - Flags: review?(jorendorff) → review+
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.
Attachment #707938 - Flags: review?(jorendorff) → review+
Attachment #707939 - Flags: review?(jorendorff) → review+
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!
Attachment #707942 - Flags: review?(jorendorff) → review+
Attachment #707944 - Flags: review?(jorendorff) → review+
Attachment #707946 - Flags: review?(jorendorff) → review+
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;'?
Attachment #707947 - Flags: review?(jorendorff) → review+
Attachment #707948 - Flags: review?(jorendorff) → review+
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.
Attachment #707949 - Flags: review?(jorendorff) → review+
Attachment #707950 - Flags: review?(jorendorff) → review+
Attachment #707951 - Flags: review?(jorendorff) → review+
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++
Attachment #707952 - Flags: review?(jorendorff) → review+
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...)
Attachment #707955 - Flags: review?(jwalden+bmo) → review+
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?
(Assignee)

Comment 48

4 years ago
> 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.
(Assignee)

Comment 49

4 years ago
> >        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.
(Assignee)

Comment 50

4 years ago
> ::: 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.
(Assignee)

Comment 51

4 years ago
> decompiler_tco++

"Total Cost of Ownership"?
(Assignee)

Updated

4 years ago
Blocks: 836949
(Assignee)

Comment 52

4 years ago
I filed bug 836949 for the follow-ups.
No longer blocks: 836949
(Assignee)

Updated

4 years ago
Blocks: 836949
(Assignee)

Comment 53

4 years ago
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.
(Assignee)

Comment 54

4 years ago
Here's a try server run that uses |-p all| instead of |-p none|, sigh:

https://tbpl.mozilla.org/?tree=Try&rev=ca608715cd06
(Assignee)

Comment 55

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/66882ea6f8c7
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
(Assignee)

Updated

4 years ago
QA Contact: n.nethercote
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
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
I am little bit late, but anyway. Great!
https://www.youtube.com/watch?v=WAQbRFZU7rE

Updated

4 years ago
Keywords: dev-doc-needed

Comment 63

4 years ago
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.

Comment 65

4 years ago
For the fate of "for each", see bug 804834 and bugs mentioned there.
relnote-firefox: --- → ?

Updated

4 years ago
relnote-firefox: ? → 21+
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.
Keywords: dev-doc-needed → dev-doc-complete
(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

4 years ago
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 ;-)

Comment 71

4 years ago
> 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.

Comment 73

4 years ago
As promised:
http://download.beonex.com/component/JXON.js
http://download.beonex.com/component/JXON.txt
(Reporter)

Updated

4 years ago
Summary: Remove E4X from Spidermonkey → Remove E4X from SpiderMonkey
(Assignee)

Comment 74

4 years ago
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."
Depends on: 1083470
You need to log in before you can comment on or make changes to this bug.