429281 - JS_SetTrap does not unroot a fresh trap before calling free on it

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

Currently JS_SetTrap does not release a root for the newly created trap when, after relocking the runtime, the code discovers that the debugger has added a trap for the same pc on another thread. See http://lxr.mozilla.org/seamonkey/source/js/src/jsdbgapi.c#124 for details.

When this is triggered, js_GC() would eventually dereference a pointer to the freed memory when enumerating roots created with JS_AddRoot.

Comment 1

[Igor Bukanov]

Attachment: v1

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 315939 [details] [diff] [review]
v1

Wouldn't a more minimal change just remove junk's root if (junk) at the bottom?

/be

Comment 3

[Igor Bukanov]

(In reply to comment #2)
> (From update of attachment 315939 [details] [diff] [review])
> Wouldn't a more minimal change just remove junk's root if (junk) at the bottom?

That would do the job, but the proposed changes are more future-proof. For example, the patch from bug 422137 comment 64 relies on the this changes since there it is necessary to release the cloned code if the current looses the race. 

Comment 4

[Brendan Eich [:brendan]]

Let me have a look at your big patch for bug 422137 before reviewing here, then. Sorry for slow reviews -- traveling this week.

/be

Comment 5

[Igor Bukanov]

Attachment: v2

This is minimal patch: in just calls the missing js_RemoveRoot and initialized trap->closure before calling js_AddRoot(cx, &trap->closure, ...).

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 318102 [details] [diff] [review]
v2

Igor, do you know why your recent patches are attaching as application/octet-stream?

/be

Comment 7

[Igor Bukanov]

(In reply to comment #6)
> (From update of attachment 318102 [details] [diff] [review])
> Igor, do you know why your recent patches are attaching as
> application/octet-stream?

This is what happens when one run a debug build with js/tests and compilation running in parallel. The browser then may miss a click on the patch field, I guess. I have to remember to check the attachment the next time.

Comment 8

[Damon Sicore (:damons)]

Comment on attachment 318102 [details] [diff] [review]
v2

a1.9+=damons

Comment 9

[Igor Bukanov]

I checked in the patch from the comment 5 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1209504091&maxdate=1209504241&who=igor%25mir2.org