Closed Bug 989528 Opened 10 years ago Closed 10 years ago

Nail down planned semantics for AutoEntryScript and other JSAPI-related RAII guards

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: bholley, Assigned: bholley)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

So, I've been trying to sort out the right semantics for entry points on the script settings stack. This is all motivated by Bob's question of what we want to do in nsGlobalWindow::SetNewDocument in bug 988383. I'm filing this bug as a discussion forum, and CCing interested parties.

The basic issue is that there are lot of distinct concepts that we're hoping to map to entry points. The more of them we try to cram in, the more wishy-washy and imperfect the mapping becomes (i.e. what happened with JSContexts). So I want to design this right.

Here are the concepts and issues that I can think of. Some of these are things we currently need and hope to get rid of. Others are things we don't have but hope to get.

(A) A queryable mechanism to determine the proper script settings object per HTML5 for stuff like base URI and referer. This has clear meaning in the spec, and we should make sure that anything we tack onto it doesn't hamstring our ability to answer that question.
 
(B) Indicating to the JS engine that we're "running". This is currently done via JSAutoRequest and friends, which is bundled into AutoCxPusher, which is itself bundled into AutoEntryScript. This is currently important in non-exact-rooting builds to tell the conservative stack scanner when to run. It also does a few other odd things. Luke hopes to get rid of it (bug 722345).

(C) A place to save frame chains, barriering JS execution and making the engine appear that nothing is currently running. We're going to try to remove this concept with bug 979730.

(D) A mechanism to track where/how to report errors. In the long run, this probably boils down to either reporting on a given DOMWindow (and potentially invoking its onerror), or invoking a generic system-wide error reporter. See also the next item.

(E) A checkpoint to report uncaught errors. At present, this is done with a hodgepodge of things. If the JSContext has dontReportUncaught=true, the topmost caller is supposed to handle the exception manually, by either propagating it or manually calling JS_ReportPendingException. Otherwise, AutoLastFrameCheck calls the error reporter when it reaches a saved frame chain boundary. My tenatitive plan is to have AutoEntryScript (or something) do error reporting when it pops off the stack, unless the caller explicitly steals or squelches the exception. Bug 981187 is one of the bugs for this project.

(F) A way to indicate that the caller is "System". This is currently implemented by AutoSystemCaller, which internally pushes a sentinel onto the Script Settings Stack and a null JSContext onto the cx stack.

(G) A way to get a JSContext.

(An additional dimension of this issues is the question of entering a compartment. Long-term, we won't have JSContexts, and the JSRuntime itself will be in a compartment, or none at all).

When we're just running script in something like nsScriptLoader, it's pretty clear what we need to do. But when we're just poking around at stuff in JSAPI, it's much less clear. We're not really expecting to run script. But in general, we might, via accessors and such. And even if we don't, the code might throw exceptions, and in the long-run we don't want the JS engine playing any guessing games about whether there's "enough code running" to throw vs report.

Here are some thoughts, in no particular order:

* (E) seems like a major reason of why we need _something_ on the stack before calling into JSAPI at all. But it doesn't have to be the AutoEntryScript itself. It could be a helper that AutoEntryScript contains or inherits.

* The current "null cx push" mechanism of indicating a system caller has the nice feature that the callee can't accidentally run JS with system privileges. But the flip side is that callees everywhere have to worry about whether the caller left the engine in a usable state or not. This is effectively the issue with AutoJSContext, which may need to grab/push the SafeJSContext if one of its callers pushed null (see bug 979481). We should think about whether we want to bring this behavior into the new world.

* We need to decide what kind of behavior we want out of GetEntryPoint. Do we mind if script runs with no entry point on the stack? If we do, then we really need to push entry points all over the place, and they become less semantically meaningful. If we don't, all callers of GetEntryGlobal need to handle the null case.

* How much do we care about sealing off entry points when something "unrelated" happens? Suppose some DOM API does some JSAPI manipulation as an implementation detail. Should it see the caller's entry point? What about over nested event loops and such?
ni?ing two opinionated people to get the discussion going. But everyone should feel free to jump in with thoughts.
Flags: needinfo?(luke)
Flags: needinfo?(bzbarsky)
> either reporting on a given DOMWindow (and potentially invoking its onerror)

Or equivalent for workers, right?  Those have onerror bits going on too.

I'd like to add an item to your list:

(H) A way to get the ES spec's "current Realm".  This is _almost_ but not quite the same thing as the current compartment.  In particular it kinda differs in the case of Xrays, when we want to be creating objects in the callee Realm/global but doing security checks based on the caller origin.  :(  I'm not sure how best to set this up in a way that's not failure-prone.  :(

Right now we basically handle (E) via CallSetup (or via handwaving and pixie dust in the case of XPCWrappedJS and other XPConnect bits).  You're probably right that we should move this part of CallSetup to AutoEntryScript or some other object.  That said, we have to be a bit careful about the order in which we do these things, since we want to report exceptions while in the right compartment (whatever "right" means, given the onerror filtering bits).

> But the flip side is that callees everywhere have to worry about whether the caller
> left the engine in a usable state or not.

Mmm...  So I feel pretty strongly that any time we spin the event loop we should put the engine in an "unusable" state, just like it would be if the toplevel event loop were spinning.  So we need _some_ mechanism of doing that.

> Do we mind if script runs with no entry point on the stack?

What happens when such script performs an operation that depends on entry script settings objects?

> Suppose some DOM API does some JSAPI manipulation as an implementation detail.

It can't do manipulation of the web page's DOM, since that would so not be an implementation detail, right?  So presumably this is manipulating some sort of system objects?

Can we just assume that system code will be sane and that hence manipulation that doesn't expect calling into actual script (which would presumably set up an entry point) will in fact not call into script?  ;)
Flags: needinfo?(bzbarsky)
(In reply to Bobby Holley (:bholley) from comment #0)
The only point I'd add (which might go without saying) is
(I) (After all the reporting/save-frame-chain/request exorcisms are finished) The JS engine is left with a clear set of stack operations and invariants.

On this subject: I forget where the last set of discussions in bugs wound up, but is the following by any chance a valid summary of what JSAPI would provide:
 - Support for the implementing the script settings stack (currently: HideScriptedCaller/DescribeScriptedCaller)
 - For all stack iteration *other* than for the script settings stack: all frames are shown whose principal is subsumed (either from some given principal or from the current compartment's principal)
?

(In reply to Boris Zbarsky [:bz] from comment #2)
> Mmm...  So I feel pretty strongly that any time we spin the event loop we
> should put the engine in an "unusable" state, just like it would be if the
> toplevel event loop were spinning.  So we need _some_ mechanism of doing
> that.

+1

> Can we just assume that system code will be sane and that hence manipulation
> that doesn't expect calling into actual script (which would presumably set
> up an entry point) will in fact not call into script?  ;)

Somewhat mirroring the above, when we call into JSAPI in a way where we don't expect to transitively reenter content, can we put the system in a state where it will fail if we try to run content?  It'd be nice if we had hard guarantees about what entry points could run content.  As I type this I assume the exception is addon chrome JS; is there anything that can be done here?  If not, then it seems like we can't make the above assumption.
Flags: needinfo?(luke)
So after giving this some thought, here's the scheme that I've come up with:

class AutoJSAPI {}

This is a class that must be used before any operation on JSAPI. We can hijack the current CHECK_REQUEST assertions to enforce this, though it's quite possible that simply enforcing it at the level of the first JSAutoCompartment might be enough. When it's destroyed, this class makes sure to handle any exceptions on the stack to a system-wide error reporter, or optionally to a given global specified by .SetGlobalForErrorReporting() (So it takes care of (D) and (E)). For the time being, we can also use it to handle requests (i.e. (B)). When used on its own (i.e. without the AutoEntryScript subclass) it pushes the SafeJSContext and enters a null compartment, handling (C) and (G) for the non-entry-script case. It does not push anything onto the Script Settings stack. 


class AutoNoJSAPI {}

This puts the JS engine in an unusable state, thereby emulating the state of nature when there is no AutoJSAPI on the stack. It also sets the current compartment to null, and pushes a null Script Settings Entry. It asserts that there is no pending exception when it is instantiated. This takes care of (F), as well as Boris' and luke's concerns about nested event loops.


class AutoEntryScript : public AutoJSAPI

This handles the case where we plan to execute script. It does everything AutoJSAPI does, but additionally:
* Pushes the global onto the Script Settings Stack (handling (A)).
* Pushes the global's associated cx (handling (C) and (G), for now).
* Invokes SetGlobalForErrorReporting(global) (handling (D), for the case where we have specific error reporters)


This handles all of the points, except for (H) I think, which I'd like to punt on for the time being, because it feels sort of orthogonal.


As for the script settings stack - I'd like to see if we can make the JS engine MOZ_CRASH if we run script without an AutoEntryScript on the stack. But I think that's something we can try once this is all in place.

Sound good?
I'd like us to at least have a plan for (H)...  But yes, apart from that this sounds pretty reasonable.
Oh, would AutoJSContext do an AutoJSAPI?  It seems like the big concern with AutoJSAPI is that putting one on the stack is not a no-op because it will block errors propagating past it, right?  So seems like AutoJSContext might not want to do it.

Also, we'll need something wrt AutoJSAPI that lets us implement the exception reporting mode tristate CallSetup has right now.  Shouldn't be too bad.
(In reply to Boris Zbarsky [:bz] from comment #6)
> Oh, would AutoJSContext do an AutoJSAPI?  It seems like the big concern with
> AutoJSAPI is that putting one on the stack is not a no-op because it will
> block errors propagating past it, right?  So seems like AutoJSContext might
> not want to do it.

Right. It most definitely wouldn't. In the JSContext-free world, AutoJSContext maps to nothing, and AutoSafeJSContext maps to AutoJSAPI.
> 
> Also, we'll need something wrt AutoJSAPI that lets us implement the
> exception reporting mode tristate CallSetup has right now.  Shouldn't be too
> bad.

Yeah.
AutoJSAPI could have a bit different name. Something which hints that what it is for.
EnterJSAPIUsage is ugly, so is WillUseJSAPI. I don't have a good suggestion.

The name AutoEntryScript it is even less clear what it is for. Sounds like some 
script object on stack.
Overall this is sounding great.

Question about AutoJSAPI: does ~AutoJSAPI() always report pending exceptions, or does only the outermost ~AutoJSAPI() to do this?  I can think of reasons for and against:
 - for: AutoEntryScript would want the error reported with the given global
 - against: it seems like one would need to be careful about where AutoJSAPI was placed to avoid accidentally reporting errors too early or with the system reporter when an enclosing AutoEntryScript provides a global.  This would mostly be a worry if AutoJSAPI was scattered around (as we do with AutoCompartment) in random functions that use JSAPI that are sometimes are called without an enclosing AutoJSAPI.  This wouldn't be an issue if you think AutoJSAPI/AutoEntryScript or only called at a few pinchpoints that dominate all control flow into JSAPI.

Similar questions apply for AutoEntryScript.  Mostly I'm just wondering what nesting means: sometimes nesting means "these two things are totally unrelated" and sometimes the inner nesting is an extension of the outer.
(In reply to Olli Pettay [:smaug] from comment #8)
> AutoJSAPI could have a bit different name. Something which hints that what
> it is for.
> EnterJSAPIUsage is ugly, so is WillUseJSAPI. I don't have a good suggestion.

Absent a better suggestion, I like AutoJSAPI.

> The name AutoEntryScript it is even less clear what it is for. Sounds like
> some 
> script object on stack.

It could be renamed AutoEntryGlobal, which might be better.

(In reply to Luke Wagner [:luke] from comment #9)
> Overall this is sounding great.
> 
> Question about AutoJSAPI: does ~AutoJSAPI() always report pending
> exceptions

Yes, unless someone calls StealPendingException() before it goes out of scope.

>, or does only the outermost ~AutoJSAPI() to do this?

No.

>  - against: it seems like one would need to be careful about where AutoJSAPI
> was placed to avoid accidentally reporting errors too early or with the
> system reporter when an enclosing AutoEntryScript provides a global.  This
> would mostly be a worry if AutoJSAPI was scattered around (as we do with
> AutoCompartment) in random functions that use JSAPI that are sometimes are
> called without an enclosing AutoJSAPI.  This wouldn't be an issue if you
> think AutoJSAPI/AutoEntryScript or only called at a few pinchpoints that
> dominate all control flow into JSAPI.

It will certainly not be used as often as JSAutoCompartment. It roughly corresponds to the kind of place where someone would currently use AutoSafeJSContext. It means "I'm not interested or related to whatever JS execution is on the stack above me. I just want to do some stuff with JSAPI".
 
> Similar questions apply for AutoEntryScript.

These will be used any time we call into JSAPI planning to run script.

>  Mostly I'm just wondering what
> nesting means: sometimes nesting means "these two things are totally
> unrelated" and sometimes the inner nesting is an extension of the outer.

Somewhere in between the two really, depending on the operations performed at the nesting point.
OK, I'm going to use this bug to land the new infrastructure.
Assignee: nobody → bobbyholley
(In reply to Bobby Holley (:bholley) from comment #10)
Ok, that's good.

As for what this means to the JS engine:
 - AutoJSAPI will enter/leave requests (until we remove requests)
 - ~AutoJSAPI will check for pending exceptions, report them, and clear them
 - AutoEntryScript will HideScriptedCaller
 - ~AutoEntryScript will UnhideScriptedCaller
 - JS-visible stack-walking will visit all frames that are subsumed by cx->compartment->principals (except for DescribeScriptedCaller which checks if the current activation is hidden).
 - SaveFrameChain will go away
Is that all right?

Also: will AutoJSAPI enter some compartment?  Even when not executing scripts, most JSAPI methods depend on being in the "right" compartment (or, once default compartment object is removed, *some* compartment).  Assuming 'yes', then it seems like we could have a nice JS engine invariant that !cx->compartment() implies !cx->isExceptionPending().
Compartments is why I'd like to see a plan for (H).

Right now, compartments conflate two things: they're the security context for security checks and they're the global that will be used for JS_New*.

But if we want to move to the ES6 Realm model, the Realm becomes the global you use for JS_New*, while in the Xray case the security context is something different...  I'd like us to at least have an idea of how we plan to go about dealing with this.
I don't know any of the new tricks coming from Realms; what is a specific case that wouldn't be handled if, e.g., compartment == Realm?
The way Realms work is whenever you create objects you use the standard prototypes of the current Realm.

Consider a WebIDL API like so:

  object foo();

which is specified to do the following:

1)  If an object is not already cached, call ObjectCreate (in the sense of <http://people.mozilla.org/~jorendorff/es6-draft.html#sec-objectcreate>) using %ObjectPrototype% as the proto, and cache it.
2)  Return the cached object.

When called over Xrays, the compartment will be the calling chrome code.  But the object is being cached in the callee.  So it really ought to be created in the callee global.  This is pretty simple to do in a principled way: make sure that the "current Realm" as defined as http://people.mozilla.org/~jorendorff/es6-draft.html#sec-execution-contexts is used to create objects (which is how it works in the ES6 spec itself) and that the "current Realm" is set based on the callee (which, again, is how the ES6 spec works).  But then you have the "current Realm" and the "compartment" being different: the former is the callee, the latter is the caller (because security checks in the Xray case need to be based on the caller).

This is only a problem for Xrays.  Everything else can assume that compartment == Realm.
(In reply to Boris Zbarsky [:bz] from comment #15)
I vaguely recall this already being a problem with compartments and Xrays, even pre-Realms.  That is, I've heard this same callee vs. caller compartment situation before.  (It also sounded terrifying and I don't recall there being a great solution.)
There is no great solution right now.  I would like us to have one.

The Realm thing is mostly relevant in that even for cases when we don't cache we should really be creating stuff in the callee compartment to be consistent with how ES6 Realms work....
This all looks like it will make things much cleaner and clearer, even to someone like me, who's fairly new to this stuff, thanks.

(In reply to Bobby Holley (:bholley) from comment #10)
> (In reply to Olli Pettay [:smaug] from comment #8)
> > AutoJSAPI could have a bit different name. Something which hints that what
> > it is for.
> > EnterJSAPIUsage is ugly, so is WillUseJSAPI. I don't have a good suggestion.
> 
> Absent a better suggestion, I like AutoJSAPI.

I agree with smaug, it's not particularly obvious.
How about AutoJSAPITicket or Token / Pass / Session or some such thing?
For AutoNoJSAPI maybe AutoJSAPIBlocker.
(Oh, and all bike sheds should be painted grey.)


(In reply to Bobby Holley (:bholley) from comment #4)
> class AutoJSAPI {}
... 
> When it's destroyed, this class makes
> sure to handle any exceptions on the stack to a system-wide error reporter,
> or optionally to a given global specified by .SetGlobalForErrorReporting()

So, would this be used to replace AutoPushJSContextForErrorReporting?

> class AutoEntryScript : public AutoJSAPI

Why inheritance instead of a member variable?
The same goes for the current:
class AutoEntryScript : protected ScriptSettingsStackEntry
Particularly with these extra things added and if it is renamed to AutoEntryGlobal.

I suppose with the automatic way these types of objects work, it's not as important as it might be normally.
However, would you be able to call autoEntryScript.SetGlobalForErrorReporting() and set a different global?
Would that ever be a valid thing to do?
I feel pretty strongly that the discussion of (H) is out of scope for this bug (though I'm still very interested in having that discussion). Let's continue that discussion in bug 991661.
(In reply to Luke Wagner [:luke] from comment #12)
> As for what this means to the JS engine:
>  - AutoJSAPI will enter/leave requests (until we remove requests)
>  - ~AutoJSAPI will check for pending exceptions, report them, and clear them

Correct.

>  - AutoEntryScript will HideScriptedCaller
>  - ~AutoEntryScript will UnhideScriptedCaller

That's actually a good question. At present, we only use JSAutoHideScriptedCaller inside AutoIncumbentScript, but I guess in the AutoEntryScript case we may be relying on the saved frame chain stuff having a similar effect. It probably can't hurt, but we should figure out if it's actually necessary.

>  - JS-visible stack-walking will visit all frames that are subsumed by
> cx->compartment->principals (except for DescribeScriptedCaller which checks
> if the current activation is hidden).
>  - SaveFrameChain will go away

Correct.

> Is that all right?

> Also: will AutoJSAPI enter some compartment?

No. It will explicitly enter a null compartment, so as not to provide a footgun default (like the SafeJSContext global), and so as to force the consumer to explicitly enter a compartment.

> Assuming
> 'yes', then it seems like we could have a nice JS engine invariant that
> !cx->compartment() implies !cx->isExceptionPending().

I think we should instead base this on whatever state AutoJSAPI sets to indicate that it's active (rt->isActive() or somesuch).
(In reply to Bob Owen (:bobowen) from comment #18)
> This all looks like it will make things much cleaner and clearer, even to
> someone like me, who's fairly new to this stuff, thanks.

\o/ Sorry to hold up your work with this, but I wanted to make sure we do this right. :-)

 
> (In reply to Bobby Holley (:bholley) from comment #10)
> > (In reply to Olli Pettay [:smaug] from comment #8)
> > > AutoJSAPI could have a bit different name. Something which hints that what
> > > it is for.
> > > EnterJSAPIUsage is ugly, so is WillUseJSAPI. I don't have a good suggestion.
> > 
> > Absent a better suggestion, I like AutoJSAPI.
> 
> I agree with smaug, it's not particularly obvious.
> How about AutoJSAPITicket or Token / Pass / Session or some such thing?
> For AutoNoJSAPI maybe AutoJSAPIBlocker.
> (Oh, and all bike sheds should be painted grey.)

My feelings on the name: I think the struct's various purposes will be varied enough that it won't be simple to describe past "you need one of these on the stack before interacting with JSAPI". Something like AutoActivateJSAPI or AutoEnterJSAPI or AutoEnableJSAPI would be clearer in some ways, but also misleading. For example, it's kind of weird that AutoEnableJSAPI also does things with exceptions, and that you'd need to steal exceptions from it. Moreover, there will be a fair amount of AutoJSAPI nesting, which doesn't make as much sense as AutoEnableJSAPI.

> (In reply to Bobby Holley (:bholley) from comment #4)

>> So, would this be used to replace AutoPushJSContextForErrorReporting?

Exactly.

> > class AutoEntryScript : public AutoJSAPI
> 
> Why inheritance instead of a member variable?

I think that the is-a relationship here is helpful, as well as access to certain protected variables.

> The same goes for the current:
> class AutoEntryScript : protected ScriptSettingsStackEntry
> Particularly with these extra things added and if it is renamed to
> AutoEntryGlobal.

We need this, unfortunately, so that we can cast from a ScriptSettingsStackEntry to an AutoEntryScript. It'll go away eventually.

> However, would you be able to call
> autoEntryScript.SetGlobalForErrorReporting() and set a different global?
> Would that ever be a valid thing to do?

I don't think it would.
Blocks: 991754
(In reply to Bobby Holley (:bholley) from comment #21)

> \o/ Sorry to hold up your work with this, but I wanted to make sure we do
> this right. :-)

No problem, I already seem to have acquired quite a queue of other things I need to work on anyway.

> My feelings on the name: I think the struct's various purposes will be
> varied enough that it won't be simple to describe past "you need one of
> these on the stack before interacting with JSAPI". Something like
> AutoActivateJSAPI or AutoEnterJSAPI or AutoEnableJSAPI would be clearer in
> some ways, but also misleading. For example, it's kind of weird that
> AutoEnableJSAPI also does things with exceptions, and that you'd need to
> steal exceptions from it. Moreover, there will be a fair amount of AutoJSAPI
> nesting, which doesn't make as much sense as AutoEnableJSAPI.

Yeah, AutoEnableJSAPI was my first thought, but I rejected it for the same reasons.
That's why I thought maybe something like Ticket or Session, but that's probably already enough debate.
I doubt it will cause much confusion.
Attachment #8401891 - Flags: review?(bzbarsky)
Attachment #8401890 - Flags: review?(luke) → review+
Comment on attachment 8401891 [details] [diff] [review]
Part 2 - Implemented AutoJSAPI. v1

>+++ b/dom/base/ScriptSettings.cpp
> AutoEntryScript::AutoEntryScript(nsIGlobalObject* aGlobalObject,
>+  : AutoJSAPI(aCx ? aCx : FindJSContext(aGlobalObject), aIsMainThread)

Did you mean to pass true for aSkipNullAc here, since we're going to go ahead and construct mAc anyway?

>+++ b/dom/base/ScriptSettings.h
>+ * For any interaction with JSAPI, and AutoJSAPI (or one of its subclasses)

s/and/an/

>+ * This base class should be instantiated as-is when the caller wants to use
>+ * JSAPI but doesn't expect to run script.

I'm not sure what the "doesn't expect to run script" bit means...  How can a caller tell whether JSAPI will end up running script?  This might want more clear documentation or examples of the sorts of APIs that are OK to use an AutoJSAPI for.

>+ *   system goes through AutoJSAPI (see bug 951991). For now, this poisoning

s/poisoning/de-poisoning/?

r=me with those fixed
Attachment #8401891 - Flags: review?(bzbarsky) → review+
Comment on attachment 8401892 [details] [diff] [review]
Part 3 - Introduce AutoJSAPIWithErrorsReportedToWindow as a replacement for AutoPushJSContextForErrorReporting. v1

r=me
Attachment #8401892 - Flags: review?(bzbarsky) → review+
Comment on attachment 8401893 [details] [diff] [review]
Part 4 - Rename AutoSystemCaller to AutoNoJSAPI, and assert against pre-existing exceptions. v1

I'm not sure about the "Barrier" naming.  Too many other barriers around.

How about s/Barrier/NoJSAPI/ or something like that?

r=me with better naming.
Attachment #8401893 - Flags: review?(bzbarsky) → review+
Blocks: 971673
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: