Closed Bug 594108 Opened 10 years ago Closed 10 years ago

fix RegExp.exec()-to-RegExp.test() conversion

Categories

(Core :: JavaScript Engine, defect, blocker)

x86
macOS
defect
Not set
blocker

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: sayrer, Unassigned)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(3 files)

I'm seeing this on several sites. Many of them use Google APIs, so they may be related that way.

http://www.9to5mac.com/

and

http://www.mashable.com/

are two examples.

This bug blocks merging to mozilla-central.
Severity: normal → blocker
blocking2.0: --- → beta6+
Duplicate of this bug: 594126
Good STR from bug 594126 comment 0:

>Steps to Reproduce:
>1. Start TM build with New profile
>2. Open URL ( http://en.wikipedia.org/wiki/Mozilla )
>
>Actual Results:
> Script timeout.

Disabling the optimization in bug 468214 fixes the problem.

The attached patch fixes the problem in a more targeted way. When I showed him the problem, dvander guessed that it might have something to do with the types returned by regexp_test not matching the values in the interpreter (which are still returned from regexp_exec). The attached patch, by fixing the problem, provides pretty good support for this guess.

The patch retains the optimization, in the sense that it doesn't construct a real results object, but it returns an empty JSObject that is not all like the real results. This falsifies the hypothesis that not constructing the results object is the cause of the bug. But the hypothesis that returning different types for the call in the trace vs. the interpreter is still possible.
Huh, my original patch constructed an empty array, but it seemed unnecessarily complicated :/  Sorry for the breakage.


(In reply to comment #3)
>
> But the hypothesis that returning different
> types for the call in the trace vs. the interpreter is still possible.

So it would be better to return an empty array instead of an empty object?

And one thing about the patch I don't understand:  how is js_regexp_test2 called?
(In reply to comment #3)
> Created attachment 472795 [details] [diff] [review]
> Fix the problem by making types match
> 
> Good STR from bug 594126 comment 0:
> 
> >Steps to Reproduce:
> >1. Start TM build with New profile
> >2. Open URL ( http://en.wikipedia.org/wiki/Mozilla )
> >
> >Actual Results:
> > Script timeout.
> 
> Disabling the optimization in bug 468214 fixes the problem.

That's a mochitest support for Fennec bug -- do you mean bug 581595 ? If so, please make this bug block that one.

/be
Shell reproducer:

for (var i = 0; i < 10; i++) {
    var re = /a(b)c/;
    var match = re.exec("123abc456abc789");
    var b = (re.exec(""), v = re.exec(match)) !== null;
    print(v);
}

Expected:
abc,b
abc,b
abc,b
abc,b
abc,b
abc,b
abc,b
abc,b
abc,b
abc,b

Actual, with -j:
abc,b
abc,b
abc,b
abc,b
true
false
false
false
false
false
Blocks: 581595
I remember raising the trace-type return type difference on IRC -- probably was not clear about it, though -- sorry.

/be
This one's simpler:

for (var i = 0; i < 10; i++) {
    var re = /a(b)c/;
    var b = (re.exec(""), v = re.exec("abc")) !== null;
    print(v);
}
Duplicate of this bug: 593632
This patch replaces the call to RegExp.exec() on the stack with a call to RegExp.test().  It also adds "test" to the pre-defined table of atoms.

I'm uncertain about the whole thing, and particularly uncertain about the NULL argument to js_GetClassPrototype().  

It fixes my small reproducer, I haven't tried it on the full websites yet.
Attachment #472837 - Flags: review?(brendan)
The patch appears to fix the script time-outs on http://www.9to5mac.com/
and http://en.wikipedia.org/wiki/Mozilla.  Yay!
Comment on attachment 472837 [details] [diff] [review]
alternative approach (against TM 52855:6659fdc25196)

> JS_REQUIRES_STACK RecordingStatus
> TraceRecorder::callNative(uintN argc, JSOp mode)
> {
>     LIns* args[5];
> 
>     JS_ASSERT(mode == JSOP_CALL || mode == JSOP_NEW || mode == JSOP_APPLY);
> 
>     Value* vp = &stackval(0 - (2 + argc));
>+
>+  retry:
>     JSObject* funobj = &vp[0].toObject();
>     JSFunction* fun = GET_FUNCTION_PRIVATE(cx, funobj);
>     Native native = fun->u.n.native;

Looks like you could move these three variables' declarations down to after the switch and avoid the goto.

>+                        JSObject* proto;
>+                        Value test;
>+                        jsid testId = ATOM_TO_JSID(cx->runtime->atomState.testAtom);
>+                        if (js_GetClassPrototype(cx, NULL, JSProto_RegExp, &proto) &&

It shouldn't matter which global object's RegExp.prototype.test you find, but just in case it does, pass vp[0].getParent() not NULL.

>+                            js_GetProperty(cx, proto, testId, &test)) {
>+                            vp[0] = test;

Argh, what if someone messes with RegExp.prototype.test behind your back (overwrites it with some other function)?

Instead we could save the original value of RegExp.prototype.test in a global-reserved slot. See JSCLASS_GLOBAL_FLAGS. Or we could link from RegExp.prototype.exec to its peer RegExp.prototype.test via a slot reserved by the Function class.

Probably this should be a followup bug, though. r=me with that filed and the other points addressed.

/be
Attachment #472837 - Flags: review?(brendan) → review+
Summary: hang involving match() and array_trace() → fix RegExp.exec()-to-RegExp.test() conversion
http://hg.mozilla.org/mozilla-central/rev/2bfe3a246736
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.