1608891 - Assertion failure: observing, at debugger/Debugger.cpp:3182

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20200107-e728bf01a2b6 (build with --enable-debug, run with --fuzzing-safe --ion-offthread-compile=off --baseline-eager):

var g71 = newGlobal({
    newCompartment: true
});
var dbg = Debugger(g71);
var num = 20;
function loop(i39) {}
g71.eval(loop.toString());
dbg.onEnterFrame = function(frame) {};
dbg.collectCoverageInfo = true;
g71.eval("loop(" + num + ");");
dbg.collectCoverageInfo = false;

Backtrace:

received signal SIGSEGV, Segmentation fault.
0x0000555555fb6764 in UpdateExecutionObservabilityOfScriptsInZone(JSContext*, JS::Zone*, js::DebugAPI::ExecutionObservableSet const&, js::DebugAPI::IsObserving) ()
#0  0x0000555555fb6764 in UpdateExecutionObservabilityOfScriptsInZone(JSContext*, JS::Zone*, js::DebugAPI::ExecutionObservableSet const&, js::DebugAPI::IsObserving) ()
#1  0x0000555555fb5931 in js::Debugger::updateExecutionObservabilityOfScripts(JSContext*, js::DebugAPI::ExecutionObservableSet const&, js::DebugAPI::IsObserving) ()
#2  0x0000555555fb7a19 in js::Debugger::updateObservesCoverageOnDebuggees(JSContext*, js::DebugAPI::IsObserving) ()
#3  0x0000555555fbdb24 in js::Debugger::CallData::setCollectCoverageInfo() ()
#4  0x0000555555fcc9de in bool js::Debugger::CallData::ToNative<&js::Debugger::CallData::setCollectCoverageInfo>(JSContext*, unsigned int, JS::Value*) ()
#5  0x00005555558f07b2 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) ()
#6  0x00005555558f00c3 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) ()
#7  0x00005555558f2772 in js::CallSetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<JS::Value>) ()
#8  0x0000555555cc91aa in SetExistingProperty(JSContext*, JS::Handle<JS::PropertyKey>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<js::NativeObject*>, JS::Handle<JS::PropertyResult>, JS::ObjectOpResult&) ()
#9  0x0000555555cc8326 in bool js::NativeSetProperty<(js::QualifiedBool)1>(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JS::PropertyKey>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&) ()
#10 0x00005555563afd69 in js::jit::DoSetPropFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICSetProp_Fallback*, JS::Value*, JS::Handle<JS::Value>, JS::Handle<JS::Value>) ()
#11 0x00000a8c873c1205 in ?? ()
[...]
#26 0x0000000000000000 in ?? ()
rax	0x555556f2185b	93825019287643
rbx	0x2b8183ca8060	47835261861984
rcx	0x555557f1d838	93825036048440
rdx	0x0	0
rsi	0x7ffff6efd770	140737336301424
rdi	0x7ffff6efc540	140737336296768
rbp	0x7fffffffad80	140737488334208
rsp	0x7fffffffa8f0	140737488333040
r8	0x7ffff6efd770	140737336301424
r9	0x7ffff7f98d00	140737353714944
r10	0x58	88
r11	0x7ffff6ba47a0	140737332791200
r12	0x2b8183cbf088	47835261956232
r13	0x7ffff5e27000	140737318645760
r14	0x7ffff4cc7000	140737300426752
r15	0x0	0
rip	0x555555fb6764 <UpdateExecutionObservabilityOfScriptsInZone(JSContext*, JS::Zone*, js::DebugAPI::ExecutionObservableSet const&, js::DebugAPI::IsObserving)+3428>
=> 0x555555fb6764 <_ZL43UpdateExecutionObservabilityOfScriptsInZoneP9JSContextPN2JS4ZoneERKN2js8DebugAPI22ExecutionObservableSetENS5_11IsObservingE+3428>:	movl   $0xc6e,0x0
   0x555555fb676f <_ZL43UpdateExecutionObservabilityOfScriptsInZoneP9JSContextPN2JS4ZoneERKN2js8DebugAPI22ExecutionObservableSetENS5_11IsObservingE+3439>:	callq  0x5555557f7fc2 <abort>

Comment 1

[Jason Orendorff [:jorendorff]]

Once we get a regression range, this will be high priority.

Comment 2

[Nicolas B. Pierron [:nbp]]

Ted,

This is an old issue that I saw before, I was wondering whether we should not disable the recompilation based on the debugger flag and identically prefer the runtime flag over a dynamic flag. In which case, the debugger would also require the shell flag.

(debugger only + code coverage + fuzz-bug => P2)

Comment 3

[Sean Voisen (:svoisen)]

Hi Jason — Will the bot be bisecting this soon?

Comment 4

[Ted Campbell [:tcampbell]]

The execution-observer system is used by both the onEnterFrame hook and the collectCoverageInfo system and we end up toggling it incorrectly. This issue reproduces in FF72, but I haven't done a proper bisection.

I'll see if I can clean up the observer definition to either disallow using both onEnterFrame / collectCoverageInfo at same time, or ensuring we can't clear execution-observer flag while one of them is still using it.

Comment 5

[Fuzzing Team]

JSBugMon: Cannot process bug: Error: Failed to isolate original revision for test

Comment 8

[Ted Campbell [:tcampbell]]

This apparently is an old issue and a matter of the execution-observer flag being used for too many things. I'd like to restrict how flexible our usage of coverage is since in practice it has been a source of complexity in the vm.

Comment 9

[Christian Holler (:decoder)]

Looks like we don't need a bisect here as the bug is already assigned.

Comment 11

[Ted Campbell [:tcampbell]]

This is more of an old design issue than a regression.
Matt, do you want to retriage if we should do anything here?

Comment 12

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

Hi Nicolas,

We've had a conflict in how code-coverage and onEnterFrame observation happens for a long while. My preference would be to make these two states exclusive (making the onEnterFrame and collectCoverageInfo setters throw if the other is already set). Do you forsee any issues with doing that?

Comment 13

[Nicolas B. Pierron [:nbp]]

As far as I know the only consumer of the code coverage info, outside branch pruning, is the code-profiling instrumentation to provide profiling information on the JS code of Firefox while running the test suite. Last time I heard they were using the Debugger API in order to isolate the coverage of each test while firefox is running multiple tests.

I do not know how onEnterFrame and collectCoverageInfo behave when requested by different Debuggers. So this bug might have an impact of the Debugger test suite, or the code coverage of it.

Comment 14

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

Ah interesting. We'll see what try has to say. :)

Comment 15

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

Attachment: Bug 1608891 - Disallow onEnterFrame and collectCoverageInfo from being active at the same time r?jandem

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1803988f6373
Disallow onEnterFrame and collectCoverageInfo from being active at the same time r=jandem

Comment 17

[Nicolas Chevobbe [:nchevobbe]]

Not really my area of expertise here, maybe Alex has a better grasp on this

Comment 18

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/1803988f6373

Comment 19

[Alexandre Poirot [:ochameau]]

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #12)

We've had a conflict in how code-coverage and onEnterFrame observation happens for a long while. My preference would be to make these two states exclusive (making the onEnterFrame and collectCoverageInfo setters throw if the other is already set). Do you forsee any issues with doing that?

Sounds good to me, DevTools isn't using collectCoverageInfo.

I imagine you could still spawn two distinct Debuggers:

• one debugging js and involving onEnterFrame (i.e. DevTools),
• another one computing coverage (i.e. CodeCoverageUtils)
  And both would run fine independently of each others?
  If they don't, that's not a big deal, it would only mean we can't use DevTools when CodeCoverageUtils kicks in, which is probably rare. Only done on-demand and may be restricted to some globals only.