js_ThreadDestructorCB calls JS_REMOVE_AND_INIT_LINK incorrectly, resulting in an infinite loop

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
JavaScript Engine
P1
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Wan-Teh Chang, Assigned: brendan)

Tracking

({fixed1.8.1.2})

1.8 Branch
mozilla1.9alpha1
fixed1.8.1.2
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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

10 years ago
Created attachment 251325 [details] [diff] [review]
Patch v1

This patch avoids the infinite loop.

Comment 2

10 years ago
Comment on attachment 251325 [details] [diff] [review]
Patch v1

Brendan, can you please review?
Attachment #251325 - Flags: review?(brendan)
(Assignee)

Comment 3

10 years ago
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
Assignee: general → brendan
Attachment #251325 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #251329 - Flags: review+
Attachment #251325 - Flags: review?(brendan)
(Assignee)

Comment 4

10 years ago
Fixed on trunk:

js/src/jscntxt.c rev 3.98

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Priority: -- → P1
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Comment 5

10 years ago
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
Attachment #251329 - Flags: approval1.8.1.2?
(Reporter)

Comment 6

10 years ago
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?
(Assignee)

Comment 7

10 years ago
(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 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
Attachment #251329 - Flags: approval1.8.1.2? → approval1.8.1.2+
(Assignee)

Comment 9

10 years ago
Fixed on the 1.8 branch:

js/src/jscntxt.c rev 3.60.4.11

/be
Keywords: fixed1.8.1.2

Updated

10 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.