1316067 - Savedstacks should do something useful with the async stack even if there are no activations

Status: NEW

(Core :: JavaScript Engine, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

I ran into this in a situation in which a dead object proxy is being used as an event listener.  Here's a simple way to reproduce:

1)  Make sure the devtools.chrome.enabled preference is true.
2)  Open a non-e10s window.
3)  Load about:blank in that window.
4)  Open the scratchpad (Tools > Web Developer > Scratchpad)
5)  Switch scratchpad to browser mode (Environment > Browser)
6)  Evaluate this in the scratchpad:

  window.addEventListener("click", new content.Object(), true)

7)  In the browser, load about:memory instead of the about:blank.
8)  Minimize memory usage.
9)  Click in that about:memory page a bit.

Every click, you will get "TypeError: can't access dead object" in the console that have no useful location information or stack.

What happens here is that our CallbackObject code tries to do a JS_GetProperty on the dead object, which throws.  At this point there is no JS on the stack, so no activation, so no useful location information.  There's not that much we can do with that.  But the CallbackObject _does_ set up an asyncStackForNewActivations on the JSContext, and we _could_ show that as an async stack for the error, at least.  Right now we don't, because all the async stack code in savedstacks is conditioned on having activations.

Ideally we would communicate something about the async stack here even though there is no current stack.

Note that once this is fixed, we should look at the "TypeError: Property 'handleEvent' is not callable." bit from steps 7/8 above and see whether that's fixed too, or whether it needs additional work; my local hack to just toss the cx->asyncStackForNewActivations into "frame" in SavedStacks::insertFrames when the iter has no activations is working for the dead object exception, but not that one...

Nick, I know you're not actively working on saved stack stuff right now; do you know who might be able to pick this up?

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Let me make sure I have this straight:

* We install `new content.Object()` as a cross-compartment listener
* Then the listener becomes dead after the "minimize memory usage"
* Then we trigger the event the listener was bound to
  * Which throws an error when we try to access the dead object
  * But this is *before* we have any JS activation for this event
  * So we end up without a useful stack to report *nor* taking advantage of our `asyncStackForNewActivations`

Is that correct?

I suppose we could try and eagerly grab an async stack in `js::SavedFrame::insertFrames`, although I'd have to dig deeper to really assess that.

Maybe Tromey or Jim have a few cycles?

Comment 2

[Boris Zbarsky [:bzbarsky]]

> Is that correct?

Yes, that is correct.  In particular, the exception is thrown _after_ the DOM code has set up the asyncStackForNewActivations, but before any actual activations have been created.  So we do have a useful asyncStackForNewActivations here.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: What I was experimenting with

This doesn't make the async nature of the stack clear, of course....

Comment 4

[Jim Blandy :jimb]

I will not have time to work on this.

Comment 5

[Julien Cristau [:jcristau]]

Too late for firefox 52, mass-wontfix.

Comment 6

[Tom Tromey :tromey]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #1)

> Maybe Tromey or Jim have a few cycles?

Past time to admit I don't.

One idea for the patch would be to push some sort of dummy frame, so that the
rest of the trace is more obviously async.

Comment 7

[Tom Tromey :tromey]

(In reply to Tom Tromey :tromey from comment #6)

> One idea for the patch would be to push some sort of dummy frame, so that the
> rest of the trace is more obviously async.

This would also help with bug 1206403.