Last Comment Bug 600853 - last ditch GC and OOM reporting with title lock held
: last ditch GC and OOM reporting with title lock held
Status: RESOLVED FIXED
: [sg:critical]
:
Product: Core
Classification: Components
Component: JavaScript Engine
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Igor Bukanov
: general
:
: 633869
: 477627
  Show dependency treegraph
 
Reported: 2010-09-30 08:37 PDT by Igor Bukanov
Modified: 2011-03-29 19:26 PDT (History)
10 users (show)
See Also:
Crash Signature:
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  -
  unaffected
  needed
  .14-fixed
  needed
  .17-fixed


Attachments
v1 (28.43 KB, patch)
2010-10-08 13:34 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
v2 (24.59 KB, patch)
2010-10-13 05:34 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
patch for 192 v1 (12.55 KB, patch)
2010-12-06 07:48 PST, Igor Bukanov
brendan: review+
dveditz: approval1.9.2.14+
Details | Diff | Splinter Review
patch for 191 (13.14 KB, patch)
2010-12-15 10:55 PST, Igor Bukanov
igor: review+
dveditz: approval1.9.1.17+
Details | Diff | Splinter Review
regression fix (654 bytes, patch)
2011-01-17 06:36 PST, Igor Bukanov
brendan: review+
clegnitto: approval1.9.2.14+
clegnitto: approval1.9.1.17+
Details | Diff | Splinter Review

Summon comment box

Description Igor Bukanov 2010-09-30 08:37:08 PDT
obj_toSource contains:

        ok = obj->lookupProperty(cx, id, &obj2, &prop);
...
        idstr = js_ValueToString(cx, IdToValue(id));
...
        
        JS_UNLOCK_OBJ(cx, obj2);

Here js_ValueToString allocates a string when id is an integer. This implies that the last-ditch GC and OOM reporting will run with title lock held.

Similarly JS_SetWatchPoint contains:

    if (!js_LookupProperty(cx, obj, propid, &pobj, &prop))
...
        watcher = js_WrapWatchedSetter(cx, propid, shape->attributes(), shape->setter());
...
JS_UNLOCK_OBJ(cx, obj);

Here js_WrapWatchedSetter allocates at least new Function leading to the same bug.

Marking as security-sensitive as this is related to the bug 569162 and in case other places contains the similar pattern but when an arbitrary JS code can run.
Comment 1 Igor Bukanov 2010-10-07 05:46:11 PDT
The bugs in the comment 0 are a real hazards if the corresponding code is run against thread-shared objects. If another thread is waiting for the current thread to share the objects, then, as a consequence of changes in the big 477627, the last-ditch GC shares the object. That allows that thread to run until it suspends its the request. That other thread can then delete the shapes and let the GC to collect them before the first thread accesses them.
Comment 2 Igor Bukanov 2010-10-08 13:34:34 PDT
Created attachment 481920 [details] [review]
v1

There are a lot of places where the the GC allocation happens when the object is locked. It is rather non-trivial and often very error-prone to change the corresponding code. So the patch takes simpler approach of just disabling the last-ditch GC when the current thread has any titles to share.
Comment 3 Robert Sayre 2010-10-12 14:19:19 PDT
is this ready for review?
Comment 4 Igor Bukanov 2010-10-12 14:35:38 PDT
(In reply to comment #3)
> is this ready for review?

It seems the try server gives more orange than usual for this patch. I am investigating that.
Comment 5 Igor Bukanov 2010-10-13 05:34:27 PDT
Created attachment 482832 [details] [review]
v2

here is a rebased patch. It passes the tryserver and the oranges that I have seen comes from unrelated patch.

To allow for simpler detection of waiting titles for the current thread the patch moves the title sharing list into JSThread.
Comment 6 Johnny Stenback (:jst, jst@mozilla.com) 2010-11-09 14:19:51 PST
Brendan, this is a beta8 blocker and a security bug, can you do this review?
Comment 7 Jeff Walden (remove +bmo to email) 2010-11-09 14:42:20 PST
We killed titles within the last two weeks, so is this even a bug any more?
Comment 8 Brendan Eich [:brendan] 2010-11-09 14:45:40 PST
(In reply to comment #7)
> We killed titles within the last two weeks, so is this even a bug any more?

For the branches.

/be
Comment 9 Johnny Stenback (:jst, jst@mozilla.com) 2010-11-23 13:55:10 PST
Igor, is the attached patch good for older branches as well? If not, please upload a branch version.
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2010-11-30 13:42:03 PST
Igor, ping?
Comment 11 Igor Bukanov 2010-11-30 14:40:06 PST
(In reply to comment #10)
> Igor, ping?

We need a new patch. I should have it this week.
Comment 12 Johnny Stenback (:jst, jst@mozilla.com) 2010-11-30 16:05:17 PST
Excellent, thanks Igor!
Comment 13 Igor Bukanov 2010-12-06 07:48:46 PST
Created attachment 495512 [details] [review]
patch for 192 v1

Here is a minimal patch. It makes sure that the GC allocation paths do not invoke the last ditch GC if the current thread has titles to share. I will ask for a review after more testing.
Comment 14 Igor Bukanov 2010-12-07 07:41:37 PST
Comment on attachment 495512 [details] [review]
patch for 192 v1

The patch passes testing.
Comment 15 Brendan Eich [:brendan] 2010-12-08 13:23:55 PST
Comment on attachment 495512 [details] [review]
patch for 192 v1

Looks good -- any reason not to make HAS_TITLES_TO_SHARE be an inline hasTitlesToShare method?

/be
Comment 16 Igor Bukanov 2010-12-08 13:54:20 PST
(In reply to comment #15)
> any reason not to make HAS_TITLES_TO_SHARE be an inline
> hasTitlesToShare method?

The reason is the patch minimality to avoid extra #ifdef C++ as LiveConnect is alive and kicking on 1.9.2.
Comment 17 Brendan Eich [:brendan] 2010-12-08 15:16:22 PST
Agh, you reminded me of the nameless horror that was L***C*****t!

/be
Comment 18 Igor Bukanov 2010-12-15 10:55:59 PST
Created attachment 497823 [details] [review]
patch for 191

191 version required a minimal backporting from 192. Here is a plain diff between 192 and 191 patches:

> @@ -1807,8 +1809,9 @@ IsGCThresholdReached(JSRuntime *rt)
129c122
< -           rt->gcBytes >= rt->gcTriggerBytes;
---
> -           rt->gcBytes / rt->gcTriggerFactor >= rt->gcLastBytes / 100;
131c124,125
< +            rt->gcBytes >= rt->gcTriggerBytes) && !HAS_TITLES_TO_SHARE(cx);
---
> +            rt->gcBytes / rt->gcTriggerFactor >= rt->gcLastBytes / 100) &&
> +           !HAS_TITLES_TO_SHARE(cx);
134,135c128,129
<  template <class T> static JS_INLINE T*
< @@ -1840,7 +1842,7 @@ NewGCThing(JSContext *cx, uintN flags)
---
> @@ -2298,9 +2301,10 @@ js_AddAsGCBytes(JSContext *cx, size_t sz
>      JSRuntime *rt;
>  
>      rt = cx->runtime;
> -    if (rt->gcBytes >= rt->gcMaxBytes ||
> -        sz > (size_t) (rt->gcMaxBytes - rt->gcBytes) ||
> -        IsGCThresholdReached(rt)) {
> +    if ((rt->gcBytes >= rt->gcMaxBytes ||
> +         sz > (size_t) (rt->gcMaxBytes - rt->gcBytes) ||
> +         IsGCThresholdReached(cx)) &&
> +        !HAS_TITLES_TO_SHARE(cx)) {
>          if (JS_ON_TRACE(cx)) {
>              /*
>               * If we can't leave the trace, signal OOM condition, otherwise

Here 1.9.1-only js_AddAsGCBytes required an extra change to disable GC when HAS_TITLES_TO_SHARE is true.
Comment 19 Daniel Veditz 2010-12-20 10:37:38 PST
Comment on attachment 495512 [details] [review]
patch for 192 v1

Approved for 1.9.2.14, a=dveditz for release-drivers
Comment 20 Daniel Veditz 2010-12-20 10:37:55 PST
Comment on attachment 497823 [details] [review]
patch for 191

Approved for 1.9.1.17, a=dveditz for release-drivers
Comment 21 Robert Sayre 2011-01-05 15:45:19 PST
Igor, could you land this on the branch?
Comment 23 Bob Clary [:bc:] 2011-01-09 22:30:25 PST
(In reply to comment #22)

> http://hg.mozilla.org/releases/mozilla-1.9.1/rev/81e4317eb7f8

FYI, I think this is related to an ./shell.js:701 out of memory error (jit, shell only) I started seeing with 2 tests on 1.9.1 linux 32bit with 4G (the only jstests I still run regularly). It seems this would be expected behavior from this patch in some cases and probably is not of concern. I thought I would mention it though. The tests in question are:

js1_5/extensions/regress-350531.js and js1_6/extensions/regress-456826.js
Comment 24 Igor Bukanov 2011-01-10 00:54:11 PST
(In reply to comment #23)
э> FYI, I think this is related to an ./shell.js:701 out of memory error (jit,
> shell only) I started seeing with 2 tests on 1.9.1 linux 32bit with 4G (the
> only jstests I still run regularly). 

Is this only n 1.9.1 or 1.9.2 is also affected?
Comment 25 Bob Clary [:bc:] 2011-01-10 07:55:53 PST
1.9.1 only.
Comment 26 Igor Bukanov 2011-01-17 05:55:33 PST
The failing test cases revealed a bug in landed patches. So I clear the status the status and will post the regression fixes soon.
Comment 27 Igor Bukanov 2011-01-17 06:36:20 PST
Created attachment 504450 [details] [review]
regression fix

When NewArena returns NULL, it signals to to the GC if possible or report OOM. The essence of the landed patch was to prevent that GC form happening when the current thread has titles to share. So the idea was to ignore the heap size limit in NewArena with to-be-shared-titles and report OOM when when when NewArena failed to get new GC arenas via mmap. 

Needless to say, the landed patch did exactly the opposite - it would ignore the limit when there were no waiting titles and would enforce the limit where there are titles. This patch fixes the typo. It applies to both ranches as-is.
Comment 28 Igor Bukanov 2011-01-19 06:53:31 PST
Brendan - do you have time to look to the regression fix today? If not I will have to back out the landed patches.
Comment 29 Brendan Eich [:brendan] 2011-01-19 13:16:55 PST
Comment on attachment 504450 [details] [review]
regression fix

Sorry for the tardy r+ -- obvious fix now that you point it out.

/be
Comment 30 Igor Bukanov 2011-01-19 13:19:16 PST
Comment on attachment 504450 [details] [review]
regression fix

asking for regression fix approval
Comment 31 Christian Legnitto [:LegNeato] 2011-01-19 14:19:54 PST
Comment on attachment 504450 [details] [review]
regression fix

Approving regression fix
Comment 33 Daniel Veditz 2011-02-10 14:28:05 PST
Since 2.0 is marked "unaffected" and the branches are patched this bug is FIXED, right?
Comment 34 Christian Legnitto [:LegNeato] 2011-03-02 20:16:07 PST
I backed this out of 1.9.2 (default and 1.9.2.14 relbranch):

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/844ae0377afa
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a50a49f952c0

(see bug 633869)
Comment 35 Ludovic Hirlimann [:Usul ] 2011-03-03 01:13:37 PST
Shall this be reopened then ?

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