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)

18 Branch
x86
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla19
Tracking Status
firefox18 + verified

People

(Reporter: simon.lindholm10, Assigned: ejpbruel)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file, 2 obsolete files)

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);
Yep.  Looks like fallout from bug 703537.
Blocks: 703537
Severity: normal → critical
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: crash, regression
Crash Signature: [@ proxy]
(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
Attached patch Patch to be reviewed (obsolete) — Splinter Review
My first patch
Attachment #666328 - Flags: review?(ejpbruel)
Attachment #666328 - Flags: checkin?(ejpbruel)
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)
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;
> }
>
Attached patch removed the tabs, added spaces (obsolete) — Splinter Review
replaced the tabs that slipped in with spaces
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 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)
Comment on attachment 669701 [details] [diff] [review]
removed the tabs, added spaces

checkin-needed tends to work better ;)
Attachment #669701 - Flags: checkin?(ejpbruel)
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
Attachment #666328 - Attachment is obsolete: true
Attachment #669701 - Attachment is obsolete: true
(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.
Attached patch bug 793160Splinter Review
author: guidocalvano@yahoo.com
Attachment #679509 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/5a98250eb00c
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
(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?
(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?
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)
Depends on: 811343
(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?
(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).
(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.
Eddy - now that bug 811343 has been approved for beta, is there any more work to be done here for FF18?
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.

Attachment

General

Creator:
Created:
Updated:
Size: