Last Comment Bug 366869 - js_ThreadDestructorCB calls JS_REMOVE_AND_INIT_LINK incorrectly, resulting in an infinite loop
: js_ThreadDestructorCB calls JS_REMOVE_AND_INIT_LINK incorrectly, resulting in...
Status: RESOLVED FIXED
: fixed1.8.1.2
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 1.8 Branch
: All All
: P1 normal (vote)
: mozilla1.9alpha1
Assigned To: Brendan Eich [:brendan]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-12 15:31 PST by Wan-Teh Chang
Modified: 2007-01-17 04:12 PST (History)
2 users (show)
bob: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (701 bytes, patch)
2007-01-12 15:35 PST, Kai Engert (:kaie)
no flags Details | Diff | Review
oops, I read bugmail and patched over in bug 362768 (985 bytes, patch)
2007-01-12 16:39 PST, Brendan Eich [:brendan]
brendan: review+
dveditz: approval1.8.1.2+
Details | Diff | Review

Description Wan-Teh Chang 2007-01-12 15:31:38 PST
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) 2007-01-12 15:35:37 PST
Created attachment 251325 [details] [diff] [review]
Patch v1

This patch avoids the infinite loop.
Comment 2 Kai Engert (:kaie) 2007-01-12 15:38:43 PST
Comment on attachment 251325 [details] [diff] [review]
Patch v1

Brendan, can you please review?
Comment 3 Brendan Eich [:brendan] 2007-01-12 16:39:07 PST
Created attachment 251329 [details] [diff] [review]
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] 2007-01-12 16:41:35 PST
Fixed on trunk:

js/src/jscntxt.c rev 3.98

/be
Comment 5 Brendan Eich [:brendan] 2007-01-12 16:43:06 PST
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 2007-01-12 16:44:38 PST
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] 2007-01-12 16:51:14 PST
(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] 2007-01-16 15:37:46 PST
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] 2007-01-16 17:02:34 PST
Fixed on the 1.8 branch:

js/src/jscntxt.c rev 3.60.4.11

/be

Note You need to log in before you can comment on or make changes to this bug.