Last Comment Bug 327712 - nsXBLProtoImplProperty::InstallMember doesn't root correctly
: nsXBLProtoImplProperty::InstallMember doesn't root correctly
Status: RESOLVED FIXED
[sg:critical?][patch]
: fixed1.8.1, verified1.8.0.4
Product: Core
Classification: Components
Component: XBL (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
: Hixie (not reading bugmail)
Mentors:
Depends on:
Blocks: 307312
  Show dependency treegraph
 
Reported: 2006-02-18 03:56 PST by David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
Modified: 2006-07-13 15:33 PDT (History)
8 users (show)
dveditz: blocking1.7.14?
dveditz: blocking‑aviary1.0.9?
dveditz: blocking1.8.0.4+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (5.41 KB, patch)
2006-02-18 03:58 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Splinter Review
patch (5.66 KB, patch)
2006-02-18 04:11 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
bzbarsky: review+
bzbarsky: superreview+
bzbarsky: approval‑branch‑1.8.1+
dveditz: approval1.8.0.4+
Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-02-18 03:56:28 PST
nsXBLProtoImplProperty::InstallMember doesn't root correctly because it doesn't give its temporary variable a name!  (LXR says this is the only use of nsAutoGCRoot with this problem.)
Comment 1 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-02-18 03:58:47 PST
Created attachment 212308 [details] [diff] [review]
patch

The first chunk is the important one, the rest is cleanup that I've had in my tree for ages (the call used to require a cx argument, it no loner does).
Comment 2 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-02-18 04:03:38 PST
Actually, we need to root the setter too, because DefineUCProperty can cause GC:

js_GC (/builds/trunk/mozilla/js/src/jsgc.c:1947)
js_NewGCThing (/builds/trunk/mozilla/js/src/jsgc.c:635)
AllocSlots (/builds/trunk/mozilla/js/src/jsobj.c:1925)
js_NewObject (/builds/trunk/mozilla/js/src/jsobj.c:2052)
js_CloneFunctionObject (/builds/trunk/mozilla/js/src/jsfun.c:2070)
JS_CloneFunctionObject (/builds/trunk/mozilla/js/src/jsapi.c:3414)
xpc_CloneJSFunction(XPCCallContext&, JSObject*, JSObject*) (/builds/trunk/mozilla/js/src/xpconnect/src/xpcwrappednativeinfo.cpp:56)
DefinePropertyIfFound (/builds/trunk/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:453)
XPC_WN_ModsAllowed_Proto_Resolve (/builds/trunk/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1574)
js_LookupPropertyWithFlags (/builds/trunk/mozilla/js/src/jsobj.c:2801)
js_LookupProperty (/builds/trunk/mozilla/js/src/jsobj.c:2660)
js_DefineNativeProperty (/builds/trunk/mozilla/js/src/jsobj.c:2524)
js_DefineProperty (/builds/trunk/mozilla/js/src/jsobj.c:2469)
DefineUCProperty (/builds/trunk/mozilla/js/src/jsapi.c:2381)
Comment 3 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-02-18 04:11:53 PST
Created attachment 212309 [details] [diff] [review]
patch
Comment 4 Boris Zbarsky [:bz] 2006-02-19 18:20:06 PST
Comment on attachment 212309 [details] [diff] [review]
patch

r+sr=bzbarsky, but we need similar changes to root newly-cloned function objects across JS_DefineUCProperty in nsXBLProtoImplMethod::InstallMember and the code added in bug 307040.

It looks like thanks to the wonders of XPConnect, Brendan's bug 299205 comment 10 is not quite right.  :(

We probably want this on both 1.8 branches.
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-02-20 22:37:55 PST
Fix checked in to trunk.
Comment 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-02-21 00:18:13 PST
Filed comment 4 as bug 328007.
Comment 7 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-03-10 10:40:34 PST
Checked in to MOZILLA_1_8_BRANCH.
Comment 8 Tim Riley [:timr] 2006-04-05 12:06:08 PDT
Can we get some info about how this affect people in the wild?  How will this  benefit the users?  We need this benefit before we can approve the patch for 1.8.0.3.
Comment 9 Darin Fisher 2006-04-10 12:27:46 PDT
Since some null checks are being removed by this patch, we'd like a little more clarity w.r.t. risk before accepting this patch.  Can someone please address Tim's question (comment #8).  Thanks!
Comment 10 Boris Zbarsky [:bz] 2006-04-10 12:48:50 PDT
> Since some null checks are being removed by this patch,

The null checks being removed were holdovers from back when we actually used the |cx| object there (to root stuff).  We don't anymore, so it's not an issue whether cx is null (not that it ever can be in this case).

As for comment 8, this just makes sure we don't access deleted objects by keeping them alive while we're working with them.  The effect should be fewer random exploitable deleted-memory-access crashes and nothing else.
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-04-11 03:59:36 PDT
The null-check removals were just cleanup -- very low risk cleanup, though.  But I could easily skip that part of the patch for a 1.8.0 branch landing.
Comment 12 Brendan Eich [:brendan] 2006-04-11 04:42:23 PDT
For crying out loud...

Read http://lxr.mozilla.org/mozilla/source/content/base/src/nsContentUtils.cpp#2552 and note that there is no dependency on a (JSContext *) cx cast from any aContext that is *not* passed to nsContentUtils::AddJSGCRoot.

The whole GetNativeContext call, assignment to cx, and test of cx are useless.  The patch should be taken in full, fiddling to remove these null check removals is a waste of time and a minor but non-zero merge hazard.

/be
Comment 13 Daniel Veditz [:dveditz] 2006-04-17 12:09:13 PDT
Comment on attachment 212309 [details] [diff] [review]
patch

approved for 1.8.0 branch, a=dveditz for drivers
Comment 14 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-04-17 16:37:53 PDT
Fix checked in to MOZILLA_1_8_0_BRANCH.
Comment 15 Jay Patel [:jay] 2006-04-20 14:49:49 PDT
v.fixed on 1.8.0 branch: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
rv:1.8.0.2) Gecko/20060420 Firefox/1.5.0.2, based on code inspection and brendan's comments.

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