1824123 - Opening inspector in a debug build of thunderbird consistently hits MOZ_ASSERT(obj->compartment() == this);

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

On a Thunderbird debug build, press Ctrl+Shift+I to open the browser toolbox. I consistently hit:

#0  0x00007f166b8c5db5 in __GI___clock_nanosleep (clock_id=clock_id@entry=0, flags=flags@entry=0, req=req@entry=0x7ffec5a5b880, rem=rem@entry=0x7ffec5a5b880) at ../sysdeps/unix/sysv/linux/clock_nanosleep.c:48
#1  0x00007f166b8ca987 in __GI___nanosleep (req=req@entry=0x7ffec5a5b880, rem=rem@entry=0x7ffec5a5b880) at ../sysdeps/unix/sysv/linux/nanosleep.c:25
#2  0x00007f166b8ca8be in __sleep (seconds=0) at ../sysdeps/posix/sleep.c:55
#3  0x00007f166341dbf5 in common_crap_handler(int, void const*) (signum=11, aFirstFramePC=0x7f16633fe4ca <nsProfileLock::FatalSignalHandler(int, siginfo_t*, void*)+290>) at /home/emilio/src/moz/tb/toolkit/xre/nsSigHandlers.cpp:96
#4  0x00007f166341dcd5 in ah_crap_handler(int) (signum=0) at /home/emilio/src/moz/tb/toolkit/xre/nsSigHandlers.cpp:104
#5  0x00007f16633fe4ca in nsProfileLock::FatalSignalHandler(int, siginfo_t*, void*) (signo=11, info=0x7ffec5a5bb70, context=0x7ffec5a5ba40) at /home/emilio/src/moz/tb/toolkit/profile/nsProfileLock.cpp:183
#6  0x00007f16649ca658 in WasmTrapHandler(int, siginfo_t*, void*) (signum=11, info=0x7ffec5a5bb70, context=0x7ffec5a5ba40) at /home/emilio/src/moz/tb/js/src/wasm/WasmSignalHandlers.cpp:783
#7  0x00007f166b831f50 in <signal handler called> () at /usr/lib/libc.so.6
#8  JS::Compartment::objectMaybeInIteration(JSObject*) (this=<optimized out>, obj=<optimized out>) at /home/emilio/src/moz/tb/js/src/vm/Compartment-inl.h:424
#9  js::SuppressDeletedProperty(JSContext*, JS::Handle<JSObject*>, JS::PropertyKey) (cx=<optimized out>, obj=(JSObject * const) 0x3e3fb242acb8 [object Proxy], id=Python Exception <class 'gdb.error'>: value has been optimized out
) at /home/emilio/src/moz/tb/js/src/vm/Iteration.cpp:1837
#10 0x00007f1663e54413 in js::DeleteProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::ObjectOpResult&)
    (cx=0x7f1655830100, obj=(JSObject * const) 0x3e3fb242acb8 [object Proxy], id=$jsid("debounce"), result=...) at /home/emilio/src/moz/tb/js/src/vm/ObjectOperations-inl.h:370
#11 js::ForwardingProxyHandler::delete_(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::ObjectOpResult&) const (this=<optimized out>, cx=<optimized out>, proxy=Python Exception <class 'gdb.error'>: value has been optimized out
, id=$jsid("debounce"), result=...)
    at /home/emilio/src/moz/tb/js/src/proxy/Wrapper.cpp:81
#12 0x00007f1663e44d9a in js::Proxy::delete_(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::ObjectOpResult&)
    (cx=0x7f1655830100, proxy=(JSObject * const) 0x3e3fb242ad60 [object Proxy], id=$jsid("debounce"), result=...) at /home/emilio/src/moz/tb/js/src/proxy/Proxy.cpp:263
#13 0x00007f1663e491cb in proxy_DeleteProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::ObjectOpResult&)
    (cx=0x7f166b9d3840 <_IO_stdfile_2_lock>, obj=<error reading variable: Cannot access memory at address 0x0>, id=<error reading variable: Cannot access memory at address 0x1>, result=...)
    at /home/emilio/src/moz/tb/js/src/proxy/Proxy.cpp:829
#14 0x00007f16638b4f83 in js::DeleteProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::ObjectOpResult&)
    (cx=0x7f1655830100, obj=(JSObject * const) 0x3e3fb242ad60 [object Proxy], id=$jsid("debounce"), result=...) at /home/emilio/src/moz/tb/js/src/vm/ObjectOperations-inl.h:370
#15 js::DelElemOperation<true>(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, bool*) (cx=<optimized out>, val=Python Exception <class 'gdb.error'>: value has been optimized out
, index=Python Exception <class 'gdb.error'>: value has been optimized out
, res=0x7ffec5a5c228) at /home/emilio/src/moz/tb/js/src/vm/Interpreter.cpp:4833

Which is this assert: https://searchfox.org/mozilla-central/rev/4d6a262deed98ddb4d609de94048e7f97b6e85d3/js/src/vm/Compartment-inl.h#424

DumpJSStack() says:

123770> 0 get() ["resource://devtools/shared/loader/builtin-modules.js":38:6]
123770>     this = [object Window]
123770> 1 Inspector(toolbox = [cross-compartment wrapper], commands = [cross-compartment wrapper]) ["resource://devtools/client/inspector/inspector.js":174:2]
123770>     this = [object Object]
123770> 2 InspectorPanel(iframeWindow = [cross-compartment wrapper], toolbox = "[object Object]", commands = "[object Object]") ["resource://devtools/client/inspector/panel.js":8:20]
123770>     this = [object Object]
123770> 3 build(iframeWindow = [cross-compartment wrapper], toolbox = "[object Object]", commands = "[object Object]") ["resource://devtools/client/definitions.js":172:11]
123770>     this = [object Object]
123770> 4 onLoad() ["resource://devtools/client/framework/toolbox.js":2587:31]
123770> 

Comment 1

[Iain Ireland [:iain]]

Huh, weird. We have a testcase that covers deleting properties from cross-compartment wrappers. Poking at that testcase in the debugger, it looks like it avoids this issue because CrossCompartmentWrapper::delete_ uses PIERCE, which contains an AutoRealm to enter the realm of the wrapped object. I'm not sure why your proxy isn't doing the same thing. Maybe it's not actually a cross-compartment wrapper? The DumpJSStack output sure makes it look like CCWs are involved.

Is there any way you could get more information about what kind of proxy this is / where it comes from?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Sure, I can try to get more info tomorrow.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

I got a crash in nsOuterWindowProxy::delete_ with the proxy being:

object 2b31ea6fd200
  global 2b31ea6a3190 [Window]
  class 7fe336d988e0 Proxy
    handler 7fe336d98a80
    private <Window object at 2b31ea6a3190>
  shape 33d14a37ae80
  flags: maybe_has_interesting_symbol not_native
  proto <dynamic>

Does that help? I uploaded a pernosco recording, hopefully it comes back usable and I can link it here.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

https://pernos.co/debug/i5LWkEJ3XXT58f3HMiGWTA/index.html

Comment 6

[Iain Ireland [:iain]]

I have poked at this in Pernosco a bit. This is my first time using Pernosco, so I'm still learning the ropes.

It's a bit tricky to tell because of inlining/etc in an optimized build, but I'm pretty sure the sequence of events is:

We have a cross-compartment wrapper around this function. We call it, which enters its realm. We execute delete object[name]. object is a proxy (presumably a CCW) and lives in the same realm as get. Its target is an nsOuterWindowProxy, which lives in a different realm. The target of that proxy is a dom::WindowBinding.

DeleteProperty sees that the CCW has a delete hook and calls proxy_DeleteProperty, which in turn calls Proxy::delete_. This dispatches to ForwardingProxyHandler::delete_, which unwraps the target and calls DeleteProperty on that. We're still in the same realm as get, but now obj is an outer window proxy from another realm.

This call to DeleteProperty sees that obj also has a delete hook, so we call proxy_DeleteProperty again. This does two things. First, it calls into OuterWindowProxy::delete_, which (after some steps that don't matter) eventually ends up finding a real object and deleting the property. Second, it calls SuppressDeletedProperty on the outer window proxy. But we're not in the realm of the outer window proxy, so we trigger this assertion.

And now looking at the history of that assertion more closely, which I suppose I could have done before spending all this time messing around in pernosco, I see that when I moved the enumerators list from Realm to Compartment, I replaced code that used the object's realm with code that uses the current compartment. So it looks like this is probably my fault, and the fix is to grab the compartment from the object instead of the context.

If I put up a patch can you test it out?

Comment 7

[Iain Ireland [:iain]]

Attachment: Bug 1824123: Use object's compartment instead of context's when suppressing deleted properties

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Yep, that patch fixes the issue for me!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c0f1b8f54b01
Use object's compartment instead of context's when suppressing deleted properties r=jandem

Comment 10

[Noemi Erli[:noemi_erli]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Should we try to land a test for this?

Comment 12

[Iain Ireland [:iain]]

We should. Unfortunately, I don't think we can test this in the shell.

In my description above, I say "object is a proxy (presumably a CCW) and lives in the same realm as get." In practice, though, if I try writing this testcase with a CCW, we enter the right realm and this issue doesn't occur. A little more Pernosco digging shows that the proxy is actually a CrossOriginObjectWrapper, which deliberately avoids the realm-entering behaviour, delegating it to the target object (which is always a proxy itself).

Maybe I can add something here?