976348 - Change DescribeScriptedCaller to not return a JSScript

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Attachment: change-describe-scripted-caller

Callers of DescribeScriptedCaller don't really want the JSScript; they usually want the filename.  It'd be nice to avoid returning the JSScript since (1) it's kinda an implementation detail, (2) I'd like to allow functions on the stack that don't have a JSScript (asm.js).

This patch changes DescribeScriptedCaller to return a little stack guard class that holds the associated ScriptSource (which owns the filename) alive via refcount.  The only other non-js-shell use of DescribeScriptedCaller wants the global of the scripted caller so this patch also adds GetScriptedCallerGlobal().

Comment 1

[Bobby Holley (:bholley)]

Comment on attachment 8380985 [details] [diff] [review]
change-describe-scripted-caller

Review of attachment 8380985 [details] [diff] [review]:
-----------------------------------------------------------------

r=bholley with comments addressed.

::: js/src/jsapi-tests/testDebugger.cpp
@@ +257,2 @@
>  
> +        NonBuiltinScriptFrameIter iter(cx);

We should probably assert !iter.done() here, right?

::: js/src/jsapi.cpp
@@ +6164,5 @@
> +    // that it can check its own stack (see HideScriptedCaller).
> +    if (i.activation()->scriptedCallerIsHidden())
> +        return nullptr;
> +
> +    return i.activation()->compartment()->maybeGlobal();

This should always be non-null, right? Can we assert that?

::: js/xpconnect/src/XPCShellImpl.cpp
@@ -124,5 @@
> -    JS::RootedScript script(cx);
> -    JS_DescribeScriptedCaller(cx, &script, nullptr);
> -    const char *filename = JS_GetScriptFilename(cx, script);
> -
> -    if (filename) {

Don't we still need the null check on filename (or filename.get(), in the new world)?

Comment 2

[Luke Wagner [:luke]]

(In reply to Bobby Holley (:bholley) from comment #1)
> We should probably assert !iter.done() here, right?

We could, but the first line of ScriptFrameIter::script() does that.

> This should always be non-null, right? Can we assert that?

I *think* so; the comment says that the only two null cases are: (1) atoms compartment (which shouldn't push an Activation) and (2) a live compartment which only contains live strings (which again shouldn't push an Activation).  I'll add the assert.

> Don't we still need the null check on filename (or filename.get(), in the
> new world)?

Technically, I think the embedding can pass a null filename when compiling and the pre-existing null checks made me worried that that was happening in some cases.  Some places in Gecko appear to forego the check but I didn't want to change this assumption in the same patch (a different patch that asserted this property would be good).

Comment 3

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] from comment #2)
> Technically, I think the embedding can pass a null filename when compiling
> and the pre-existing null checks made me worried that that was happening in
> some cases.  Some places in Gecko appear to forego the check but I didn't
> want to change this assumption in the same patch (a different patch that
> asserted this property would be good).

Yes, but this patch is removing a null check.

Comment 4

[Luke Wagner [:luke]]

(In reply to Bobby Holley (:bholley) from comment #3)
> Yes, but this patch is removing a null check.

How so?

Comment 5

[Luke Wagner [:luke]]

Oops, n/m, I read "don't" as "do" in comment 1; you're right.

Comment 6

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9dcd0eff1b23

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9dcd0eff1b23