Closed
Bug 464138
Opened 16 years ago
Closed 16 years ago
Intermittent JS regexp error on tinderbox.
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9.1b2
People
(Reporter: jst, Assigned: dmandelin)
References
()
Details
(Keywords: fixed1.9.1, regression)
Attachments
(2 files, 2 obsolete files)
1.64 KB,
patch
|
damons
:
approval1.9.1b2+
|
Details | Diff | Splinter Review |
745 bytes,
patch
|
Details | Diff | Splinter Review |
The above URL shows the failed test. The failure is this: Failure: assertMatch : regex: "/knowmad.jpg$/ did not match: 'http://script.aculo.us/images/knowmad.jpg'" Line #133 AFAIK we've only seen this once on Tinderbox so far, but I could be wrong.
Reporter | ||
Comment 1•16 years ago
|
||
This happened again, http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1226370450.1226374042.12401.gz
Assignee | ||
Comment 2•16 years ago
|
||
"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.
Comment 3•16 years ago
|
||
This is a beta2 blocker because it's causing intermittent tinderbox orange.
Assignee | ||
Comment 4•16 years ago
|
||
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)
Comment 5•16 years ago
|
||
Nit: if () foo;
Updated•16 years ago
|
Attachment #347662 -
Flags: review?(gal) → review+
Comment 6•16 years ago
|
||
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.
Assignee | ||
Comment 7•16 years ago
|
||
Assignee | ||
Comment 8•16 years ago
|
||
Attachment #347663 -
Attachment is obsolete: true
Reporter | ||
Updated•16 years ago
|
Attachment #347665 -
Flags: approval1.9.1b2?
Updated•16 years ago
|
Attachment #347665 -
Flags: approval1.9.1b2? → approval1.9.1b2+
Comment 9•16 years ago
|
||
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); >-#endif >+ /* 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). /be
Assignee | ||
Comment 10•16 years ago
|
||
Nit noted. The fix was pushed as changeset - 21572:1c473d3f87ea. We'll be replacing this clearing mechanism eventually so I can de-nit then.
Assignee | ||
Comment 11•16 years ago
|
||
Comment 12•16 years ago
|
||
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().
Comment 13•16 years ago
|
||
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?
Comment 14•16 years ago
|
||
On tracemonkey branch, this caused bug 464413.
Updated•16 years ago
|
Attachment #347665 -
Attachment description: Other little nit fixed → Other little nit fixed
[Checkin: Comment 10]
Updated•16 years ago
|
Attachment #347662 -
Attachment is obsolete: true
Comment 15•16 years ago
|
||
Comment on attachment 347678 [details] [diff] [review] Fix non-tracing platform bustage [Checkin: Comment 15 & 21] http://hg.mozilla.org/mozilla-central/rev/b80820b02087
Attachment #347678 -
Attachment description: Fix non-tracing platform bustage → Fix non-tracing platform bustage
[Checkin: Comment 15]
Assignee | ||
Comment 16•16 years ago
|
||
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?
Comment 17•16 years ago
|
||
Yes.
Assignee | ||
Comment 18•16 years ago
|
||
(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.
Comment 20•16 years ago
|
||
(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.
Comment 21•16 years ago
|
||
Landed on mozilla-central http://hg.mozilla.org/mozilla-central/rev/8a3e4bede583
Comment 22•16 years ago
|
||
So, fixed?
Updated•16 years ago
|
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Attachment #347665 -
Attachment description: Other little nit fixed
[Checkin: Comment 10] → Other little nit fixed
[Checkin: Comment 10 & 21]
Updated•16 years ago
|
Attachment #347678 -
Attachment description: Fix non-tracing platform bustage
[Checkin: Comment 15] → Fix non-tracing platform bustage
[Checkin: Comment 15 & 21]
Updated•16 years ago
|
Keywords: fixed1.9.1
Updated•15 years ago
|
Flags: in-testsuite-
Flags: in-litmus-
You need to log in
before you can comment on or make changes to this bug.
Description
•