1656701 - Crash [@ js::GetOwnPropertyDescriptor] with private field and Proxy

Status: RESOLVED DUPLICATE of bug 1653567

(Core :: JavaScript Engine, defect, P2)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20200727-932240e49142 (opt build, run with --fuzzing-safe --no-threads --enable-private-fields):

class A83 {
  #z = () => {}
  static chainTest(o25) {
    o25?.#z().#z()?.#z();
  }
};
b39 = new Proxy({}, {});
A83.chainTest(b39);

Backtrace:

received signal SIGSEGV, Segmentation fault.
0x5689b339 in js::GetOwnPropertyDescriptor(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::PropertyDescriptor>) ()
#0  0x5689b339 in js::GetOwnPropertyDescriptor(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::PropertyDescriptor>) ()
#1  0x5677f81a in js::BaseProxyHandler::getPrivate(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) const ()
#2  0x567a1777 in js::Proxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) ()
#3  0x566db427 in Interpret(JSContext*, js::RunState&) ()
[...]
#12 0x565cd3f4 in main ()
eax	0x0	0
ebx	0x585a1000	1482297344
ecx	0xffffab8c	-21620
edx	0xffffab88	-21624
esi	0xffffabb8	-21576
edi	0xffffabb8	-21576
ebp	0xffffab78	4294945656
esp	0xffffab30	4294945584
eip	0x5689b339 <js::GetOwnPropertyDescriptor(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::PropertyDescriptor>)+41>
=> 0x5689b339 <_ZN2js24GetOwnPropertyDescriptorEP9JSContextN2JS6HandleIP8JSObjectEENS3_INS2_11PropertyKeyEEENS2_13MutableHandleINS2_18PropertyDescriptorEEE+41>:	mov    (%eax),%eax
   0x5689b33b <_ZN2js24GetOwnPropertyDescriptorEP9JSContextN2JS6HandleIP8JSObjectEENS3_INS2_11PropertyKeyEEENS2_13MutableHandleINS2_18PropertyDescriptorEEE+43>:	mov    (%eax),%eax

This crash is really weird as it only reproduces on a 32-bit non-debug optimized build. If I try anything else (all 64-bit builds as well as the 32-bit debug build), I get TypeError: Trying to read undeclared field.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20200727203201-932240e49142.
The bug appears to have been introduced in the following build range:
> Start: 2f2cb7c9bcced2ec48d8fba6a369af03d1b46f64 (20200720214226)
> End: f0ed5585b6adac269e39dc3241fba9c4f52aa4c4 (20200721032334)
> Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2f2cb7c9bcced2ec48d8fba6a369af03d1b46f64&tochange=f0ed5585b6adac269e39dc3241fba9c4f52aa4c4

Comment 3

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Crash Bytecode

Great test case. I'm deeply confused by the platform requirement. Here's my initial diagnosis though (I'm still going to try to figure out the platform dependence issue). I believe this is fixed by Bug 1653567, which changed the bytecode emission strategy for private accesses, and blocks this particular issue. 

Looking at the Bytecode, dumped directly from a crash: 

```
$1 = {<js::RootedBase<JSScript*, JS::Rooted<JSScript*> >> = {<js::MutableWrappedPtrOperations<JSScript*, JS::Rooted<JSScript*> >> = {<js::WrappedPtrOperations<JSScript*, JS::Rooted<JSScript*> >> = {<No data fields>}, <No data fields>}, <No data fields>}, stack = 0xf6b0a014, prev = 0xff891acc, ptr = 0xf69740b0}
(rr) call js::DumpScript(cx, script.ptr)
loc   line  op
----- ----  --
main:
00000:   4  GetArg 0                    # 
00003:   4  Dup                         # 
00004:   4  Undefined                   # 
00005:   4  StrictNe                    # 
00006:   4  And 20 (+14)                # 
00011:   4  JumpTarget (ic: 4)          # 
00016:   4  Pop                         # 
00017:   4  Dup                         # 
00018:   4  Null                        # 
00019:   4  StrictNe                    # 
00020:   4  JumpTarget (ic: 5)          # 
00025:   4  Not                         # 
00026:   4  IfEq 41 (+15)               # 
00031:   4  JumpTarget (ic: 7)          # 
00036:   4  Goto 132 (+96)              # 
00041:   4  JumpTarget (ic: 7)          # 
00046:   4  Dup                         # 
00047:   4  GetAliasedVar "#z" (hops = 0, slot = 2) # 
00052:   4  CheckAliasedLexical "#z" (hops = 0, slot = 2) # 
00057:   4  CallElem                    # 
<snip> 
```

You can see we're using CallElem; it's worth noting that I had only ever implemented special case code for private fields for Init/Get and Set. I'd *not* implemented special support for `CallElem`. This means that the first `CallElem` may freely try to read `#z` off the proxy, without there having been any check for `#z` existing: that checking would have happened as part of `{Init,Get,Set}PrivateElem`. Obviously this is insufficient. We ended up aborting inside of `getPrivate` where we asserted `expando` already existed (as would have been invariant should we only ever taken `Init,Get,Set` paths. 

Now. I still don't have a good answer for why this failure is dependent on architecture. This seems like a clear case that was missed and should crash everywhere. I need to dig deeper still to see if I can't answer that question. 

This is fixed by Bug 1653567, which changed the bytecode emission strategy to instead separate the check into a separate operation (`CheckPrivateField`). 

The relevant bytecode for chainTest becomes: 

```
00046:  Dup                             # o25 o25
00047:  GetAliasedVar "#z" (hops = 0, slot = 2) # o25 o25 #z
00052:  CheckAliasedLexical "#z" (hops = 0, slot = 2) # o25 o25 #z
00057:  CheckPrivateField 1 5           # o25 <unknown> <unknown> <unknown>
00060:  Pop                             # o25 <unknown> <unknown>
00061:  CallElem                        # o25 <unknown>[<unknown>]
```

So we first check and throw, before doing the callelem. Which semantically is how things ought to work.

Comment 4

[Matthew Gaudet (he/him) [:mgaudet]]

(The above comment got splinched as the result of a bugzilla bug methinks)

Comment 5

[Matthew Gaudet (he/him) [:mgaudet]]

If you gently modify the test case, you can see other peculiarities of the old implementation:

class A {
  #z = () => {};

  static chainTest(o) {
    o?.#z().#z()?.#z();
  }
};

var obj = {};

A.chainTest(obj);

you get an error messages that say o[#z] is not a function.

Still. This is fixed by the new implementation.

Comment 6

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Bug marked as FIXED but still reproduces on mozilla-central 20200727203201-932240e49142.

Comment 7

[Matthew Gaudet (he/him) [:mgaudet]]

For some reason bugmon is trying an old version of central?

Comment 8

[Christian Holler (:decoder)]

Yes, it's because linux opt builds were removed and we are ony left with shippable builds on mozilla-central. We are working on it.

Comment 9

[Jason Kratzer [:jkratzer]]

We should have a fix in place later today. If you want to close this sooner, you can go ahead and remove the bugmon keyword to prevent it from being scanned again.

Comment 10

[Nicolas B. Pierron [:nbp]]

We should mark this bug as a duplicate of Bug 1653567 based on comment 3.