1291011 - Crash when the debugger retrieves the proxyTarget or proxyHandler of a revoked scripted proxy.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Oriol Brufau [:Oriol]]

Attachment: revoked-proxy-crash.patch

Run this code in a privileged console:

    Components.utils.import('resource://gre/modules/jsdebugger.jsm');
    var iframe = document.createElement("iframe");
    document.body.appendChild(iframe);
    addDebuggerToGlobal(this);
    var dbg = new Debugger().addDebuggee(iframe.contentWindow);
    var r = Proxy.revocable([1,2,3], {});
    r.revoke();
    dbg.makeDebuggeeValue(r.proxy).unwrap().proxyTarget;

Firefox crashes because proxyTarget and proxyHandler expect an object.

But in revoked proxies, [[ProxyTarget]] and [[ProxyHandler]] can be null.

Not sure if using wrapDebuggeeValue instead of wrapDebuggeeObject would be a better way, but I didn't manage to do it. So I simply added a conditional.

Comment 1

[Oriol Brufau [:Oriol]]

After bug 1274657 any page can make Firefox crash just by using

    var r = Proxy.revocable({}, {});
    r.revoke();
    console.log(r.proxy);

and telling the user to open the console. I think this means the bug is critical.

Comment 2

[Oriol Brufau [:Oriol]]

Attachment: revoked-proxy-crash-alternative.patch

With wrapDebuggeeValue seems cleaner IMO.

Choose whichever patch you prefer.

Comment 3

[Jim Blandy :jimb]

Agree it's critical.

Comment 4

[Jim Blandy :jimb]

Comment on attachment 8776694 [details] [diff] [review]
revoked-proxy-crash.patch

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

This is the version we should take; I think it's important to keep DebuggerObject::scriptedProxyTarget etc. well-typed, and Value is pretty dynamic.

The tests look good.

This needs accompanying changes in js/src/doc/Debugger/Debugger.Object.md.

Comment 5

[Jim Blandy :jimb]

Comment on attachment 8778595 [details] [diff] [review]
revoked-proxy-crash-alternative.patch

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

Declining this approach, due to Value being more loosely typed.

Comment 6

[Oriol Brufau [:Oriol]]

Attachment: revoked-proxy-crash-v2.patch (for Aurora)

Adding the doc changes.

Comment 7

[Jim Blandy :jimb]

Comment on attachment 8779161 [details] [diff] [review]
revoked-proxy-crash-v2.patch (for Aurora)

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

Thanks very much!

Comment 8

[Carsten Book [:Tomcat]]

has problems to apply:
applying revoked-proxy-crash-v2.patch
patching file js/src/vm/Debugger.cpp
Hunk #2 FAILED at 9895
1 out of 2 hunks FAILED -- saving rejects to file js/src/vm/Debugger.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh revoked-proxy-crash-v2.patch

Comment 9

[Oriol Brufau [:Oriol]]

I just pulled and updated to latest mozilla-central revision, and I can apply my patch cleanly.

Maybe the problem is that Debugger.cpp has been changed only in inbound?

Let's wait a bit for these changes to land in mozilla-central, then.

Comment 10

[Oriol Brufau [:Oriol]]

Attachment: This is a mistake. Ignore it.

Previous patch didn't apply because of https://hg.mozilla.org/mozilla-central/rev/1d9554d44cbc

Comment 11

[Jim Blandy :jimb]

Comment on attachment 8779936 [details] [diff] [review]
This is a mistake. Ignore it.

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

I think this is the wrong patch.

Comment 12

[Oriol Brufau [:Oriol]]

Attachment: revoked-proxy-crash-v3.patch

Arrgh! When I wanted to submit that patch to its bug I submitted the one for this bug, and now the opposite!

And thanks for your kind words in the mail you sent me.

Comment 13

[Jim Blandy :jimb]

Comment on attachment 8779938 [details] [diff] [review]
revoked-proxy-crash-v3.patch

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

Looks good!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de2206532859
When inspecting an object, do not trim property names except when displaying them. r=jimb

Comment 15

[Oriol Brufau [:Oriol]]

Arrgh, why is the commit message the one of the patch I attached by mistake?

Comment 16

[Oriol Brufau [:Oriol]]

Comment on attachment 8779161 [details] [diff] [review]
revoked-proxy-crash-v2.patch (for Aurora)

The fix should be uplifted to Aurora, but the latest patch won't apply cleanly.

This one should work.

Comment 17

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

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

Comment 19

[Oriol Brufau [:Oriol]]

Comment on attachment 8779161 [details] [diff] [review]
revoked-proxy-crash-v2.patch (for Aurora)

Approval Request Comment
[Feature/regressing bug #]: Bug 1282257
[User impact if declined]: Any page can make Firefox crash when the console is open.
[Describe test coverage new/current, TreeHerder]: Few days nightly coverage
[Risks and why]: Fixes a crash. Low risk, only takes into consideration that a JSObject* could be null, and updates the documentation and a test.
[String/UUID change made/needed]: None

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8779161 [details] [diff] [review]
revoked-proxy-crash-v2.patch (for Aurora)

Crash fix, new automated test, stabilized on Nightly for a few days, Aurora50+

Comment 21

[[Back July 11] Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0ccf48273f8e