454682 - TM: Crash in [@MatchRegExp] with JIT content enabled

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Marcia Knous [:marcia]]

Seen while running Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b1pre) Gecko/20080910020330 Minefield/3.1b1pre.

STR:
1. I think I was scrolling an ebags.com page.

Crash: http://crash-stats.mozilla.com/report/index/defcca61-7f70-11dd-a038-001cc45a2ce4

Comment 2

[Marcia Knous [:marcia]]

http://www.koingosw.com/products/airradar.php crashes for me on Windows XP as well. Changing OS to all and adding that site to the URL since it crashes 100% for me.

Comment 3

[Brian Crowder]

I have this crash in a debugger now, I'll check it out.

Comment 4

[Brian Crowder]

What seems to be happening here is that js_String_p_match is being called with
a bogus "str"...  I'll try to investigate more tomorrow (ideally providing a
reduced testcase).

Comment 5

[Brian Crowder]

The pointer being passed in to js_String_p_match as the "str" parameter is, in fact, actually a String object, not a JSString.  Not sure how this happened or how to debug further on this.  Will seek help on IRC.

Comment 6

[Brian Crowder]

Shell testcase:

a = new String("foo");
for (i = 0; i < 300; i++) {
    a.match(/bar/);
}

Comment 8

[Brian Crowder]

Attachment: a stab at a patch

There might be some better ways to approach this, I'm asking for review here, prior to testing, just to make sure I'm on the right track before spending much more time with this.

Comment 9

[Brian Crowder]

Possible alternatives would be adding a flag to the JSTraceableNative structure, or making the prefix parsing a little smarter (ie., "Ts" for string-this or "To" for objects, and so on).  I think this implementation might be a hair faster than those, though?  Not sure.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Another option, maybe

Comment 11

[Brian Crowder]

Attachment: v2 of "stab"

This adds the _obj version of the builtin, stolen from bz's patch.  Obsoleting bz's patch per discussion on IRC.

Comment 12

[Brian Crowder]

I love it when bugzilla interdiff works.

Comment 13

[Brian Crowder]

For this testcase:

a = "foo";
for (i = 0; i < 300; i++) {
    a.match(/bar/);
}

a = new String("foo");
for (i = 0; i < 300; i++) {
    a.match(/bar/);
}

I get the following result in debug output from the shell:
recorder: started(2), aborted(0), completed(2), different header(0), trees trashed(0), slot promoted(0), unstable loop variable(0), breaks(0), returns(0)

Comment 14

[Boris Zbarsky [:bzbarsky]]

Do we need to worry about the right jsval not being tval (in the call() case)?

Comment 15

[Brian Crowder]

I believe the current restrictions on the js_fun_apply case actually save us from having to worry about this.  Either that, or my testcase is bogus.

Comment 16

[Brian Crowder]

Attachment: v3

Looks like brendan is doing work that'll make fun_apply handle more cases, so it probably needs to be handled more like this patch.

Comment 17

[Brian Crowder]

Comment on attachment 338373 [details] [diff] [review]
v3

review? => mrbkap

Comment 18

[Brian Crowder]

Comment on attachment 338373 [details] [diff] [review]
v3

Just to get another set of eyes on it before I land it.

Comment 19

[Andreas Gal :gal]

Comment on attachment 338373 [details] [diff] [review]
v3

Do we have a test case for this already?

Comment 20

[Brian Crowder]

The testcases in this bug are probably the only ones that actually use String objects with regexps in a loop.  They'll need to get into the suite.

Comment 21

[Andreas Gal :gal]

Whats the status on this bug?

Comment 22

[Brian Crowder]

All it needs is to land.

Comment 23

[Andreas Gal :gal]

Attachment: Refreshed patch with test case.

Comment 24

[Andreas Gal :gal]

Crashes without the patch on the new test case in trace-test.js, passes with. Crowder had a better idea for a test case. He might add another test tomorrow.

Comment 25

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/cc12c800c346

Comment 28

[Marcia Knous [:marcia]]

I just hit this crash again today using  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b1pre) Gecko/20080924020442 Minefield/3.1b1pre. My STR in this instance were:

1. Visit http://www.37signals.com/svn/posts/592-been-lightboxed-lately
2. Click on the That’s right, I got lightboxed link. I crashed right away.

I tried to reproduce but the second time the site loaded fine.

Breakpad: http://crash-stats.mozilla.com/report/index/5d5aca40-8a86-11dd-b3e7-001321b13766

Comment 29

[Andreas Gal :gal]

Follow performance fix:

http://hg.mozilla.org/tracemonkey/rev/80d1db7be330

Comment 31

[RFLazaro]

(In reply to comment #28)
> I just hit this crash again today using  Mozilla/5.0 (Macintosh; U; Intel Mac
> OS X 10.5; en-US; rv:1.9.1b1pre) Gecko/20080924020442 Minefield/3.1b1pre. My
> STR in this instance were:
> 
> 1. Visit http://www.37signals.com/svn/posts/592-been-lightboxed-lately
> 2. Click on the That’s right, I got lightboxed link. I crashed right away.
> 
> I tried to reproduce but the second time the site loaded fine.
> 
> Breakpad:
> http://crash-stats.mozilla.com/report/index/5d5aca40-8a86-11dd-b3e7-001321b13766

I don't see the crash using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080925033548 Minefield/3.1b1pre

Comment 32

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/Regress/regress-454682.js,v  <--  regress-454682.js
initial revision: 1.1

http://hg.mozilla.org/mozilla-central/rev/b04c04268a94