Closed Bug 454682 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: marcia, Assigned: gal)

References

()

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 4 obsolete files)

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
Duplicate of this bug: 454675
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.
OS: Mac OS X → All
Hardware: PC → All
Summary: Crash in [@MatchRegExp] while scrolling a page with JIT enabled → Crash in [@MatchRegExp] with JIT content enabled
Summary: Crash in [@MatchRegExp] with JIT content enabled → TM: Crash in [@MatchRegExp] with JIT content enabled
I have this crash in a debugger now, I'll check it out.
Assignee: general → 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).
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.
Shell testcase:

a = new String("foo");
for (i = 0; i < 300; i++) {
    a.match(/bar/);
}
Duplicate of this bug: 455019
Attached patch a stab at a patch (obsolete) — Splinter Review
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.
Attachment #338345 - Flags: review?(shaver)
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.
Attached patch Another option, maybe (obsolete) — Splinter Review
Attached patch v2 of "stab" (obsolete) — Splinter Review
This adds the _obj version of the builtin, stolen from bz's patch.  Obsoleting bz's patch per discussion on IRC.
Attachment #338345 - Attachment is obsolete: true
Attachment #338360 - Attachment is obsolete: true
Attachment #338362 - Flags: review?(shaver)
Attachment #338345 - Flags: review?(shaver)
I love it when bugzilla interdiff works.
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)
Do we need to worry about the right jsval not being tval (in the call() case)?
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.
Attached patch v3 (obsolete) — Splinter Review
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.
Attachment #338362 - Attachment is obsolete: true
Attachment #338373 - Flags: review?(shaver)
Attachment #338362 - Flags: review?(shaver)
Comment on attachment 338373 [details] [diff] [review]
v3

review? => mrbkap
Attachment #338373 - Flags: review?(shaver) → review?(mrbkap)
Attachment #338373 - Flags: review?(mrbkap) → review+
Attachment #338373 - Flags: review?(gal)
Comment on attachment 338373 [details] [diff] [review]
v3

Just to get another set of eyes on it before I land it.
Comment on attachment 338373 [details] [diff] [review]
v3

Do we have a test case for this already?
Attachment #338373 - Flags: review?(gal) → review+
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.
Flags: in-testsuite?
Whats the status on this bug?
All it needs is to land.
Keywords: checkin-needed
Attachment #338373 - Attachment is obsolete: true
Attachment #340032 - Flags: review?
Assignee: crowder → 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.
http://hg.mozilla.org/tracemonkey/rev/cc12c800c346
Status: NEW → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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
Follow performance fix:

http://hg.mozilla.org/tracemonkey/rev/80d1db7be330
Duplicate of this bug: 456774
(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
/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
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus-
Duplicate of this bug: 464238
Crash Signature: [@MatchRegExp]
You need to log in before you can comment on or make changes to this bug.