979926 - Sort out the distinction between the saved frame chain and scripted caller override abstractions

Status: NEW

(Core :: DOM: Core & HTML, defect)

Description

[Bobby Holley (:bholley)]

See bug 960820 comment 14.

I think we should do 2 things here:

(1) replace all of the uses of !! uses of DescribeScriptedCaller with JS_IsRunning calls. We can then try to incrementally remove them, if we decide that's a dead concept.

(2) Bundle line and filename information into the Script Settings stack, and track that where it makes sense. Then, we can remove all of the calls to DescribeScriptedCaller throughout Gecko, and replace them with calls into the ScriptSettingsStack machinery.

Comment 1

[Luke Wagner [:luke]]

From bug 960820 comment 15:

I think we should remove JS_IsRunning() since it's meaning is ambiguous.  (Does entering a compartment mean I'm running?  Calling a getter?  Calling a native?  Calling a native getter?  Performing a GC?)

Rather, it'd be nice to sort out: what are the hard spec-defined operations we need to support and where do we want this not-particularly-well-defined "best-effort, respecting security" stack walk.

Comment 2

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] from comment #1)
> From bug 960820 comment 15:
> 
> I think we should remove JS_IsRunning() since it's meaning is ambiguous. 
> (Does entering a compartment mean I'm running?  Calling a getter?  Calling a
> native?  Calling a native getter?  Performing a GC?)

It has a meaning in the engine at present - see bug 960820 comment 16. I'm totally fine with removing it if we can get there.

> Rather, it'd be nice to sort out: what are the hard spec-defined operations
> we need to support and where do we want this not-particularly-well-defined
> "best-effort, respecting security" stack walk.

I think that we should focus on supporting the concept of the incumbent, and attaching extra file/line information to those incumbent entries when we can.

Comment 3

[Luke Wagner [:luke]]

(In reply to Bobby Holley (:bholley) from comment #2)
> It has a meaning in the engine at present - see bug 960820 comment 16. I'm
> totally fine with removing it if we can get there.

Well, I know it's used, but those are more of "creepy and non-obvious effects" that we'd like to replace with logical effects.  On the subject of error reporting, as discussed on irc, we just need to change change to more sane APIs.

> I think that we should focus on supporting the concept of the incumbent, and
> attaching extra file/line information to those incumbent entries when we can.

Why would the embedding need to track this?  Is it not from some frame on the stack?

Comment 4

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] from comment #3)
> > I think that we should focus on supporting the concept of the incumbent, and
> > attaching extra file/line information to those incumbent entries when we can.
> 
> Why would the embedding need to track this?  Is it not from some frame on
> the stack?

In the override case, no. That's why we had to put in AutoHideScriptedCaller - there are cases where the embedding wants to effectively push a virtual frame onto the stack. At the moment, the main place this happens is when firing callbacks, where we restore the incumbent that was active when the callback was created.

Comment 5

[Luke Wagner [:luke]]

Ah, interesting, that makes sense.

Comment 6

[Bobby Holley (:bholley)]

I've been thinking about this a bit. I think we _could_ track file/line on the script settings stack, but I don't think think the consumers we have of that information justify the performance overhead of tracking that information over every CallbackObject call.

So my revised proposal here is the following:

(1) Fix up the error reporting situation so that it doesn't depend on JS_IsRunning and !!DescribeScriptedCaller.
(2) Rename GetScriptedCallerGlobal to IncumbentGlobalOrNull, and rename scriptedCallerIsHidden to hasIncumbentGlobalOverride() etc. These APIs will be governed by the semantics of incumbent globals.
(3) Make DescribeScriptedCaller ignore the overrides, and turn it into a simple API that says "what is the most recent non-builtin scripted stack frame that I'm allowed to see"? This seems to be what most consumers want.

Comment 7

[Luke Wagner [:luke]]

\o/

Comment 8

[Bobby Holley (:bholley)]

I filed bug 981187 and bug 981209 for (1). The rest of comment 6 can constitute the patches that will be written for this bug.

Comment 9

[Boris Zbarsky [:bzbarsky]]

OK, so the current state of things is:

1)  We're going to kill JS_IsRunning completely.
2)  We're going to kill saved frame chains completely.
3)  Item 1 from comment 6 is done.
4)  Item 3 from comment 6 should imo _not_ be done, or if it's done
    we need to make it an option or something.  Some callers of
    DescribeScriptedCaller really do want to pay attention to the
    override.  See discussion in bug 1274915.

Comment 10

[Jan de Mooij [:jandem]]

bz, anything left here?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Maybe comment 6 item 2?