702426 - Crash [@ JSC::ExecutablePool::~ExecutablePool] or [@ delete_<JSC::ExecutablePool>]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stacks

function g(code) {
    eval("(function(){" + code + "})")()
}
g("evalcx(\"(/a/g,function(){})\")");
g("'a'.replace(/a/g,gc)")

crashes js debug shell on m-c changeset eb84780783ed without any CLI arguments at JSC::ExecutablePool::~ExecutablePool and js opt shell at delete_<JSC::ExecutablePool>

s-s just to be safe even though this seems to be a null crash, scary function delete_ is on the opt shell stack.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   79966:366d80e91816
user:        Chris Leary
date:        Mon Nov 07 13:42:31 2011 -0800
summary:     Bug 634654 - Add RegExpPrivate cache. (r=Waldo)

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Happens on Mac too.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Mac Valgrind opt stacks - shows Invalid free() / delete / delete[] / realloc()

Mac Valgrind opt stacks

Comment 3

[Chris Leary [:cdleary] (not checking bugmail)]

Very nice find. I think I have to back my patches out of Aurora because of this gaffe. Thread data level cache outlives the jaegerCompartment. I totally forgot about that constraint because I didn't have to touch the ExecutableAllocator acquisition path.

That change would be too much to land on Aurora. Backout diffs forthcoming, which will negate the aurora approval flag on those other RegExp bugs. I'll also create a fix for Nightly.

Comment 4

[Chris Leary [:cdleary] (not checking bugmail)]

Oh, and I have to back out the lazification change as well because backing out the regexp cache regresses our V8-regexp score.

Comment 5

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Hoist executable allocator to thread data level.

It's a simple fix, but we'll want to observe this on nightlies instead of blindly applying to Aurora.

Comment 6

[Chris Leary [:cdleary] (not checking bugmail)]

Note to self: add the wonderful test before committing!

Comment 7

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Back out laziness and related patches; folded.

Reversed in this patch (newest to oldest):

366d80e91816: RegExpPrivate cache
2da44633e0bb: Eagerly compile clone source
d50bd6d5b097: Style followup
61dd23c012ee: Lazy regexps

Interdiffs to follow for ease of review.

Comment 8

[Chris Leary [:cdleary] (not checking bugmail)]

(Oh, and I confirmed that the v8-regexp score was restored to its original value.)

Comment 9

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Eagerly clone source interdiff.

Comment 10

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Style followup interdiff.

Comment 11

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Lazy regexp backout conflict.

I couldn't interdiff this one successfully, so I've attached the conflict hunk for comparison with the folded patch. Sorry these are kind of boring, but I figured I'd err on the side of documenting / getting easy reviews on things.

Comment 12

[Luke Wagner [:luke]]

Comment on attachment 574497 [details] [diff] [review]
Hoist executable allocator to thread data level.

Review of attachment 574497 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/RegExpObject-inl.h
@@ +380,5 @@
>  #ifdef JS_METHODJIT
>      if (isJITRuntimeEnabled(cx) && !yarrPattern.m_containsBackreferences) {
> +        JSC::ExecutableAllocator *execAlloc = cx->threadData()->getOrCreateExecutableAllocator();
> +        if (!execAlloc) {
> +            js_ReportOutOfMemory(cx);

Instead of storing the rt in the threadData and having to report oom at the callsite, can you just have a JSContext* parameter to these functions?

Comment 13

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to Luke Wagner [:luke] from comment #12)
> Instead of storing the rt in the threadData and having to report oom at the
> callsite, can you just have a JSContext* parameter to these functions?

Good idea! I still need to store rt in the ThreadData for deallocation in the destructor, though.

Comment 14

[Chris Leary [:cdleary] (not checking bugmail)]

Adding Kairo so he can see the relevant backout patch. (Kairo, you were asking about it in bug 701761 comment 10.)

Comment 15

[Chris Leary [:cdleary] (not checking bugmail)]

Fix for trunk: https://hg.mozilla.org/integration/mozilla-inbound/rev/ebc6ef282df4

Note: will land the testcase after we back out on Aurora.

Comment 16

[Chris Leary [:cdleary] (not checking bugmail)]

Shell built okay but browser burned:

https://hg.mozilla.org/integration/mozilla-inbound/rev/53d36b17194e (backout)

Comment 17

[LegNeato]

Comment on attachment 574508 [details] [diff] [review]
Back out laziness and related patches; folded.

[triage comment]
Approved for aurora. If this needs to be landed for Fx9, please request beta as well.

Comment 18

[Chris Leary [:cdleary] (not checking bugmail)]

Take two, landing on m-i for FF11 (nightly). Shell build didn't do threadsafe.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6157fc56d02c

Comment 19

[Chris Leary [:cdleary] (not checking bugmail)]

Backout pushed to try: https://hg.mozilla.org/try/rev/d70a2773251a

Comment 20

[Carsten Book [:Tomcat]]

(In reply to Chris Leary [:cdleary] from comment #18)
> Take two, landing on m-i for FF11 (nightly). Shell build didn't do
> threadsafe.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6157fc56d02c

its also now on mozilla-central -> https://hg.mozilla.org/mozilla-central/rev/6157fc56d02c

Comment 21

[Daniel Veditz [:dveditz]]

Assuming exploitable based on comment 2

Comment 22

[Chris Leary [:cdleary] (not checking bugmail)]

Backout pushed to aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/3f725329f26d

Comment 23

[Chris Leary [:cdleary] (not checking bugmail)]

Now that we've landed on aurora I've landed the test on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/cee431a0f087

Comment 24

[Chris Leary [:cdleary] (not checking bugmail)]

https://hg.mozilla.org/mozilla-central/rev/cee431a0f087

Comment 25

[Chris Leary [:cdleary] (not checking bugmail)]

I think the right status for backout is |fixed|...

Comment 26

[u279076]

Verified fixed for using Firefox 10.0 and 11.0 debug js-shells.

Comment 27

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #26)
> Verified fixed for using Firefox 10.0 and 11.0 debug js-shells.

Also setting to VERIFIED.

Comment 28

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug702426-regexp-gc.js.