JS_ExecuteRegExpNoStatics() is heavily broken

ASSIGNED
Assigned to

Status

()

Core
JavaScript Engine
P3
normal
ASSIGNED
a year ago
9 months ago

People

(Reporter: Ehsan, Assigned: kalpa)

Tracking

({triage-deferred})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
While working on bug 1347425, I tried to use JS_ExecuteRegExpNoStatics() in a similar manner to how it's used in nsContentUtils::IsPatternMatching() <http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/dom/base/nsContentUtils.cpp#6814>, and it seemed to work, and the test we had in the tree with the regular expression to match passed: <http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/netwerk/test/mochitests/test_user_agent_overrides.html#26>.

And then I tried adding a similar test with a different regex!  This time, "0".  That didn't work.  I narrowed it down to ExecuteRegExpImpl <http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/js/src/builtin/RegExp.cpp#153> returning RegExpRunStatus_Success_NotFound.  The string in which I was searching was "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:55.0) Gecko/20100101 Firefox/55.0".

I then tried "zilla".  That one worked.  "illa" didn't.  "[0-9]+" din't either...  This code is being used for several use cases but in every case for full string matches (i.e., "^(...)$"), so I hope whatever the underlying cause ends up being doesn't affect that, but at this is at least worth investigation given the fact that we're shipping code relying on this function.  :(
(Reporter)

Comment 1

a year ago
Created attachment 8851254 [details]
The patch from bug 1347425 that adds the code that allows an easy way to test this

After applying this patch, you can build with it, then edit this test and change the regex on line 26 of netwerk/test/mochitests/test_user_agent_overrides.html to "illa".
(Assignee)

Comment 2

a year ago
Hello,

Will it be okay if I work on this bug?
Flags: needinfo?(ehsan)
(Reporter)

Comment 3

a year ago
Yes, by all means! :-)
Flags: needinfo?(ehsan)
(Assignee)

Comment 4

a year ago
(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #3)
> Yes, by all means! :-)

Thanks

Can you assign me this bug then?
Flags: needinfo?(ehsan)
(Reporter)

Comment 5

a year ago
Here you go!  I'll also grant you editbugs access so that you can you can do this yourself the next time you were interested to fix a bug.  :-)

(BTW sorry for the long delay to get back to you.)
Assignee: nobody → avikalpakundu
(Reporter)

Updated

a year ago
Flags: needinfo?(ehsan)
(Assignee)

Updated

a year ago
Status: NEW → ASSIGNED
Keywords: triage-deferred
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.