Closed
Bug 793160
Opened 12 years ago
Closed 12 years ago
Crash [@ proxy] with proxy of object with null prototype
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla19
People
(Reporter: simon.lindholm10, Assigned: ejpbruel)
References
Details
(Keywords: crash, regression)
Crash Data
Attachments
(1 file, 2 obsolete files)
1.35 KB,
patch
|
guidocalvano
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Linux i686; rv:18.0) Gecko/18.0 Firefox/18.0 Build ID: 20120921030601 Steps to reproduce: new Proxy(Object.create(null), {}) Actual results: Firefox crashes with a null point dereference. From an outsider perspective it seems that target->getProto() is null in jsproxy.cpp, line 3115: JSObject *proto = target->getProto(); JSObject *proxy = NewProxyObject(cx, &ScriptedDirectProxyHandler::singleton, ObjectValue(*target), proto, proto->getParent(), fun, fun);
Comment 1•12 years ago
|
||
Yep. Looks like fallout from bug 703537.
Blocks: 703537
Severity: normal → critical
Status: UNCONFIRMED → NEW
tracking-firefox18:
--- → ?
Ever confirmed: true
Keywords: crash,
regression
Updated•12 years ago
|
Crash Signature: [@ proxy]
Assignee | ||
Comment 2•12 years ago
|
||
(In reply to Simon Lindholm from comment #0) > User Agent: Mozilla/5.0 (X11; Linux i686; rv:18.0) Gecko/18.0 Firefox/18.0 > Build ID: 20120921030601 > > Steps to reproduce: > > new Proxy(Object.create(null), {}) > > > Actual results: > > Firefox crashes with a null point dereference. From an outsider perspective > it seems that target->getProto() is null in jsproxy.cpp, line 3115: > > JSObject *proto = target->getProto(); > JSObject *proxy = NewProxyObject(cx, > &ScriptedDirectProxyHandler::singleton, > ObjectValue(*target), proto, > proto->getParent(), > fun, fun); Thanks for reporting this Simon!
Assignee: general → ejpbruel
Updated•12 years ago
|
My first patch
Attachment #666328 -
Flags: review?(ejpbruel)
Attachment #666328 -
Flags: checkin?(ejpbruel)
Comment 4•12 years ago
|
||
Comment on attachment 666328 [details] [diff] [review] Patch to be reviewed You should only request checkin once the patch has received r+.
Attachment #666328 -
Flags: checkin?(ejpbruel)
Assignee | ||
Comment 5•12 years ago
|
||
Comment on attachment 666328 [details] [diff] [review] Patch to be reviewed Review of attachment 666328 [details] [diff] [review]: ----------------------------------------------------------------- Please fix the indenting so that it lines up with the rest of the block: 3134 JSObject *proto = target->getProto(); 3135 JSObject *parent = NULL; 3136 if(proto) 3137 parent = proto->getParent(); r+ with comments addressed
Attachment #666328 -
Flags: review?(ejpbruel) → review+
Comment on attachment 666328 [details] [diff] [review] Patch to be reviewed >diff -r d912cef9b337 js/src/jit-test/tests/proxy/testBug793160.js >--- /dev/null Thu Jan 01 00:00:00 1970 +0000 >+++ b/js/src/jit-test/tests/proxy/testBug793160.js Sun Sep 30 17:04:57 2012 +0200 >@@ -0,0 +1,3 @@ >+var obj = new Proxy(Object.create(null), {}); >+assertEq(typeof obj, 'object'); >+assertEq(obj != null, true); >diff -r d912cef9b337 js/src/jsproxy.cpp >--- a/js/src/jsproxy.cpp Sun Sep 30 09:53:39 2012 +0200 >+++ b/js/src/jsproxy.cpp Sun Sep 30 17:04:57 2012 +0200 >@@ -3127,19 +3127,22 @@ proxy(JSContext *cx, unsigned argc, jsva > } > JSObject *target = NonNullObject(cx, args[0]); > if (!target) > return false; > JSObject *handler = NonNullObject(cx, args[1]); > if (!handler) > return false; > JSObject *proto = target->getProto(); >+ JSObject *parent = NULL; >+ if(proto) >+ parent = proto->getParent(); > JSObject *fun = target->isCallable() ? target : NULL; > JSObject *proxy = NewProxyObject(cx, &ScriptedDirectProxyHandler::singleton, >- ObjectValue(*target), proto, proto->getParent(), >+ ObjectValue(*target), proto, parent, > fun, fun); > if (!proxy) > return false; > SetProxyExtra(proxy, 0, ObjectOrNullValue(handler)); > vp->setObject(*proxy); > return true; > } >
Reporter | ||
Comment 8•12 years ago
|
||
I don't know much more about parents than what's stated at the JS_GetParent wiki page, but asking out of curiosity: - Why is proto->getParent() used here and not target->getParent()? - Is it okay to give arbitrary proxies NULL parents?
Comment 9•12 years ago
|
||
Comment on attachment 669701 [details] [diff] [review] removed the tabs, added spaces Eddy, I think your comments were addressed here so this should be landed, right?
Attachment #669701 -
Flags: checkin?(ejpbruel)
Updated•12 years ago
|
Keywords: checkin-needed
Comment 10•12 years ago
|
||
Comment on attachment 669701 [details] [diff] [review] removed the tabs, added spaces checkin-needed tends to work better ;)
Attachment #669701 -
Flags: checkin?(ejpbruel)
Comment 11•12 years ago
|
||
Actually, this was bitrotted by bug 787856. This is going to need revision before it can land. Also, what happens with AssertEq in a test when it runs on a non-debug build? This should probably go through Try first. Either myself or Eddy can help with the Try run. Guido, sorry about this delay :(. Once you've got a rebased patch that's got r+ and passes on Try, add checkin-needed back to the keywords and I promise I'll land it shortly afterwards. Also, while you're making revisions to this, please check out the link below so that your patch has all the needed commit information in it. It makes life much easier for those checking in on your behalf. Thanks! https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in
Keywords: checkin-needed
Updated•12 years ago
|
Attachment #666328 -
Attachment is obsolete: true
Updated•12 years ago
|
Attachment #669701 -
Attachment is obsolete: true
Assignee | ||
Comment 12•12 years ago
|
||
(In reply to Ryan VanderMeulen from comment #11) > Actually, this was bitrotted by bug 787856. This is going to need revision > before it can land. Also, what happens with AssertEq in a test when it runs > on a non-debug build? This should probably go through Try first. Either > myself or Eddy can help with the Try run. > > Guido, sorry about this delay :(. Once you've got a rebased patch that's got > r+ and passes on Try, add checkin-needed back to the keywords and I promise > I'll land it shortly afterwards. > > Also, while you're making revisions to this, please check out the link below > so that your patch has all the needed commit information in it. It makes > life much easier for those checking in on your behalf. Thanks! > https://developer.mozilla.org/en-US/docs/ > Creating_a_patch_that_can_be_checked_in Guido told me he'll file a rebased patch tonight. I can give it a run on try and check it in first thing tomorrow.
Assignee | ||
Comment 14•12 years ago
|
||
Running on try: https://tbpl.mozilla.org/?tree=Try&rev=c59f1852bf21
Assignee | ||
Comment 15•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a98250eb00c
Comment 16•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5a98250eb00c
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Comment 17•12 years ago
|
||
(In reply to Eddy Bruel [:ejpbruel] from comment #15) > https://hg.mozilla.org/integration/mozilla-inbound/rev/5a98250eb00c Are we ready to uplift this on aurora?
Assignee | ||
Comment 18•12 years ago
|
||
(In reply to bhavana bajaj [:bajaj] from comment #17) > (In reply to Eddy Bruel [:ejpbruel] from comment #15) > > https://hg.mozilla.org/integration/mozilla-inbound/rev/5a98250eb00c > > Are we ready to uplift this on aurora? I guess so. I'm not too familiar with the process for landing stuff on aurora. As a reviewer for this patch, do you need my help with this?
Reporter | ||
Comment 19•12 years ago
|
||
Any thoughts on the questions in comment 8? In particular; > a = new Proxy(Object.create(null), {}); > w = Components.utils.getGlobalForObject(a); now gives an Assertion failure: isGlobal(), at ../../../js/src/vm/GlobalObject.h:499 (and in release builds has w === a)
Comment 21•12 years ago
|
||
(In reply to Simon Lindholm from comment #8) > - Is it okay to give arbitrary proxies NULL parents? It doesn't stay null - it'll default to the global object somewhere in the bowels of spidermonkey. > - Why is proto->getParent() used here and not target->getParent()? The parent chain is sort of vestigial, so it doesn't really matter from the perspective of the engine, so long as the parent chain eventually leads to the global. Presumably the parent chain is being used to find the proto at some point? Eddy, can you shed some light on this?
Comment 22•12 years ago
|
||
(In reply to Eddy Bruel [:ejpbruel] from comment #18) > (In reply to bhavana bajaj [:bajaj] from comment #17) > > (In reply to Eddy Bruel [:ejpbruel] from comment #15) > > > https://hg.mozilla.org/integration/mozilla-inbound/rev/5a98250eb00c > > > > Are we ready to uplift this on aurora? > > I guess so. I'm not too familiar with the process for landing stuff on > aurora. As a reviewer for this patch, do you need my help with this? Probably, yes. Either you or Guido should send out an approval request (probably for beta, given that 18 is being moved from aurora to beta this Monday). There's a few questions about risk and reward to answer for this. Once it gets approval, it should be pushed/transplanted to the appropriate repo (releases/mozilla-beta probably).
Assignee | ||
Comment 23•12 years ago
|
||
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #22) > (In reply to Eddy Bruel [:ejpbruel] from comment #18) > > (In reply to bhavana bajaj [:bajaj] from comment #17) > > > (In reply to Eddy Bruel [:ejpbruel] from comment #15) > > > > https://hg.mozilla.org/integration/mozilla-inbound/rev/5a98250eb00c > > > > > > Are we ready to uplift this on aurora? > > > > I guess so. I'm not too familiar with the process for landing stuff on > > aurora. As a reviewer for this patch, do you need my help with this? > > Probably, yes. Either you or Guido should send out an approval request > (probably for beta, given that 18 is being moved from aurora to beta this > Monday). There's a few questions about risk and reward to answer for this. > Once it gets approval, it should be pushed/transplanted to the appropriate > repo (releases/mozilla-beta probably). I've already done an approval request for bug 811343, which fixes this bug along with some other issues.
Comment 25•12 years ago
|
||
Eddy - now that bug 811343 has been approved for beta, is there any more work to be done here for FF18?
Updated•12 years ago
|
status-firefox18:
--- → fixed
Comment 26•12 years ago
|
||
Reproduced the issue on Nightly 2012-10-01. No crashes on FF 18b3 on Win 7, Ubuntu 12.04 and Mac OS X 10.8.2. Verified fixed.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•