E4X and imacros don't mix

VERIFIED FIXED

Status

()

Core
JavaScript Engine
P2
normal
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: Neil Deakin, Assigned: Waldo)

Tracking

({regression, verified1.9.1})

Trunk
x86
Mac OS X
regression, verified1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

9 years ago
Created attachment 366317 [details]
testcase

The attached testcase iterates over the attributes of an e4x node. Upon reaching the third attribute, it fails with a 'str is not a function' error. Sometimes it fails on the second attribute. It doesn't matter what attribute names or values are used.

Seems to work when running through xpcshell though, but not when loaded in an xhtml or html file. It also works in Firefox 3.0 so seems to be a regression.
(Reporter)

Updated

9 years ago
Blocks: 378893

Comment 1

9 years ago
This regression is still there in the trunk. I think this bug should be fixed for 1.9.1 since it could break some XUL/HTML applications.
Flags: blocking1.9.1?

Updated

9 years ago
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
Another failure case, possibly more illuminating:

h-118:~ jwalden$ ~/moz/js-tm/obj-i386-apple-darwin8.11.1/dist/bin/js -j
js> var x = <a/>;
js> for (var i = 0; i < 3; i++) x == ""
typein:2: TypeError:  is not a function

Intrepid knights of the imacro calculus and of E4X will immediately recognize the problem.  I don't see a centralized place where this can be fixed; looks like it'll be scatter-shot everywhere.

In the meantime I recommend E4X users stop using it, at least if they wish to experience the speedups tracing brings, because I expect to fix this by not tracing such cases.
Summary: Iterating of attributes of e4x node fails → E4X and imacros don't mix
Created attachment 374166 [details] [diff] [review]
Eat viola

We don't want to add the E4X branches to all these object points, I can't imagine we want to burn a trace-time type on E4X, so this is what we're left doing.
Attachment #374166 - Flags: review?(graydon)
A DOM built from XML is not traced either, and if you don't need the full DOM, E4X is cheaper. You're not gonna get people off of E4X by taunting them about lack of trace-JIT coverage, is my point!

/be
Comment on attachment 374166 [details] [diff] [review]
Eat viola

>@@ -7706,16 +7757,18 @@ TraceRecorder::record_JSOP_GETELEM()
>         LIns* unitstr_ins = lir->insCall(&js_String_getelem_ci, args);
>         guard(false, lir->ins_eq0(unitstr_ins), MISMATCH_EXIT);
>         set(&lval, unitstr_ins);
>         return true;
>     }
> 
>     if (JSVAL_IS_PRIMITIVE(lval))
>         ABORT_TRACE("JSOP_GETLEM on a primitive");
>+    else if (OBJECT_IS_XML(cx, JSVAL_TO_OBJECT(lval)))

No else after (macro-ized via ABORT_TRACE, but this macro must return and its call sites must "know" this -- so must readers) return.

>     /* no guards for type checks, trace specialized this already */
>     if (JSVAL_IS_PRIMITIVE(lval))
>         ABORT_TRACE("left JSOP_SETELEM operand is not an object");
>+    else if (OBJECT_IS_XML(cx, JSVAL_TO_OBJECT(lval)))

Ditto.

>     if (!VALUE_IS_FUNCTION(cx, vp[0]))
>         return record_JSOP_CALL();
>+    else if (OBJECT_IS_XML(cx, JSVAL_TO_OBJECT(vp[0])))

Ditto.

>     if (JSVAL_IS_PRIMITIVE(v))
>         ABORT_TRACE("for-in on a primitive value");
>+    else if (OBJECT_IS_XML(cx, JSVAL_TO_OBJECT(v)))

Ditto.

>@@ -8842,16 +8901,18 @@ TraceRecorder::record_JSOP_ITER()
> }
> 
> JS_REQUIRES_STACK bool
> TraceRecorder::record_JSOP_NEXTITER()
> {
>     jsval& iterobj_val = stackval(-2);
>     if (JSVAL_IS_PRIMITIVE(iterobj_val))
>         ABORT_TRACE("for-in on a primitive value");
>+    else if (OBJECT_IS_XML(cx, JSVAL_TO_OBJECT(iterobj_val)))
>+        ABORT_TRACE("xml detected in nextiter, prepare to deoptimize");

Ditto, but you should do this in JSOP_OBJTOP! That gets rid of E4X objects in a bunch of places.

/be
Created attachment 374182 [details] [diff] [review]
With those

(In reply to comment #5)
> No else after (macro-ized via ABORT_TRACE, but this macro must return and its
> call sites must "know" this -- so must readers) return.

Sigh, macro-hiding returns...too easy to (at some level, only sometimes, as I demonstrated in this patch) unconsciously forget that.

> Ditto, but you should do this in JSOP_OBJTOP! That gets rid of E4X objects in a
> bunch of places.

Ah, true.
Attachment #374166 - Attachment is obsolete: true
Attachment #374182 - Flags: review?(graydon)
Attachment #374166 - Flags: review?(graydon)
Created attachment 374184 [details] [diff] [review]
The right patch
Attachment #374182 - Attachment is obsolete: true
Attachment #374184 - Flags: review?(graydon)
Attachment #374182 - Flags: review?(graydon)
Comment on attachment 374184 [details] [diff] [review]
The right patch

I just realized this will break when !JS_HAS_XML_SUPPORT. Simple to fix with an early

#ifndef OBJECT_IS_XML
# define OBJECT_IS_XML(cx,obj) false
#endif

near the top of jstracer.cpp, say right after the "#include "jsxml.h" line you're adding.

/be
Also, in the interests of minimizing new lines, can we do:

#define ABORT_IF_XML(cx, v) \
    JS_BEGIN_MACRO          \
    if (!JSVAL_IS_PRIMITIVE(v) && OBJECT_IS_XML((cx), JSVAL_TO_OBJECT(v))) \
        ABORT_TRACE("xml detected"); \
    JS_END_MACRO

and just use ABORT_IF_XML everywhere?
Created attachment 376348 [details] [diff] [review]
Combine the last two comments for the win
Attachment #374184 - Attachment is obsolete: true
Attachment #376348 - Flags: review?(graydon)
Attachment #374184 - Flags: review?(graydon)

Updated

9 years ago
Attachment #376348 - Flags: review?(graydon) → review+
http://hg.mozilla.org/tracemonkey/rev/ff836542060b
Whiteboard: fixed-in-tracemonkey

Comment 12

9 years ago
http://hg.mozilla.org/mozilla-central/rev/ff836542060b
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 13

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5191386baa44
Keywords: fixed1.9.1
verified FIXED using the attached testcase (over 3 iterations) on builds:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090604 Shiretoko/3.5pre ID:20090604031153

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090604 Minefield/3.6a1pre ID:20090604105829
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
You need to log in before you can comment on or make changes to this bug.