Last Comment Bug 763233 - IonMonkey: Forbid JITing annotated frames
: IonMonkey: Forbid JITing annotated frames
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: David Anderson [:dvander]
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: LandIon
  Show dependency treegraph
Reported: 2012-06-09 15:02 PDT by David Anderson [:dvander]
Modified: 2012-08-16 18:27 PDT (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch for m-i (2.14 KB, patch)
2012-06-18 15:16 PDT, David Anderson [:dvander]
mrbkap: review+
Details | Diff | Splinter Review
patch for ionmonkey (6.17 KB, patch)
2012-06-18 15:17 PDT, David Anderson [:dvander]
jdemooij: review+
Details | Diff | Splinter Review

Description David Anderson [:dvander] 2012-06-09 15:02:55 PDT
IonMonkey cannot annotate frames, so js_setframeannotation may not work. Current plan is to rename the API to not take an fp, and to only operate on the topmost frame. From there, we can make the API invalidate and forbid ever JITing the calling function.
Comment 1 David Anderson [:dvander] 2012-06-09 15:10:10 PDT
Err, this isn't totally good enough since invalidation does not give us an fp. But maybe as part of the bailout we can move the annotation from the entry fp (which could no have had one at entry) to the bailed fp.
Comment 2 David Anderson [:dvander] 2012-06-18 15:16:30 PDT
Created attachment 634198 [details] [diff] [review]
patch for m-i

On try, SetFrameAnnotation's |fp| was always |cx->fp()|, so this should work.
Comment 3 David Anderson [:dvander] 2012-06-18 15:17:10 PDT
Created attachment 634200 [details] [diff] [review]
patch for ionmonkey

This patch disables IonMonkey for annotated frames.
Comment 4 Blake Kaplan (:mrbkap) 2012-06-18 15:42:05 PDT
Comment on attachment 634198 [details] [diff] [review]
patch for m-i

Review of attachment 634198 [details] [diff] [review]:

::: caps/src/nsScriptSecurityManager.cpp
@@ +2742,5 @@
>          return NS_ERROR_FAILURE; // XXX better error code?
>      }
>      if (NS_FAILED(principal->EnableCapability(capability, &annotation)))
>          return NS_ERROR_FAILURE;
> +    JS_SetTopFrameAnnotation(cx, annotation);

Is there a way we can assert here or in JS_SetTopFrameAnnotation that fp is the top frame?

::: js/src/jsdbgapi.cpp
@@ +524,2 @@
>  {
> +    cx->fp()->setAnnotation(annotation);

Is it worth asserting that we have a frame?
Comment 5 Jan de Mooij [:jandem] 2012-06-19 10:03:45 PDT
Comment on attachment 634200 [details] [diff] [review]
patch for ionmonkey

Review of attachment 634200 [details] [diff] [review]:

Looks good, I'm glad this didn't get too complicated.
Comment 6 David Anderson [:dvander] 2012-08-16 18:27:59 PDT

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