Closed Bug 1291011 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, defect)

50 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox49 --- unaffected
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: Oriol, Assigned: Oriol)

References

Details

(Keywords: crash)

Crash Data

Attachments

(2 files, 3 obsolete files)

Attached 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.
Attached 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
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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.

Attachment

General

Created:
Updated:
Size: