Closed
Bug 1291011
Opened 8 years ago
Closed 8 years ago
Crash when the debugger retrieves the proxyTarget or proxyHandler of a revoked scripted proxy.
Categories
(Core :: JavaScript Engine, defect)
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)
6.80 KB,
patch
|
jimb
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
6.78 KB,
patch
|
jimb
:
review+
|
Details | Diff | 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)
Assignee | ||
Updated•8 years ago
|
Crash Signature: [@ js::Debugger::wrapDebuggeeObject ]
Assignee | ||
Comment 1•8 years ago
|
||
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
Assignee | ||
Comment 2•8 years ago
|
||
With wrapDebuggeeValue seems cleaner IMO. Choose whichever patch you prefer.
Attachment #8778595 -
Flags: review?(jimb)
Comment 3•8 years ago
|
||
Agree it's critical.
Comment 4•8 years ago
|
||
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 5•8 years ago
|
||
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-
Assignee | ||
Comment 6•8 years ago
|
||
Adding the doc changes.
Attachment #8776694 -
Attachment is obsolete: true
Attachment #8778595 -
Attachment is obsolete: true
Attachment #8779161 -
Flags: review?(jimb)
Comment 7•8 years ago
|
||
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+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 8•8 years ago
|
||
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
Assignee | ||
Comment 9•8 years ago
|
||
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.
Assignee | ||
Comment 10•8 years ago
|
||
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 11•8 years ago
|
||
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)
Assignee | ||
Comment 12•8 years ago
|
||
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 13•8 years ago
|
||
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+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 14•8 years ago
|
||
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
Assignee | ||
Comment 15•8 years ago
|
||
Arrgh, why is the commit message the one of the patch I attached by mistake?
Assignee | ||
Updated•8 years ago
|
Attachment #8779936 -
Attachment description: object-inspector-whitespace-property-v3.patch → This is a mistake. Ignore it.
Assignee | ||
Comment 16•8 years ago
|
||
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
Comment 17•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/de2206532859
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Assignee | ||
Comment 19•8 years ago
|
||
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?
Assignee | ||
Updated•8 years ago
|
status-firefox49:
--- → unaffected
status-firefox50:
--- → affected
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+
Comment 21•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/0ccf48273f8e
You need to log in
before you can comment on or make changes to this bug.
Description
•