800631 - Remove JSContext arg from JSDebugErrorHook

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

The problem here is that this is called by js_ReportOutOfMemoryError.  If we allow this hook to trigger GC, then effectively cx/rt->malloc can trigger GC.  This would be bad, since we currently assume they do not GC.

Currently this hook is unused, so there is no current GC hazard, but we should prevent it while doing so is still easy.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

We explicitly forbid GCs during that hook to make it impossible for GCs to happen during malloc. There's a flag called rt->inOOMReport for this purpose.

Comment 2

[Terrence Cole [:terrence]]

Attachment: v0

Looks like we'll be able to remove the hook entirely once JSD goes away.  For now, we can guard with our fancy new assertions that this property holds.

Comment 3

[Terrence Cole [:terrence]]

Also meant to say that we can't drop the |cx| because we store the exception info on JSContext and JSD needs to be able to clear it.  Ideally we'd factor out the exception info block of the JSContext into its own structure and pass it around as a pointer, but this is just too much work just for JSD's sake.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 670936 [details] [diff] [review]
v0

I think this will assert. The debug hook is allowed to do things that invoke GC, but the GC will return immediately. However, the assertion will happen before the early return.

If we really want to do this, we would need a way to temporarily cancel the effect of AutoAssertNoGC. Yuck.

Comment 5

[Terrence Cole [:terrence]]

Looks like we just can't do this.