1139775 - SavedFrame.prototype.parent need not skip non-subsumed frames eagerly

Status: NEW

(Core :: JavaScript Engine, defect, P3)

Description

[Jim Blandy :jimb]

JS::GetSavedFrameParent (and hence SavedFrame.prototype.parent) should simply return frame->getParent(), without calling GetFirstSubsumedFrame on that.

We always skip non-subsumed frames before accessing any frame
properties, so there's no risk that we would reveal information we
shouldn't. Handing out SavedFrame objects to code that shouldn't see
the data they captured is the normal case.

But by not calling GetFirstSubsumedFrame, we would lose less
information : if low code extracts a frame's parent, and hands that to
high code, the high code will see a more useful stack.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Consider this stack:

  a -> B -> C

where "a" is content and B/C are chrome and "->" represents "is called from".

Now say you have something like console.trace walking this stack.  Right now it has the frame for "a", gets its info, goes to the parent, gets null, and is done.

If the parent returned "B", then it would try to get _its_ info and get all "" and 0 and whatnot, right?  I actually ran into this exact problem with my initial patches for bug 1137910 where I had put the AutoMaybeEnterFrameCompartment inside UnwrapSavedFrame and hence ended up with GetSavedFrameParent returning a frame we were not supposed to subsume (because we weren't in the right compartment during it GetFirstSubsumedFrame call).

All of which is to say that if we make this change, we need to make sure that we figure out a better way to reflect things via nsIStackFrame than we have now, because as things stand anyone walking an nsIStackFrame stack in the situation above would end up with a bogus "empty" frame at the top, right?

Comment 2

[:Paolo Amadini]

(In reply to Not doing reviews right now from comment #1)
> If the parent returned "B"

In fact, I think we should still return "null" in this case, as if the parent frames didn't exist at all. This means we should still call GetFirstSubsumedFrame internally. This is what bug 1083359 does.

> anyone walking an nsIStackFrame stack in
> the situation above would end up with a bogus "empty" frame at the top,
> right?

Well, anyone walking nsIStackFrame does get a bogus stack frame at the top now:

http://mxr.mozilla.org/mozilla-central/source/dom/bindings/Exceptions.cpp#602

I believe they shouldn't see it...

Comment 3

[Boris Zbarsky [:bzbarsky]]

> In fact, I think we should still return "null" in this case

That's not what comment 0 is proposing, yes?

> Well, anyone walking nsIStackFrame does get a bogus stack frame at the top now:

Well, sort of.  Except it's not a JS stackframe and consumers know to filter it out.  Plus I've been wanting to remove it, as you can see from that comment.