Closed Bug 464138 Opened 11 years ago Closed 11 years ago

Intermittent JS regexp error on tinderbox.


(Core :: JavaScript Engine, defect)

Not set





(Reporter: jst, Assigned: dmandelin)




(Keywords: fixed1.9.1, regression)


(2 files, 2 obsolete files)

The above URL shows the failed test. The failure is this:

Failure: assertMatch : regex: "/knowmad.jpg$/ did not match: ''" Line #133

AFAIK we've only seen this once on Tinderbox so far, but I could be wrong.
"This" does appear related to running regexps that were compiled to native code, but exactly which test fails, and whether they fail at all, seems to vary with the mochitest configuration (i.e., test subset) as well. I seem to be able to duplicate a failure well in the prototypes test directory, but the failure appears at a different point. I will need to look further.
This is a beta2 blocker because it's causing intermittent tinderbox orange.
Flags: blocking1.9.1+
Keywords: regression
Target Milestone: --- → mozilla1.9.1b2
Attached patch Patch for moz-central (obsolete) — Splinter Review
The cause was that my code was assuming that the address of a JSRegExp could be used as a unique key for the fragment cache. That assumption is invalid partly because of GC but more to the point because of "temporary" JSRegExp objects created and quickly destroyed (thus increasing the probability the memory will be soon reused for another JSRegExp) by the implementation of string.replace.

The fix is to blacklist a fragment when the regexp that created it is destroyed, guaranteeing that it will never be used again. I will create a higher performance fix later, but for now this is simple and should solve the problem.
Attachment #347662 - Flags: review?(gal)

if ()
Attachment #347662 - Flags: review?(gal) → review+
Could we push this into TM as well and I would suggest we follow up with the hashing idea. Seems a big win for repetitive matches and we probably blacklist a lot of stuff right now we shouldn't.
Attached patch Nit fixed (obsolete) — Splinter Review
Attachment #347665 - Flags: approval1.9.1b2?
Attachment #347665 - Flags: approval1.9.1b2? → approval1.9.1b2+
Comment on attachment 347665 [details] [diff] [review]
Other little nit fixed
[Checkin: Comment 10 & 21]

>@@ -2830,9 +2832,10 @@ js_DestroyRegExp(JSContext *cx, JSRegExp
> js_DestroyRegExp(JSContext *cx, JSRegExp *re)
> {
>     if (JS_ATOMIC_DECREMENT(&re->nrefs) == 0) {
>-#ifdef JS_TRACER
>-        JS_TRACE_MONITOR(cx).reFragmento->clearFrag(re);
>+        /* Don't reuse this compiled code for some new regexp at same addr */

Drive-by nit: full stop at end of sentences in comments (any comment taking one or more lines has sentences; smaller comments use fragments so no capitalization, no period).

Nit noted.

The fix was pushed as changeset - 21572:1c473d3f87ea. We'll be replacing this clearing mechanism eventually so I can de-nit then.
Is it possible that this is causing the Windows and Mac mochitest timeouts?  Mac cycled once green with this patch, then timed out next cycle.  Windows never cycled green.  On both the timeout is in an offline cache test (hence the dcamp cc), but the timeouts are in different tests for Mac and Windows.  The two Windows timeouts happened in the same part of the same test, though.  It looks to me like the tests simply die part way through and never call SimpleTest.finish().
I think it is known that this bug caused an Lk regression. Is this going to be addressed in the next day or so, or do I need to think about altering the thresholds on the Thunderbird tinderboxes?
Depends on: 464413
On tracemonkey branch, this caused bug 464413.
Attachment #347665 - Attachment description: Other little nit fixed → Other little nit fixed [Checkin: Comment 10]
Attachment #347662 - Attachment is obsolete: true
Comment on attachment 347678 [details] [diff] [review]
Fix non-tracing platform bustage
[Checkin: Comment 15 & 21]
Attachment #347678 - Attachment description: Fix non-tracing platform bustage → Fix non-tracing platform bustage [Checkin: Comment 15]
I think the leak might be solved by attachment 347727 [details] [diff] [review] in bug 464413. I think that patch has only been applied to the tracemonkey repo. Should we commit it to m-c as well?
(In reply to comment #17)
> Yes.

OK. That patch applies cleanly to trunk and doesn't seem to be causing any problems in TraceMonkey. I guess I'll need to get approval from someone. Also I don't have checkin access to the main tree so I'll have to find someone to do that too.
That patch doesn't seem to have fixed the leak.
(In reply to comment #19)
> That patch doesn't seem to have fixed the leak.

The leak levels have returned to approximately their original levels with the latest merging of tracemonkey.
Closed: 11 years ago
Resolution: --- → FIXED
Attachment #347665 - Attachment description: Other little nit fixed [Checkin: Comment 10] → Other little nit fixed [Checkin: Comment 10 & 21]
Attachment #347678 - Attachment description: Fix non-tracing platform bustage [Checkin: Comment 15] → Fix non-tracing platform bustage [Checkin: Comment 15 & 21]
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.