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

RESOLVED FIXED in Firefox 50

Status

()

defect
--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Oriol, Assigned: Oriol)

Tracking

({crash})

50 Branch
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 unaffected, firefox50 fixed, firefox51 fixed)

Details

(crash signature)

Attachments

(2 attachments, 3 obsolete attachments)

Posted patch revoked-proxy-crash.patch (obsolete) — Splinter Review
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.
Attachment #8776694 - Flags: review?(jimb)
Crash Signature: [@ js::Debugger::wrapDebuggeeObject ]
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.
Severity: normal → critical
With wrapDebuggeeValue seems cleaner IMO.

Choose whichever patch you prefer.
Attachment #8778595 - Flags: review?(jimb)
Agree it's critical.
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.
Attachment #8776694 - Flags: review?(jimb) → review-
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.
Attachment #8778595 - Flags: review?(jimb) → review-
Adding the doc changes.
Attachment #8776694 - Attachment is obsolete: true
Attachment #8778595 - Attachment is obsolete: true
Attachment #8779161 - Flags: review?(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!
Attachment #8779161 - Flags: review?(jimb) → review+
Keywords: checkin-needed
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
Flags: needinfo?(oriol-bugzilla)
Keywords: checkin-needed
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.
Posted patch This is a mistake. Ignore it. (obsolete) — Splinter Review
Previous patch didn't apply because of https://hg.mozilla.org/mozilla-central/rev/1d9554d44cbc
Attachment #8779161 - Attachment is obsolete: true
Flags: needinfo?(oriol-bugzilla)
Attachment #8779936 - Flags: review?(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.
Attachment #8779936 - Flags: review?(jimb)
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.
Attachment #8779936 - Attachment is obsolete: true
Attachment #8779938 - Flags: review?(jimb)
Comment on attachment 8779938 [details] [diff] [review]
revoked-proxy-crash-v3.patch

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

Looks good!
Attachment #8779938 - Flags: review?(jimb) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de2206532859
When inspecting an object, do not trim property names except when displaying them. r=jimb
Keywords: checkin-needed
Arrgh, why is the commit message the one of the patch I attached by mistake?
Attachment #8779936 - Attachment description: object-inspector-whitespace-property-v3.patch → This is a mistake. Ignore it.
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.
Attachment #8779161 - Attachment description: revoked-proxy-crash-v2.patch → revoked-proxy-crash-v2.patch (for Aurora)
Attachment #8779161 - Attachment is obsolete: false
https://hg.mozilla.org/mozilla-central/rev/de2206532859
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Duplicate of this bug: 1295003
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
Attachment #8779161 - Flags: approval-mozilla-aurora?
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+
Attachment #8779161 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.