1072817 - Crash with Proxy.revocable

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox)

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Andrew McCreight [:mccr8]]

It looks like a proxy gets revoked and then we hit a null deref when we look at the proxy, so that doesn't sound too bad.

Comment 3

[Jan de Mooij [:jandem]]

NI from our proxy guru :)

Comment 4

[Eric Faust [:efaust]]

Attachment: Fix

Don't let DirectProxyHandler come in contact with revoked proxies. Instead, add the needed-but-not-yet-written shells of get/set prototype of hooks.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 8505052 [details] [diff] [review]
Fix

Review of attachment 8505052 [details] [diff] [review]:
-----------------------------------------------------------------

Eric, while I was shuffling proxy methods over in bug 1081255, I noticed that some BaseProxyHandler subclasses don't have methods that I think they ought to have. This isn't the only case.

For example, it seems like DirectProxyHandler ought to override everything or nearly everything. It appears to be missing defaultValue, isConstructor, watch/unwatch, and slice. Maybe it's OK to omit slice. But what about the others?

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Alterna-patch

So I independently-ish found this yesterday, as commented in bug 1052139 comment 9, and wrote a patch.  I also came to the conclusion that SDPH must override every single method in DPH, but before I could modify the patch to do that, I was pointed at this bug.  (Shows how much I'm reading bugmail these days, I guess.)

This patch is largely the same as the one posted here, just with a better (clearer, not implicating all the random trapping instanceof would) test in it.  May or may not be useful, but might as well post it since it's done.

Incidentally, I need these traps added before I can progress with bug 1052139, which adds a trap that'll go right next to these new ones in SDPH.

This is pretty much just going to be null-derefs, so I'm not sure why it's security-sensitive.

Comment 7

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5a4ac30f999

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f5a4ac30f999

Comment 9

[Carsten Book [:Tomcat]]

Hi Waldo,

sorry had to back this out in https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound&revision=accd28c1aaa3 since one of this changes/bugs caused perma failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3198627&repo=mozilla-inbound

btw there seems to be a checkin comment missing for https://hg.mozilla.org/integration/mozilla-inbound/rev/7ecf4e364d05 because that was the one that got backed out, so reopening just in case.

Comment 10

[Jeff Walden [:Waldo]]

Relanded the backed-out bit:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c795042b7dcd

That was  just a very minor bit of method-reordering, plus my test, so no actual bug-fixing.  So back to FIXED it goes now.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c795042b7dcd