TM: safeguard against RegExp.prototype.test changes in exec-to-test conversions

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

(Assignee)

Description

7 years ago
From bug 594108 comment 12, w.r.t. the replacement of calls to
RegExp.prototype.exec() with calls to RegExp.prototype.test() in some circumstances:

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

Comment 1

7 years ago
Would this alternative work:  before making the change, check if RegExp.prototype.test() is still equal to js_regexp_test()?  That way you'd lose the optimization if the user overwrites RegExp.prototype.test() but that's gonna be very rare.  I like the idea because we're already looking up 'test' in RegExp.prototype, so why not look up 'exec' at the same time?
That would work -- you'd have to check that the value is a function object, then that the function is native, before you could safely test u.n.native.

I was thinking of a scheme that avoids all property accesses, instead linking exec to test via one of the two native function object reserved slots (these are up for grabs to the code that creates the native function object). But you would still need to verify that exec was exec to pull this off safely. On the upside it would get rid of testAtom as well as the getProperty call, and have the smallest recording time overhead.

/be
(Assignee)

Comment 3

7 years ago
Created attachment 473296 [details] [diff] [review]
patch (against TM 53102:82e35dc17b71)

(In reply to comment #2)
> That would work -- you'd have to check that the value is a function object,
> then that the function is native, before you could safely test u.n.native.

I went with this approach.  I particularly liked that it kept all the code relating to this exec-to-test conversion in a single place.  I measured v8-regexp, which contains 466 static calls to exec() that are convertible;  the cost of the property accesses is negligible.
Attachment #473296 - Flags: review?(brendan)
(Assignee)

Updated

7 years ago
Summary: TM: stash a copy of RegExp.prototype.test to safeguard exec-to-test conversions → TM: safeguard against RegExp.prototype.test changes in exec-to-test conversions
Comment on attachment 473296 [details] [diff] [review]
patch (against TM 53102:82e35dc17b71)

Great!

/be
Attachment #473296 - Flags: review?(brendan) → review+
(Assignee)

Comment 5

7 years ago
http://hg.mozilla.org/tracemonkey/rev/858168a1a3a7
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 6

7 years ago
http://hg.mozilla.org/mozilla-central/rev/858168a1a3a7
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.