Change DescribeScriptedCaller to not return a JSScript

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla30
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Assignee

Description

5 years ago
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().
Attachment #8380985 - Flags: review?(bobbyholley)
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)?
Attachment #8380985 - Flags: review?(bobbyholley) → review+
Assignee

Comment 2

5 years ago
(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).
(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.
Assignee

Comment 4

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

How so?
Assignee

Comment 5

5 years ago
Oops, n/m, I read "don't" as "do" in comment 1; you're right.
https://hg.mozilla.org/mozilla-central/rev/9dcd0eff1b23
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.