1417962 - Crash [@ js::ScriptedProxyHandler::boxedValue_unbox] or Hit MOZ_CRASH(Should not end up in ScriptedProxyHandler::boxedValue_unbox) at js/src/proxy/ScriptedProxyHandler.cpp:1284

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision fc194660762d+ (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --disable-debug --enable-optimize, run with --fuzzing-safe):

let hook = {}
let Base = function() {}
Base.prototype = new Proxy(Base.prototype, hook);
class Derived extends Base {
    testPrimitiveReceiver() {
        super.foo = "Derived";
    }
}
Derived.prototype.testPrimitiveReceiver.call(null);


Backtrace:

received signal SIGSEGV, Segmentation fault.
js::ScriptedProxyHandler::boxedValue_unbox (this=0x1b43ff0 <js::ScriptedProxyHandler::singleton>, cx=0x7ffff5f15000, proxy=..., vp=...) at js/src/proxy/ScriptedProxyHandler.cpp:1284
#0  js::ScriptedProxyHandler::boxedValue_unbox (this=0x1b43ff0 <js::ScriptedProxyHandler::singleton>, cx=0x7ffff5f15000, proxy=..., vp=...) at js/src/proxy/ScriptedProxyHandler.cpp:1284
#1  0x000000000081205b in JS::ObjectOpResult::reportStrictErrorOrWarning (this=this@entry=0x7fffffffc6a0, cx=cx@entry=0x7ffff5f15000, obj=..., id=..., strict=strict@entry=true) at js/src/jsapi.cpp:183
#2  0x000000000050dfa2 in JS::ObjectOpResult::checkStrictErrorOrWarning (strict=true, id=..., obj=..., cx=0x7ffff5f15000, this=0x7fffffffc6a0) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/instrumentation/none/type/opt/dist/include/js/Class.h:204
#3  js::SetPropertySuper (cx=0x7ffff5f15000, obj=..., receiver=..., name=..., rval=..., strict=<optimized out>) at js/src/vm/Interpreter.cpp:5276
#4  0x00000000005141c0 in Interpret (cx=0x7ffff5f15000, state=...) at js/src/vm/Interpreter.cpp:2916
#5  0x000000000051f90a in js::RunScript (cx=0x7ffff5f15000, state=...) at js/src/vm/Interpreter.cpp:423
#6  0x000000000051fe5b in js::InternalCallOrConstruct (cx=cx@entry=0x7ffff5f15000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:495
#7  0x00000000005200b5 in InternalCall (cx=cx@entry=0x7ffff5f15000, args=...) at js/src/vm/Interpreter.cpp:522
#8  0x0000000000520118 in js::Call (cx=cx@entry=0x7ffff5f15000, fval=..., fval@entry=..., thisv=..., args=..., rval=...) at js/src/vm/Interpreter.cpp:541
#9  0x000000000085aeb2 in js::fun_call (cx=0x7ffff5f15000, argc=1, vp=0x7ffff41b3098) at js/src/jsfun.cpp:1237
#10 0x000000000051fc94 in js::CallJSNative (args=..., native=0x85ad60 <js::fun_call(JSContext*, unsigned int, JS::Value*)>, cx=0x7ffff5f15000) at js/src/jscntxtinlines.h:291
[...]
#23 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:8987
rax	0x1b6b880	28752000
rbx	0x7ffff5f15000	140737319620608
rcx	0x7fffffffc650	140737488340560
rdx	0xe344d0	14894288
rsi	0x7ffff5f15000	140737319620608
rdi	0x1b43ff0	28590064
rbp	0x0	0
rsp	0x7fffffffc5d8	140737488340440
r8	0x2b	43
r9	0x20	32
r10	0x201	513
r11	0x7ffff6b5edc0	140737332506048
r12	0x7fffffffc6a0	140737488340640
r13	0x7fffffffc650	140737488340560
r14	0x7ffff5f15060	140737319620704
r15	0x7ffff5f49080	140737319833728
rip	0x8c82a1 <js::ScriptedProxyHandler::boxedValue_unbox(JSContext*, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>) const+17>
=> 0x8c82a1 <js::ScriptedProxyHandler::boxedValue_unbox(JSContext*, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>) const+17>:	movl   $0x0,0x0
   0x8c82ac <js::ScriptedProxyHandler::boxedValue_unbox(JSContext*, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>) const+28>:	ud2

Comment 1

[Tom S. (please needinfo tschuster)]

Attachment: Don't unbox proxies when reporting strict assignment error

I didn't think proxies could actually appear in this case somehow. I think we can just not unbox proxies at all.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8929852 [details] [diff] [review]
Don't unbox proxies when reporting strict assignment error

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

Makes sense, yeah.

Comment 3

[André Bargull [:anba]]

Is this really a proxy issue or actually an issue that the receiver can be different from |obj| in super-property accesses? For example the following test case is probably still wrong, i.e. it shouldn't unbox the Date object.

---
let Base = function() {}
Base.prototype = new Date("2017-11-16T00:00:00.000Z");

class Derived extends Base {
    testPrimitiveReceiver() {
        super.foo = "Derived";
    }
}

// TypeError: can't assign to property "foo" on 1510790400000: not an object
Derived.prototype.testPrimitiveReceiver.call(null); 
---

Comment 4

[Tom S. (please needinfo tschuster)]

I guess you are right, but I am not sure how to fix this. We would probably have to carry around the value instead of the object.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6ace8b5531c
Don't unbox proxies when reporting strict assignment error. r=jandem

Comment 6

[Tom S. (please needinfo tschuster)]

I landed this for now, but we need to figure out a proper fix. I don't think we know that we are handling a super property lookup when throwing this exception.

Comment 7

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/e6ace8b5531c

Comment 8

[Gerry Chang [:gchang]]

Hi Tom,
Is this worth uplifting to 58?

Comment 9

[Tom S. (please needinfo tschuster)]

Yes, let's do that.

Comment 10

[Tom S. (please needinfo tschuster)]

Comment on attachment 8929852 [details] [diff] [review]
Don't unbox proxies when reporting strict assignment error

Approval Request Comment
[Feature/Bug causing the regression]: bug 1241966
[User impact if declined]: Potential crash
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Just disables code for proxies, which gives us the previous, non crashing behavior
[String changes made/needed]: None

Comment 11

[Gerry Chang [:gchang]]

Comment on attachment 8929852 [details] [diff] [review]
Don't unbox proxies when reporting strict assignment error

Fix a potential crash. Beta58+.

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/a93c70fa29f5

Comment 13

[Andrei Vaida [:avaida]]

(In reply to Tom Schuster [:evilpie] from comment #10)
> [Is this code covered by automated tests?]: Yes
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]:  No

Has automated coverage, does not need manual testing, per Tom.