1120390 - Assertion failure: !k->compartment()->options_.invisibleToDebugger() - crash when closing a Hello window with the Browser Console open

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mark Banner (:standard8)]

I updated to today's fx-team and now I'm getting assertion failures.

STR:

1) Build debug fx-team (I'm on a868809113e1)
2) Start the build, open the browser debugger (note, I'm in non-e10s mode)
3) Open the Hello panel, open (or start a conversation).
4) Once the conversation has opened, close the window.

Expected Results:

-> Closes fine, no issues

Actual Results:

Assertion failure: !k->compartment()->options_.invisibleToDebugger(), at /Users/mark/loop/gecko-dev/js/src/vm/Debugger.h:100

It doesn't happen with the browser console closed.

Stack:

0   XUL                           	0x000000010511b88a bool js::DebuggerWeakMap<JSObject*, false>::relookupOrAdd<JS::Rooted<JSObject*>, js::NativeObject*>(js::detail::HashTable<js::HashMapEntry<js::PreBarriered<JSObject*>, js::RelocatablePtr<JSObject*> >, js::HashMap<js::PreBarriered<JSObject*>, js::RelocatablePtr<JSObject*>, js::DefaultHasher<js::PreBarriered<JSObject*> >, js::RuntimeAllocPolicy>::MapHashPolicy, js::RuntimeAllocPolicy>::AddPtr&, JS::Rooted<JSObject*> const&, js::NativeObject* const&) + 618 (Debugger.h:100)
1   XUL                           	0x00000001050db7b7 js::Debugger::wrapDebuggeeValue(JSContext*, JS::MutableHandle<JS::Value>) + 1415 (Debugger.cpp:841)
2   XUL                           	0x00000001050e89ce js::Debugger::makeGlobalObjectReference(JSContext*, unsigned int, JS::Value*) + 190 (Debugger.cpp:3772)
3   XUL                           	0x000000010513303c js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) + 636 (jscntxtinlines.h:228)
4   XUL                           	0x0000000105158e30 js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) + 752 (Interpreter.cpp:554)
5   XUL                           	0x00000001050b4c0e js::DirectProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const + 190 (DirectProxyHandler.cpp:79)
6   XUL                           	0x00000001050b485d js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const + 477 (CrossCompartmentWrapper.cpp:286)
7   XUL                           	0x00000001050bc698 js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) + 248 (Proxy.cpp:401)
8   XUL                           	0x00000001050be9c0 js::proxy_Call(JSContext*, unsigned int, JS::Value*) + 96 (Proxy.cpp:792)
9   XUL                           	0x000000010513323c js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) + 1148 (jscntxtinlines.h:228)
10  XUL                           	0x0000000105158e30 js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) + 752 (Interpreter.cpp:554)
11  XUL                           	0x0000000104c6000c js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) + 2252 (BaselineIC.cpp:9496)

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Jim, maybe you know what's wrong here?

Comment 2

[Jim Blandy :jimb]

Beautiful. Here's an easier way to reproduce the crash in the JavaScript shell:

$ cat ~/moz/not-so-invisible.js
var g = newGlobal({ invisibleToDebugger: true });
var dbg = new Debugger;

dbg.makeGlobalObjectReference(g);
$ ~/moz/dbg/obj-bug/dist/bin/js -f ~/moz/not-so-invisible.js
Assertion failure: !k->compartment()->options_.invisibleToDebugger(), at /home/jimb/moz/dbg/js/src/vm/Debugger.h:100
Segmentation fault (core dumped)
$ 

All calls to UnwrapOneUnchecked and UncheckedUnwrap in js/src/vm/Debugger.cpp should be checking that the newly unwrapped object is not in an invisibleToDebugger compartment.

Comment 3

[Jim Blandy :jimb]

But that's just to get an error thrown, instead of a crash. There's still a bug in the browser console, in that it's trying to use Debugger to inspect stuff that's invisible to it.

Comment 4

[Mark Banner (:standard8)]

I also saw a similar failure yesterday when I was using the browser toolbox with the inspector to inspect the browser chrome.

Comment 5

[Jim Blandy :jimb]

(In reply to Mark Banner (:standard8) from comment #4)
> I also saw a similar failure yesterday when I was using the browser toolbox
> with the inspector to inspect the browser chrome.

I'm pretty sure my reproduction recipe in comment 2 is correct, so no need for more details, as long as the assertion message is the same, and the devtools are involved.

Comment 6

[Jim Blandy :jimb]

Standard8: Sorry I missed you on IRC. I'm working on this. Should be quick.

Comment 7

[Jim Blandy :jimb]

Attachment: Make Debugger decline invisible-to-Debugger globals as debuggees and as Debugger.Object referents.

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=546916c83e1d

Comment 8

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8553381 [details] [diff] [review]
Make Debugger decline invisible-to-Debugger globals as debuggees and as Debugger.Object referents.

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

Do you know if we need the public domain boilerplate at the top of test scripts?

::: js/src/vm/Debugger.cpp
@@ +3790,5 @@
>          return false;
>  
> +    // If we create a global for an invisible compartment, then from it we can
> +    // reach function objects, scripts, environments, etc. etc. none of which
> +    // we're ever supposed to see.

Did you you intend to to duplicate etc. etc.?

Comment 9

[Jim Blandy :jimb]

(In reply to Steve Fink [:sfink, :s:] from comment #8)
> Do you know if we need the public domain boilerplate at the top of test
> scripts?

Out of all the files in js/src/jit-tests, only 75 have that boilerplate. I don't know if it's necessary...

> Did you you intend to to duplicate etc. etc.?

Yes, but maybe it's too affected.

Comment 10

[Jim Blandy :jimb]

Try push looked okay. Changed wording.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ba8480c8608

Comment 11

[Steve Fink [:sfink] [:s:]]

(In reply to Jim Blandy :jimb from comment #9)
> (In reply to Steve Fink [:sfink, :s:] from comment #8)
> > Do you know if we need the public domain boilerplate at the top of test
> > scripts?
> 
> Out of all the files in js/src/jit-tests, only 75 have that boilerplate. I
> don't know if it's necessary...

Not worth worrying about for this bug, then.

Comment 12

[Jim Blandy :jimb]

For what it's worth, here's the relevant page:
https://www.mozilla.org/MPL/license-policy.html

"PD Test Code is Test Code which is Mozilla Code, which does not carry an explicit license header, and which was either committed to the Mozilla repository on or after 23rd September 2014, or was committed before that date but all contributors up to that date were Mozilla employees, contractors or interns. PD Test Code is made available under the Creative Commons Public Domain Dedication. Test Code which has not been demonstrated to be PD Test Code should be considered to be under the MPL 2."

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4ba8480c8608