Last Comment Bug 744617 - Remove simple uses of JS_FrameIterator
: Remove simple uses of JS_FrameIterator
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: David Anderson [:dvander]
:
Mentors:
Depends on:
Blocks: 732653
  Show dependency treegraph
 
Reported: 2012-04-11 15:26 PDT by David Anderson [:dvander]
Modified: 2012-04-17 07:32 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
remove easy xpconnect uses (5.83 KB, patch)
2012-04-11 19:28 PDT, David Anderson [:dvander]
bobbyholley: review-
Details | Diff | Review
remove easy dom, caps, content uses (8.59 KB, patch)
2012-04-11 19:32 PDT, David Anderson [:dvander]
mrbkap: review+
Details | Diff | Review
remove uses from nsGlobalWindow, wrappers/AccessCheck (2.03 KB, patch)
2012-04-12 11:11 PDT, David Anderson [:dvander]
no flags Details | Diff | Review
remove easy xpconnect uses (5.80 KB, patch)
2012-04-12 19:18 PDT, David Anderson [:dvander]
bobbyholley: review+
mrbkap: review+
Details | Diff | Review
remove uses from nsGlobalWindow, wrappers/AccessCheck (3.31 KB, patch)
2012-04-12 19:21 PDT, David Anderson [:dvander]
mrbkap: review+
Details | Diff | Review

Description David Anderson [:dvander] 2012-04-11 15:26:22 PDT
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] 2012-04-11 19:28:40 PDT
Created attachment 614253 [details] [diff] [review]
remove easy xpconnect uses
Comment 2 David Anderson [:dvander] 2012-04-11 19:32:31 PDT
Created attachment 614255 [details] [diff] [review]
remove easy dom, caps, content uses
Comment 3 David Anderson [:dvander] 2012-04-11 19:44:58 PDT
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] 2012-04-12 11:11:59 PDT
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.
Comment 5 Bobby Holley (busy) 2012-04-12 13:17:46 PDT
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] 2012-04-12 13:32:13 PDT
(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 (busy) 2012-04-12 13:45:50 PDT
(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] 2012-04-12 19:18:58 PDT
Created attachment 614655 [details] [diff] [review]
remove easy xpconnect uses
Comment 9 David Anderson [:dvander] 2012-04-12 19:21:30 PDT
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.
Comment 10 Bobby Holley (busy) 2012-04-12 19:53:47 PDT
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 2012-04-13 02:46:24 PDT
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 2012-04-13 02:47:11 PDT
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

Note You need to log in before you can comment on or make changes to this bug.