Closed Bug 496113 Opened 10 years ago Closed 10 years ago

e4x regression: simple filters do not work

Categories

(Core :: JavaScript Engine, defect)

1.9.1 Branch
defect
Not set

Tracking

()

VERIFIED FIXED

People

(Reporter: wes, Assigned: mrbkap)

References

Details

(4 keywords, Whiteboard: fixed-in-tracemonkey)

Attachments

(3 files)

Attached file Reduced test case
Masakazu Takahashi posted in dev-tech-jseng today about an E4X bug in TraceMoneky; he was trying to run a sample program from the documentation here: https://developer.mozilla.org/en/Core_JavaScript_1.5_Guide/Processing_XML_with_E4X#section_7
(the one with people named Bob and Joe)

I have reduced to a test case which shows that filters like people.person.(name == "Joe") do not work (nothing is found when it should be).

This is a regression; I double-checked against a version of SpiderMonkey I yanked from CVS about 14-16 months ago. I cloned TraceMonkey from hg earlier this week.
I have also just confirmed that this bug does not affect my current browser (Firefox 3.0.10).

Wes
Flags: blocking1.9.1?
If you go into about:config and turn JIT off by flipping javascript.options.jit.content to "false" do you still see the problem?
E4X doesn't get JITted, because LOL E4X IMO.

This could have some extension exposure, but if we've had it a long time probably not. Blocking will probably depend on the regression range that's found.
Attachment #381295 - Attachment mime type: application/x-javascript → text/plain
Using this testcase suggested by Wes,
===
var people = <people>
 <person>
  <name>Joe</name>
  <age>46</age>
 </person>
</people>;

if (people.person.(name == "Joe").toString().length == 0)
  throw("it's broken");
===
"this test script will return 0 when the bug is not found, and 3 when it is"

I also needed to hack autoBisect locally.

Anyway, autoBisect shows this is probably related to bug 460882 :

The first bad revision is:
changeset:   25078:07e345fbdac2
user:        Ben Turner
date:        Mon Feb 16 13:16:13 2009 -0800
summary:     Bug 460882. r+sr=mrbkap.
Blocks: 460882
Version: 1.9.1 Branch → Trunk
So e4x/Expressions/11.2.4.js is failing on tm and new repos, passing on cvs.m.o (1.9.0 and older). When did it start failing? Was a bug filed but not assigned?

/be
Version: Trunk → 1.9.1 Branch
(In reply to comment #6)
> bug 478910

Need new bug -- regressed again (recently?). Thanks,

/be
This is, indeed a regression from bug 460882. In particular, these lines in that patch:

 #define SPROP_GET(cx,sprop,obj,obj2,vp)                                       \
     (((sprop)->attrs & JSPROP_GETTER)                                         \
      ? js_InternalGetOrSet(cx, obj, (sprop)->id,                              \
                            OBJECT_TO_JSVAL((sprop)->getter), JSACC_READ,      \
                            0, 0, vp)                                          \
-     : (sprop)->getter(cx, OBJ_THIS_OBJECT(cx,obj), SPROP_USERID(sprop), vp))
+     : (sprop)->getter(cx, obj, SPROP_USERID(sprop), vp))

mean that we no longer call the thisObject hook for getters, and with is suddenly obj. XML filters basically work by using 'with (xmlobj) filtercode', so this broke things because xml.cpp:GetProperty starts out with JS_GetInstancePrivate and doesn't walk the prototype chain to find the XML instance that we care about.

So, I could fix this by walking the prototype chain in GetProperty (which is easy) or by restoring the lost OBJ_THIS_OBJECT and trying to find another way to remedy the perf regression.
Flags: blocking1.9.1? → blocking1.9.1+
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attached patch Proposed fixSplinter Review
My dromaeo run is still going, but I am confident that this will fix the regression without regressing performance. It is the most conservative fix that I can see.
Attachment #381414 - Flags: review?(brendan)
Comment on attachment 381414 [details] [diff] [review]
Proposed fix

>Bug 496113 - Unwrap 'with' objects before calling into getters to restore API compatibility.
>
>diff --git a/js/src/jsscope.h b/js/src/jsscope.h
>--- a/js/src/jsscope.h
>+++ b/js/src/jsscope.h
>@@ -363,16 +363,24 @@ js_GetSprop(JSContext* cx, JSScopeProper
>     JS_ASSERT(!SPROP_HAS_STUB_GETTER(sprop));
> 
>     if (sprop->attrs & JSPROP_GETTER) {
>         jsval fval = js_CastAsObjectJSVal(sprop->getter);
>         return js_InternalGetOrSet(cx, obj, sprop->id, fval, JSACC_READ,
>                                    0, 0, vp);
>     }
> 
>+    /*
>+     * JSObjectOps is private, so we know there are two implementations of the

Nit: "exactly two" -- same in the copy for js_SetSprop. Maybe just comment once and back-reference in the second place?

>+     * thisObject hook: with objects and XPConnect wrapped native objects.
>+     * XPConnect objects don't expect the hook to be called here, but with
>+     * objects do.
>+     */
>+    if (STOBJ_GET_CLASS(obj) == &js_WithClass)
>+        obj = OBJ_THIS_OBJECT(cx, obj);

Could optimize the call as discussed. Saves code size (we don't care about With perf).

r=me with these.

/be
Attachment #381414 - Flags: review?(brendan) → review+
e4x/Expressions/11.2.2.js e4x/Expressions/11.2.4.js e4x/XML/13.4.4.29.js e4x/XML/13.4.4.3.js e4x/XML/13.4.4.33.js e4x/XML/13.4.4.4.js e4x/XML/13.4.4.7.js e4x/Expressions/regress-496113.js all pass on tracemonkey with this patch.
http://hg.mozilla.org/mozilla-central/rev/bdb423e6cab8
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
http://hg.mozilla.org/tracemonkey/rev/53d4c2c8e6f5

/cvsroot/mozilla/js/tests/e4x/Expressions/regress-496113.js,v  <--  regress-496113.js
initial revision: 1.1
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.