Remove simple uses of JS_FrameIterator

RESOLVED FIXED in mozilla14

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dvander, Assigned: dvander)

Tracking

unspecified
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

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?
Created attachment 614253 [details] [diff] [review]
remove easy xpconnect uses
Created attachment 614255 [details] [diff] [review]
remove easy dom, caps, content uses
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.
Created attachment 614458 [details] [diff] [review]
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.
Attachment #614253 - Flags: review?(bobbyholley+bmo)
Attachment #614255 - Flags: review?(mrbkap)
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?
Attachment #614253 - Flags: review?(bobbyholley+bmo) → review-
(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?
(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.
Created attachment 614655 [details] [diff] [review]
remove easy xpconnect uses
Attachment #614655 - Flags: review?(bobbyholley+bmo)
Attachment #614655 - Flags: review?(mrbkap)
Attachment #614253 - Attachment is obsolete: true
Created attachment 614657 [details] [diff] [review]
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.
Attachment #614458 - Attachment is obsolete: true
Comment on attachment 614655 [details] [diff] [review]
remove easy xpconnect uses

r=bholley on everything but AccessCheck. That part's for blake.
Attachment #614655 - Flags: review?(bobbyholley+bmo) → review+

Updated

5 years ago
Attachment #614655 - Flags: review?(mrbkap) → review+

Updated

5 years ago
Attachment #614255 - Flags: review?(mrbkap) → review+
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 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
Attachment #614657 - Flags: review?(mrbkap)

Updated

5 years ago
Attachment #614657 - Flags: review?(mrbkap) → review+
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
https://hg.mozilla.org/mozilla-central/rev/ecd8434e43d9
https://hg.mozilla.org/mozilla-central/rev/546132d79a53
https://hg.mozilla.org/mozilla-central/rev/e59ac316362a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.