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)
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•9 years ago
|
Crash Signature: [@ js::Debugger::wrapDebuggeeObject ]
Assignee | ||
Comment 1•9 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•9 years ago
|
||
With wrapDebuggeeValue seems cleaner IMO.
Choose whichever patch you prefer.
Attachment #8778595 -
Flags: review?(jimb)
Comment 3•9 years ago
|
||
Agree it's critical.
Comment 4•9 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•9 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•9 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•9 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•9 years ago
|
Keywords: checkin-needed
Comment 8•9 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•9 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•9 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•9 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•9 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•9 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•9 years ago
|
Keywords: checkin-needed
Comment 14•9 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•9 years ago
|
||
Arrgh, why is the commit message the one of the patch I attached by mistake?
Assignee | ||
Updated•9 years ago
|
Attachment #8779936 -
Attachment description: object-inspector-whitespace-property-v3.patch → This is a mistake. Ignore it.
Assignee | ||
Comment 16•9 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•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Assignee | ||
Comment 19•9 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•9 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•9 years ago
|
||
bugherder uplift |
You need to log in
before you can comment on or make changes to this bug.
Description
•