Closed
Bug 1138157
Opened 6 years ago
Closed 6 years ago
Crash: Proxy revoked after trap extracted
Categories
(Core :: JavaScript: Standard Library, defect)
Core
JavaScript: Standard Library
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: anba, Assigned: evilpie)
References
Details
Attachments
(2 files)
16.15 KB,
patch
|
efaust
:
review+
|
Details | Diff | Splinter Review |
2.01 KB,
patch
|
efaust
:
review+
anba
:
feedback+
|
Details | Diff | Splinter Review |
The default implementations in DirectProxyHandler access `proxy->as<ProxyObject>().target()`, but side-effects may already have revoked the proxy. This leads to null-pointer deref in multiple places. function p(o = {}) { var {proxy, revoke} = Proxy.revocable(o, new Proxy({}, { get(t, pk, r) { print("get:", pk); revoke(); } })); return proxy; } Crash: Object.isExtensible(p()); Object.preventExtensions(p()); Object.getOwnPropertyDescriptor(p(), "a"); Object.defineProperty(p(), "a", {}); "a" in p(); p().a; p().a = 0; delete p().a; for (k in p()) ; Object.getOwnPropertyNames(p()); Incorrect behaviour: js> p(function() { return "ok" })() Expected: Returns "ok" Actual: Throws `TypeError: p(...) is not a function` js> new (p(function(){})) Expected: Throws `TypeError: illegal operation attempted on a revoked proxy` Actual: Throws `TypeError: p(...) is not a constructor`
Assignee | ||
Comment 1•6 years ago
|
||
Thanks, André. I definitely had enough of these bugs now. I will see if we can stop inheriting from DirectProxyHandler.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → evilpies
Component: JavaScript Engine → JavaScript: Standard Library
Assignee | ||
Comment 2•6 years ago
|
||
Why should the last one throw?
Reporter | ||
Comment 3•6 years ago
|
||
(In reply to Tom Schuster [:evilpie] from comment #2) > Why should the last one throw? The proxy should throw a TypeError when GetPrototypeFromConstructor is called. It's quite possible this part of the ES6 spec isn't yet implemented in SM. The relevant spec steps: 9.5.14 [[Construct]] ( argumentsList, newTarget), step 7.b -> 7.3.14 Construct (F, [argumentsList], [newTarget]), step 5 -> 9.2.3 [[Construct]], step 5.a -> 9.1.14 OrdinaryCreateFromConstructor, step 2 -> 9.1.15 GetPrototypeFromConstructor ( constructor, intrinsicDefaultProto ), step 3 -> 9.5.8 [[Get]] (P, Receiver), step 3 -> Access on revoked proxy
Assignee | ||
Comment 4•6 years ago
|
||
Indeed, thanks.
Assignee | ||
Comment 5•6 years ago
|
||
Attachment #8576915 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 6•6 years ago
|
||
André can I use your test here?
Attachment #8576920 -
Flags: review?(efaustbmo)
Attachment #8576920 -
Flags: feedback?(andrebargull)
Reporter | ||
Comment 7•6 years ago
|
||
Comment on attachment 8576920 [details] [diff] [review] v1 - Test Review of attachment 8576920 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Tom Schuster [:evilpie] from comment #6) > Created attachment 8576920 [details] [diff] [review] > v1 - Test > > André can I use your test here? Sure, why not. It may make sense to add tests for the currently missing traps (getPrototypeOf and setPrototypeOf). And also check things work as expected if a property is already present resp. not present. For example like: https://gist.github.com/anba/dcb1f739fd2137f73cbe
Attachment #8576920 -
Flags: feedback?(andrebargull) → feedback+
Comment 8•6 years ago
|
||
Comment on attachment 8576915 [details] [diff] [review] v1 - Change ScriptedDirectProxy to inherit directly from BaseProxy Review of attachment 8576915 [details] [diff] [review]: ----------------------------------------------------------------- Yep. This will silence the noise. r=me The new weakmap function needs tests. ::: js/src/proxy/ScriptedDirectProxyHandler.h @@ +16,3 @@ > public: > MOZ_CONSTEXPR ScriptedDirectProxyHandler() > + : BaseProxyHandler(&family, false, false) nit: aren't these two bollean params default? Either that, or comment what |false| means in each case.
Attachment #8576915 -
Flags: review?(efaustbmo) → review+
Updated•6 years ago
|
Attachment #8576920 -
Flags: review?(efaustbmo) → review+
Assignee | ||
Comment 9•6 years ago
|
||
I guess we don't really need weakmapKeyDelegate, because we can't unwrap a ScriptedDirectProxy.
Assignee | ||
Comment 10•6 years ago
|
||
Thanks again for your test André. https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9a9acbe6fde https://hg.mozilla.org/integration/mozilla-inbound/rev/2cf8496afd5d
Comment 11•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2cf8496afd5d
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in
before you can comment on or make changes to this bug.
Description
•