Last Comment Bug 351973 - GC hazard with unrooted ids in Object.toSource
: GC hazard with unrooted ids in Object.toSource
Status: VERIFIED FIXED
[sg:critical?]
: verified1.8.0.8, verified1.8.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Igor Bukanov
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-09-09 14:21 PDT by Igor Bukanov
Modified: 2007-05-26 01:32 PDT (History)
2 users (show)
dveditz: blocking1.8.0.8+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix (2.27 KB, patch)
2006-09-09 14:46 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
Fix v2 (1.66 KB, patch)
2006-09-09 15:02 PDT, Igor Bukanov
mrbkap: review+
brendan: superreview+
dveditz: approval1.8.0.8+
mtschrep: approval1.8.1+
Details | Diff | Splinter Review
js1_5/GC/regress-351973.js (2.78 KB, text/plain)
2006-09-14 04:29 PDT, Bob Clary [:bc:]
no flags Details

Description Igor Bukanov 2006-09-09 14:21:46 PDT
Consider the following example for js shell:

function removeAllProperties(o)
{
  for (var prop in o)
    delete o[prop];
  for (var i = 0; i != 50*1000; ++i) {
    var tmp = Math.sqrt(i+0.2);
    tmp = 0;
  }
  if (typeof gc == "function")
    gc();
}

function test()
{

  var o = {};
  o.first = { toSource: function() { removeAllProperties(o); } };
  for (var i = 0; i != 10; ++i) {
    o[Math.sqrt(i + 0.1)] = 1;
  }
  return o.toSource();
}

print(test());

When executed, it crashes the shell:
~/build/1.8.1/dbg/dist/bin> run-mozilla.sh xpcshell ~/s/x.js
Type Manifest File: /home/igor/build/1.8.1/dbg/dist/bin/components/xpti.dat
nsNativeComponentLoader: autoregistering begins.
nsNativeComponentLoader: autoregistering succeeded
nsNativeComponentLoader: registering deferred (0)
before 913968, after 9232, break 08127000
./run-mozilla.sh: line 131: 10065 Segmentation fault      "$prog" ${1+"$@"}

The reason for the crash is that js_EnterSharpObject returns an array of property ids and nothing roots the atoms that can present in the array. 
Thus the GC triggered either through explicit gc() call (shell) or a branch callback (browser) would lead to accessing the freed memory in Object.toSource() when properties are removed during the string assembling loop.
Comment 1 Igor Bukanov 2006-09-09 14:46:07 PDT
Created attachment 237533 [details] [diff] [review]
Fix

The fix prevents atoms from GC for the duration of sharp table presence.
Comment 2 Igor Bukanov 2006-09-09 15:02:01 PDT
Created attachment 237534 [details] [diff] [review]
Fix v2
Comment 3 Igor Bukanov 2006-09-09 15:02:44 PDT
(In reply to comment #2)
> Created an attachment (id=237534) [edit]
> Fix v2
> 

The previous patch did not compile ;)
Comment 4 Igor Bukanov 2006-09-09 15:08:05 PDT
The bug exists in older versions of the browser as well.
Comment 5 Igor Bukanov 2006-09-09 15:17:33 PDT
It looks like all usages of JS_Enumerate() has to be monitored. For example, AFAICS with my limited knowledge of XPConnect internals http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpcwrappedjsclass.cpp#343 has the same bug.
Comment 6 Brendan Eich [:brendan] 2006-09-09 21:34:35 PDT
Comment on attachment 237534 [details] [diff] [review]
Fix v2

Looks good, thanks.

My memory is failing me: didn't you have a patch to protect (via the mark phase) the JSIdArrays returned by JS_Enumerate?

/be
Comment 7 Igor Bukanov 2006-09-10 00:15:39 PDT
(In reply to comment #6)
> My memory is failing me: didn't you have a patch to protect (via the mark
> phase) the JSIdArrays returned by JS_Enumerate?

Fix for bug 340129 roots the elements of the mark table, not the ids array.
Comment 8 Mike Schroepfer 2006-09-10 11:12:06 PDT
Comment on attachment 237534 [details] [diff] [review]
Fix v2

a=schrep for js mem issue
Comment 9 Igor Bukanov 2006-09-11 04:18:07 PDT
I committed the patch from comment 2 to the trunk.
Comment 10 Igor Bukanov 2006-09-11 15:46:47 PDT
I committed the patch from comment 2 to MOZILLA_1_8_BRANCH:

Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.208.2.32; previous revision: 3.208.2.31
done
Comment 11 Igor Bukanov 2006-09-11 15:49:39 PDT
Comment on attachment 237534 [details] [diff] [review]
Fix v2

The patch applies to  1.8.0 branch as is. It maybe too late for 1.8.0.7 but I still ask.
Comment 12 Bob Clary [:bc:] 2006-09-14 04:29:00 PDT
Created attachment 238402 [details]
js1_5/GC/regress-351973.js
Comment 13 Bob Clary [:bc:] 2006-09-14 19:12:32 PDT
verified fixed 1.8 20060914 windows/linux 1.9 20060914 windows/mac*/linux
Comment 14 Daniel Veditz [:dveditz] 2006-09-19 15:56:52 PDT
Restoring lost blocking flag
Comment 15 Daniel Veditz [:dveditz] 2006-09-26 14:33:49 PDT
Comment on attachment 237534 [details] [diff] [review]
Fix v2

approved for 1.8.0 branch, a=dveditz for drivers
Comment 16 Igor Bukanov 2006-09-26 15:19:02 PDT
I committed the pacth from comment 2 to MOZILLA_1_8_0_BRANCH:

Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.208.2.12.2.14; previous revision: 3.208.2.12.2.13
done
Comment 17 Bob Clary [:bc:] 2006-09-27 12:38:41 PDT
verified fixed 1.8.0.8 20060927 windows/mac*/linux
Comment 18 Bob Clary [:bc:] 2007-02-08 22:15:06 PST
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-351973.js,v  <--  regress-351973.js
Comment 19 Simon Paquet [:sipaq] 2007-04-11 12:21:09 PDT
This bug is verified1.8.1. No need for blocking request.

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