744617 - Remove simple uses of JS_FrameIterator

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Many uses of JS_FrameIterator are really simple so this bug is about removing all of those cases. Actually removing the API is bug 732653.

The three questions that seem to be commonly asked:
  * What's the calling script filename and line number?
  * Is there any scripted code on the stack?
  * What is the current scripted global?

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: remove easy xpconnect uses

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: remove easy dom, caps, content uses

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Sending these to try. There are two more cases that look easy, in AccessCheck and nsGlobalWindow that do some complicated logic that as far as I can tell is equivalent to calling JS_GetGlobalForScopeChain, but I'll verify this tomorrow.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: remove uses from nsGlobalWindow, wrappers/AccessCheck

Two files have an identical loop that appears to reduce to JS_GetGlobalForScopeChain. That removes two more uses of JS_FrameIterator.

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 614253 [details] [diff] [review]
remove easy xpconnect uses

># HG changeset patch
># Parent dfdcc9c6cd43cf7fb70ccd3f6e767c1b6590267e
>Remove simple JS_FrameIterator uses in xpconnect (bug 744617, r=bholley).

>diff --git a/js/xpconnect/src/XPCWrappedJSClass.cpp b/js/xpconnect/src/XPCWrappedJSClass.cpp
>--- a/js/xpconnect/src/XPCWrappedJSClass.cpp
>+++ b/js/xpconnect/src/XPCWrappedJSClass.cpp
>@@ -1032,25 +1032,19 @@ nsXPCWrappedJSClass::CheckForException(X
>                 // that a user pref was set indicating that we should report all
>                 // exceptions.
>                 if (!reportable)
>                     reportable = nsXPConnect::ReportAllJSExceptions();
> 
>                 // Finally, check to see if this is the last JS frame on the
>                 // stack. If so then we always want to report it.
>                 if (!reportable) {
>-                    bool onlyNativeStackFrames = true;
>-                    JSStackFrame * fp = nsnull;
>-                    while ((fp = JS_FrameIterator(cx, &fp))) {
>-                        if (JS_IsScriptFrame(cx, fp)) {
>-                            onlyNativeStackFrames = false;
>-                            break;
>-                        }
>+                    if (JS_DescribeScriptedCaller(cx, nsnull, nsnull)) {
>+                        reportable = false;
>                     }
>-                    reportable = onlyNativeStackFrames;
>                 }

This is wrong. By the time we're in this block, the if() condition guarantees that reportable is false. so what you really want here is to replace this whole block with |reportable = !JS_DescribeScriptedCaller();|

>diff --git a/js/xpconnect/wrappers/AccessCheck.cpp b/js/xpconnect/wrappers/AccessCheck.cpp
>--- a/js/xpconnect/wrappers/AccessCheck.cpp
>+++ b/js/xpconnect/wrappers/AccessCheck.cpp
>@@ -359,42 +359,39 @@ AccessCheck::isSystemOnlyAccessPermitted
>     }
> 
>     JSStackFrame *fp;
>     nsIPrincipal *principal = ssm->GetCxSubjectPrincipalAndFrame(cx, &fp);
>     if (!principal) {
>         return false;
>     }
> 
>+    JSScript *script = nsnull;
>     if (!fp) {
>-        if (!JS_FrameIterator(cx, &fp)) {
>+        if (!JS_DescribeScriptedCaller(cx, &script, nsnull)) {

Are these checks identical? It seems like the newer one is weaker. In particular if we somehow manage to have only a native frame on the stack (maybe this could happen with setTimeout?), this would allow access, whereas the old code wouldn't.

>             // No code at all is running. So we must be arriving here as the result
>             // of C++ code asking us to do something. Allow access.
>             return true;
>         }
>-
>-        // Some code is running, we can't make the assumption, as above, but we
>-        // can't use a native frame, so clear fp.
>-        fp = NULL;
>-    } else if (!JS_IsScriptFrame(cx, fp)) {
>-        fp = NULL;
>+    } else if (JS_IsScriptFrame(cx, fp)) {
>+        script = JS_GetFrameScript(cx, fp);
>     }

In general, I'm not really convinced that this chunk is correct. This code isn't my specialty, though I'll be ripping it out as soon as cpg lands.

Can you roll the AccessCheck.cpp stuff into blake's patch?

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Bobby Holley (:bholley) from comment #5)
> This is wrong. By the time we're in this block, the if() condition
> guarantees that reportable is false. so what you really want here is to
> replace this whole block with |reportable = !JS_DescribeScriptedCaller();|
> 

Okay, thanks.

> >-        if (!JS_FrameIterator(cx, &fp)) {
> >+        if (!JS_DescribeScriptedCaller(cx, &script, nsnull)) {
> 
> Are these checks identical? It seems like the newer one is weaker. In
> particular if we somehow manage to have only a native frame on the stack

What are native frames? The JS engine only has scripted and dummy frames (this loop skipped dummy frames, as DescribeScriptedCaller does - and dummy frames should go away after CPG I think).

> In general, I'm not really convinced that this chunk is correct. This code
> isn't my specialty, though I'll be ripping it out as soon as cpg lands.

It's not correct in that with IonMonkey you could get the wrong script->filename, but as you said CPG will remove it all. Is there anything else?

Comment 7

[Bobby Holley (:bholley)]

(In reply to David Anderson [:dvander] from comment #6)

> It's not correct in that with IonMonkey you could get the wrong
> script->filename, but as you said CPG will remove it all. Is there anything
> else?

I don't fully understand the nature of the fp that's returned from the caps call. Best to let blake review this.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: remove easy xpconnect uses

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: remove uses from nsGlobalWindow, wrappers/AccessCheck

This turned out to be slightly harder, GetGlobalForScopeChain looks at dummy frames where here we want only scripted frames. This patch introduces a new API call - sending to try.

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 614655 [details] [diff] [review]
remove easy xpconnect uses

r=bholley on everything but AccessCheck. That part's for blake.

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 614255 [details] [diff] [review]
remove easy dom, caps, content uses

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

::: dom/base/nsDOMClassInfo.cpp
@@ +1845,4 @@
>    nsAutoString sourcefile;
> +
> +  if (JS_DescribeScriptedCaller(cx, &script, &lineno)) {
> +    if (const char *filename = ::JS_GetScriptFilename(cx, script)) {

Please, no ::

::: dom/base/nsJSUtils.cpp
@@ +72,3 @@
>  
> +  if (!JS_DescribeScriptedCaller(aContext, &script, &lineno)) {
> +    return JS_FALSE;

false, please

@@ +75,3 @@
>    }
>  
> +  *aFilename = ::JS_GetScriptFilename(aContext, script);

No ::

@@ +76,5 @@
>  
> +  *aFilename = ::JS_GetScriptFilename(aContext, script);
> +  *aLineno = lineno;
> +
> +  return JS_TRUE;

true

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 614655 [details] [diff] [review]
remove easy xpconnect uses

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

::: js/xpconnect/wrappers/AccessCheck.cpp
@@ +363,5 @@
>      if (!principal) {
>          return false;
>      }
>  
> +    JSScript *script = nsnull;

NULL, please

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/integration/mozilla-inbound/rev/ecd8434e43d9
http://hg.mozilla.org/integration/mozilla-inbound/rev/546132d79a53
http://hg.mozilla.org/integration/mozilla-inbound/rev/e59ac316362a

Comment 14

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/ecd8434e43d9
https://hg.mozilla.org/mozilla-central/rev/546132d79a53
https://hg.mozilla.org/mozilla-central/rev/e59ac316362a