366869 - js_ThreadDestructorCB calls JS_REMOVE_AND_INIT_LINK incorrectly, resulting in an infinite loop

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Wan-Teh Chang]

This bug was introduced in the patch for bug 312238.

We found this bug while investigating a hang (infinite loop)
on SeaMonkey/Firefox shutdown reported in 362768 comment 14.

In js/src/jscntxt.c, we have:
 74 void JS_DLL_CALLBACK
 75 js_ThreadDestructorCB(void *ptr)
 76 {
 77     JSThread *thread = (JSThread *)ptr;
 78 
 79     if (!thread)
 80         return;
 81     while (!JS_CLIST_IS_EMPTY(&thread->contextList))
 82         JS_REMOVE_AND_INIT_LINK(thread->contextList.next);
 83     GSN_CACHE_CLEAR(&thread->gsnCache);
 84     free(thread);
 85 }

We are spinning in the while loop on lines 81-82.  This is
because the argument for the JS_REMOVE_AND_INIT_LINK macro
interacts undesirably with the macro.  The macro is defined
as follows:

112 #define JS_REMOVE_AND_INIT_LINK(_e)    \
113     JS_BEGIN_MACRO                     \
114         (_e)->prev->next = (_e)->next; \
115         (_e)->next->prev = (_e)->prev; \
116         (_e)->next = (_e);             \
117         (_e)->prev = (_e);             \
118     JS_END_MACRO

Focus on the first assignment statement in the macro's expansion.
The left-hand side of the assignment is
(thread->contextList.next)->prev->next, which is
thread->contextList.next, so the value of thread->contextList.next
gets changed in the first assignment statement.  But the macro
needs the value of its _e argument to stay constant.

I reviewed all the users of JS_REMOVE_AND_INIT_LINK and believe
this is the only one that uses JS_REMOVE_AND_INIT_LINK incorrectly.
Someone else should do an independent code review.  The users of
some other related macros may also need to be reviewed.

This bug seems to imply that js_ThreadDestructorCB was never used
before.  This issue should also be investigated.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: Patch v1

This patch avoids the infinite loop.

Comment 2

[Kai Engert (:KaiE:)]

Comment on attachment 251325 [details] [diff] [review]
Patch v1

Brendan, can you please review?

Comment 3

[Brendan Eich [:brendan]]

Attachment: oops, I read bugmail and patched over in bug 362768

Same patch as Kai's, but unobfuscated name and house-style line spacing. Thanks,

/be

Comment 4

[Brendan Eich [:brendan]]

Fixed on trunk:

js/src/jscntxt.c rev 3.98

/be

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 251329 [details] [diff] [review]
oops, I read bugmail and patched over in bug 362768

Safe rider for 1.8.1.2, to fix a real bug that could bite the 1.8 branch if the fix for bug 362768 lands on that branch.  I'd rather we take it and not have to worry.

/be

Comment 6

[Wan-Teh Chang]

Comment on attachment 251329 [details] [diff] [review]
oops, I read bugmail and patched over in bug 362768

The macro seems to evaluate its actual more than twice.
I'm not sure how many times.  Maybe 8 times?

Since js_ThreadDestructorCB may have never been executed
before, do you think it's risky to check in the NSPR patch
in bug 362768 that'll cause js_ThreadDestructorCB to be
executed on Mozilla client shutdown?

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> Since js_ThreadDestructorCB may have never been executed
> before, do you think it's risky to check in the NSPR patch
> in bug 362768 that'll cause js_ThreadDestructorCB to be
> executed on Mozilla client shutdown?

Just bake it a bit on the trunk and it should be ok.

/be

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 251329 [details] [diff] [review]
oops, I read bugmail and patched over in bug 362768

approved for 1.8 branch, a=dveditz for drivers

Comment 9

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch:

js/src/jscntxt.c rev 3.60.4.11

/be