1331662 - nsJSUtils::EvaluateString does too many things.

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Nicolas B. Pierron [:nbp] {away}]

Currently nsJSUtils::EvaluateScript handles a lot of cases, and it handles them inconsitently between all of them.  Bug 900784 prototype patches adds even more complexity by adding an extra JSScript out-param to the function, which does not behave consistently either (Bug 900784 comment 50 and Bug 900784 comment 53).

Another inconsistency is related to the usage of off-thread parsing, while running with options which expects to have a return value.  I guess this case never happens in practice, because the returned value is never set when using off-thread-parsing.

One of the reason which is likely the root cause of this, is the need to avoid code duplication for the setup of the compartment.

I think this problem would be better solved by using lambdas or RAII.

Comment 1

[Nicolas B. Pierron [:nbp] {away}]

Attachment: [lambda] Replace nsJSUtils::EvaluateString by SyncAndExec, CompileAndExec and InCompartment functions.

I am only asking for feedback, because I am currently hunting a bug from
this patch which cause Firefox to only show the URL bar, with a
non-actionable hamburger button and a blank page.

Otherwise, this patch should highlight the direction I took so far, which is
to split the EvaluateString function into SyncAndExec, CompileAndExec, and
InCompartment.

The signature of the function is made such that one cannot miss-use them, as
InCompartment is the only function capable of creating the CompartmentScope
reference expected as argument of SyncAndExec, and CompileAndExec.

I noticed that EvaluateString was used in 2 different contextes.

  1. This is a top-level script, and we do not expect any return value, and
     the scopechain argument is empty.

  2. This is a function evaluated in the context of the scope chain of
     another function, such as XBL and plugins.

So, I named them accordingly, (1) being the TopLevel context and (2) being
the Embedded context.

InCompartment is overloaded to handle (1) and (1 & 2) cases, where the (1 &
2) case rely on the (1) case, by adding the return value management.

One detail of implementation is the use of std::function<>, and in
particular the way the argument is provided.  std::function<> are by default
allocating space to copy the function content.  Lambda are not lightweight
objects, and I would prefer to avoid allocations that cannot be checked.
std::function<> constructor has one particular implementation detail about
function pointers and std::reference_wrapper [1], and thus to avoid allocations
we have to explictly take a reference_wrapper of the lambdas.

[1] http://en.cppreference.com/w/cpp/utility/functional/function/function

Comment 2

[Nicolas B. Pierron [:nbp] {away}]

Comment on attachment 8828853 [details] [diff] [review]
[lambda] Replace nsJSUtils::EvaluateString by SyncAndExec, CompileAndExec and InCompartment functions.

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

Ok, I am now able to run the browser successfully with the following self-nits applied.
(feedback -> review)

::: dom/base/nsJSUtils.cpp
@@ +176,2 @@
>  
> +  auto topLevelSwitch = [&](CompartmentScope& aCs) {

self-nit: We have to set the return value in CompileAndExec, thus we should set aCs.embedded as follow:

  aCs.embedded = &aEmbedded;

@@ +197,3 @@
>        JSString* str = JS::ToString(aCx, value);
> +      if (!!str) {
> +        aEmbedded.retValue.set(JS::UndefinedValue());

self-nit: Inverted condition.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8828853 [details] [diff] [review]
[lambda] Replace nsJSUtils::EvaluateString by SyncAndExec, CompileAndExec and InCompartment functions.

Redirecting to Boris since my knowledge of js api is marginal at best.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Sorry for the lag here... I've been trying to understand the new setup and why it's better.  It trades off having all the code be in one place (but branchy and complicated) for having the code scattered among multiple callbacks and very much nonlocal, and imo harder to reason about.

Why do we need to expose this complexity in the API?  In API terms, there are really only a few things people do here.  We should try to make those clear and understandable, with more or less one public API function per task; then we can figure out how to improve the implementation to be less branchy/complicated and still support that API, right?

I have no problem with renaming the EvaluateString that takes a script token to something else, obviously.  I agree that having it named EvaluateString is confusing, since that's not at all what it does.

Comment 5

[Nicolas B. Pierron [:nbp] {away}]

The problem I faced, is that with Bug 900784, I was either adding more branches or duplicating code[1].  I found even more confusing and harder to maintain, from a security point of view.

In comparison, this work allow me to instead add a SyncEncodeAndExec and a DecodeAndExec[2] which are first better named, and then shorter, and thus less likely to be buggy nor to introduce bug in nearby code.

One of the problem I have with EvaluateString, is the fact that it takes so many argument that we lose track[3] of what is related to what.  This patch adds dedicated functions, which do loading & eval logic and can be customize without adding even more arguments to the EvaluateString functions, nor having to change every caller[4] when another unrealted argument is added.

In comparison the InCompartment function is overloaded twice, depending if we want to evaluate the code in the context of the global, at the top-level, or in the context of an inner function like many other uses cases.

I think one of the issue here is that the lambda is not given directly as argument to the function, and this C++ limitation(5) makes things harder to read.  If I were to modify the code to male the callers look like the following be preferable from your point of view?

  rv = nsJSUtils::InCompartment(cx, topLevelCtx, [&](nsJSUtils::CompartmentScope& aCs) {
    return SyncAndExec(aCs, …);
  });

Otherwise, what kind of API are you thinking of?  Is the complexity coming from the TopLevelContext and EmbeddedContext classes, in which case we can remove them by adding extra arguments to the InCompartment functions.  What do you tihnk?

I honestly think that the example above is clear and easy to follow.  It also removes unused/useless code in the context of the nsScriptLoader and should use less stack space.  I would assume that the CPU prefetcher should be able to follow function pointers given as argument, solving the locality issue.

[1] (Bug 900784) attachment 8800644 [details] [diff] [review]
[2] https://github.com/nbp/gecko-dev/commit/c1d086ef9442e28bc1e201da2865a4c37e7b8a73?diff=split#diff-ae87530e1e8b54e2f265b7c1dceff482R299
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=900784#c53
[4] https://bugzilla.mozilla.org/show_bug.cgi?id=900784#c51
(5) C++ has no way to give a lambda as argument to a function without allocating with an std::function, or using templates.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I understand your concerns with the "private" EvaluateString function, which does in fact have too many arguments.

My concern is with the "public" API.  The public API basically consists of a very small number of operations (not necessarily well reflected in our current public API!):

1)  Evaluate the given string (always nsAString; the JS::SourceBufferHolder version 
    looks unused to me) against the given global, with the given compile options
    and given scope chain, and return the resulting value.  One of the callers here has an empty scope
    chain (and is the only consumer of setCoerceToString) the others have nonempty scope
    chains.  So maybe this should actually be two separate APIs: one with scope chain and
    no coercion, one without scope chain but with coercion.
2)  Evaluate the given string (nsAString or JS::SourceBufferHolder) with the given
    compile options and given global (implies empty scope chain, no return value needed).
3)  Take the given off-thread compilation token, finish the script, and execute it
    against the given global.  I guess we do in fact need the global here,
    because JS::FinishOffThreadScript wants to be done in the right compartment.
4)  Whatever new thing(s) you are adding.

Agreed on that?  If we agree on that, then I think we should make the public API actually reflect those tasks.  Then we can implement it internally with RAII, and lambdas if needed and whatnot, but at that point the lambdas would all be self-contained in nsJSUtils and much simpler to reason about, imo.

If you think this isn't very workable, maybe the problem is that I don't have a good handle on the new public API you want to add...

> and can be customize without adding even more arguments to the EvaluateString
> functions, nor having to change every caller[4]

Maybe I'm missing something, but I don't see the thing linked to at [4] having to change every caller...  In fact, it doesn't mention nsJSUtils at all.

> Is the complexity coming from the TopLevelContext and EmbeddedContext classes

I think for me the complexity comes from it just being hard to follow what the logic flow is and not having a good idea of what the invariants are.  By the time the lambda is executed, what work has already happened?  What work is the lambda responsible for?

It doesn't help (and this is a problem before your patches too!) that my public API #3 really doesn't need a bunch of the stuff we pass in for all the other cases, and in particular doesn't need a CompileOptions.  But I suspect neither do your two new cases, right?  Forcing all TopLevelContext consumers to produce a CompileOptions isn't all that great.  Ideally, it would only be passed to my cases 1-3 above, and the CompileOptions-related assert would move out of the shared "set up the compartment" code and closer to the actual compilation...

Comment 7

[Nicolas B. Pierron [:nbp] {away}]

Summary of the IRC discussion with bz:

We are moving away from lambda implementation toward a RAII implementation. Thus, replacing

  rv = nsJSUtils::InCompartment(cx, global, [&](nsJSUtils::CompartmentScope& aCs) {
    return SyncAndExec(aCs, …);
  });

by

  {
    nsJSUtils::ExecuteContext execCtx(cx, global);
    rv = nsJSUtils::SyncAndExec(execCtx, …);
  }

Comment 8

[Nicolas B. Pierron [:nbp] {away}]

Attachment: Replace nsJSUtils::EvaluateString by an ExecutionContext class with different compile and execute functions.

This patch re-do the same modification using RAII.

One of the problem which arised was the need for the enterCompartment
function.  The enterCompartment function is used to do the same early return
that is made if xpc::Scriptability::Get(aGlobal).Allowed() fails.

I do not know if this check is needed for all the caller, or if it can be
moved outside only one of the caller.

As I added an enterCompartment, I named the other function
exitCompartment.

Sadly, the code to be made at each call-site is quite verbose.

Comment 9

[Nicolas B. Pierron [:nbp] {away}]

Comment on attachment 8832488 [details] [diff] [review]
Replace nsJSUtils::EvaluateString by an ExecutionContext class with different compile and execute functions.

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

::: dom/base/nsJSUtils.cpp
@@ +156,5 @@
> +  : ExecutionContext(aCx)
> +{
> +  MOZ_ASSERT_IF(aCoerceToString, !aCompileOptions.noScriptRval);
> +  mHasReturnValue_ = !aCompileOptions.noScriptRval;
> +  mCoerceToString_ = aCoerceToString;

I think it would be better to have a

   exec.setExpectReturnValue()
       .setCoerceToString(true)

Instead of the boolean argument on the constructor.

Having a

  using CoerceToString = bool;

in the class is quite verbose, as it would be used as:

  nsJSUtils::ExecutionContext::CoerceToString(true)

::: dom/base/nsJSUtils.h
@@ +64,5 @@
>                                    const nsAString& aBody,
>                                    JSObject** aFunctionObject);
>  
> +  // ExecutionContext is used to switch compartment.
> +  class MOZ_STACK_CLASS ExecutionContext {

I think this name is terrible, I am open to better suggestions.

Comment 10

[Nicolas B. Pierron [:nbp] {away}]

Comment on attachment 8832488 [details] [diff] [review]
Replace nsJSUtils::EvaluateString by an ExecutionContext class with different compile and execute functions.

I will note that this patch does not work, because I am using JSErrorToNSResult, but I did not pay attention that the returned value are *NS_SUCCESS*_failure_case, which does not work as I would have expected when flowing in NS_FAILED.

I am trying to re-make this patch with more incremental modification to avoid such mistakes.

Comment 11

[Nicolas B. Pierron [:nbp] {away}]

Attachment: part 1 - Reimplement EvaluateString using the ExecutionContext class.

This patch add all ExecutionContext methods, and re-implement the current
EvaluateString function with the ExecutionContext functions.

This implementation does not attempt to be clever as the opposite one, and
keeps every variable which was present before as member of the
ExecutionContext.

I think we can improve on the mOk_ and mRv_ states, but I would leave this
task for a follow-up patch.

Comment 12

[Nicolas B. Pierron [:nbp] {away}]

Attachment: part 2 - Replace nsJSUtils::EvaluateString calls by ExecuteContext scope.

This part moves the implementation of the EvaluateString function to all the
callers.

I use the do-break-while(false) patterns as a way to avoid adding more and more
levels of indentation, which would latter matter for upcoming
nsScriptLoader.cpp patches (Bug 900784)

Comment 13

[Boris Zbarsky [:bzbarsky]]

OK, so looking at the resulting API, I'd like to propose a few changes:

1)  Have ExecutionContext have the compartment semantics of JSAutoCompartment, so it just enters the compartment in its ctor and leaves in its dtor.

2)  Just store the value of xpc::Scriptability::Get(aGlobal).Allowed() in the ctor, then check it in the various methods and just don't do any work if not allowed.  The actual methods would return nsresult if they failed.

3)  Replace the one-arg version of exitCompartment() with an extractReturnValue() or something along those lines.  

4)  Just infer the return value thing in compileAndExec, since we have a CompileOptions there anyway.

With those changes, the use in nsScriptLoader.cpp will look like this:

  nsJSUtils::ExecutionContext exec(aes.cx(), global);
  if (aRequest->mOffThreadToken) {
    rv = exec.SyncAndExec(&aRequest->mOffThreadToken, &script);
  } else {
    nsAutoString inlineData;
    SourceBufferHolder srcBuf = GetScriptSource(aRequest, inlineData);
    rv = exec.CompileAndExec(options, srcBuf);
  }

and the callsite in nsXBLProtoImplField.cpp will look like this:

    nsJSUtils::ExecutionContext exec(cx, scopeObject);
    rv = exec.CompileAndExec(options, nsDependentString(mFieldText,
                                                        mFieldTextLength));
    if (NS_FAILED(rv)) {
      return rv;
    }    
    rv = exec.ExtractReturnValue(&result);

where extractReturnValue would make sure to never clobber an existing NS_SUCCESS_DOM_SCRIPT_EVALUATION_THREW (which it can do, because it knows what compileAndExec returned), etc.

How does that sound?

As a general other comment, please no trailing '_' on member names, and method names start with uppercase chars.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8832963 [details] [diff] [review]
part 1 - Reimplement EvaluateString using the ExecutionContext class.

Pending updated patches.

Comment 15

[Nicolas B. Pierron [:nbp] {away}]

Attachment: part 1 - Reimplement EvaluateString using the ExecutionContext class.

Delta:
 - Follow comment 13 & irc recommendation.
 - Rename m*_ to m*.
 - Capitalize member function names.
 - Rename mOk_ to mSkip.
 - Replace Maybe<JSAutoCompartment> by JSAutoCompartment, and initialize it
   during the ExecutionContext constructor.
 - Move the JS_WrapValue out of the ExtractReturnValue (and forgot to add it
   back in this patch, but they should be present in part 2)
 - forward the mRv result to be returned from ExtractReturnValue if mSkip is set.

Comment 16

[Nicolas B. Pierron [:nbp] {away}]

Attachment: part 2 - Replace nsJSUtils::EvaluateString calls by ExecuteContext scope.

Delta:
 - Follow part 1 modifications, and add JS_WrapValue in all cases which are
   expecting a returned value.

I know I could remove some of the JS_WrapValue, but I prefer to keep these
changes mechanical for the moment.  Maybe I should make a part 3 to replace
these by assertSameCompartment.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8834889 [details] [diff] [review]
part 1 - Reimplement EvaluateString using the ExecutionContext class.

>+++ b/dom/base/nsJSUtils.cpp
>+  PROFILER_LABEL("nsJSUtils", "ExecutionContext",

This isn't quite right: PROFILER_LABEL sets up an RAII class (SamplerStackFrameRAII) using its args.  That used to measure the entire evaluation, but now will just measure the ExecutionContext ctor...

We probably want to make the SamplerStackFrameRAII a member instead.

>+nsJSUtils::ExecutionContext::SyncAndExec(void **aOffThreadToken,
>+  if (!aScript) {
(stuff)
>+  if (!JS_ExecuteScript(mCx, mScopeChain, aScript)) {
(same stuff)

I'd slightly prefer we do:

  if (!aScript || !JS_ExecuteScript(mCx, mScopeChain, aScript)) {
    mSkip = true;
    mRv = EvaluationExceptionToNSResult(mCx);
    return mRv;
  }

but either way.

>+nsJSUtils::ExecutionContext::ExtractReturnValue(JS::MutableHandle<JS::Value> aRetValue)
>+  if (mHasReturnValue) {

I can't think of any sane reason someone would call ExtractReturnValue if they did not want a return value.  I think this function should be able to assert mHasReturnValue up front.

I realize that makes it a slight bit more complicated for this intermediate step where we want to build EvaluateString on top of it (because now we need to make the ExtractReturnValue call conditional).  But we're about to remove EvaluateString anyway, right?

>+++ b/dom/base/nsJSUtils.h
>+    // Compartment in which we should switch to for the execution of the script.

Maybe:

  // Handles switching to our global's compartment.

?

>+    bool mHasReturnValue;

Maybe mWantReturnValue?

>+    bool mExpectScopeChain;

This seems to be unused.  Should we be asserting something about it somewhere?  Perhaps in SyncAndExec asserting that it's false?

>+    // must come from an AutoJSAPI that has had TakeOwnershipOfErrorReporting()

Actually, it needs to come from an AutoEntryScript (a slightly more stringent requirement).  Yes, I know the comment used to say what this says; it never got updated.

>+    // option should be set to false. The returned value would be wrapped in the
>+    // |aCx| compartment when |exitCompartment| is called.

There is no exitCompartment....

I'm not actually sure we need a SetReturnValue option at all.  It's only valid to use with CompileAndExec, right?  And CompileAndExec already takes a CompileOptions.  So can we not just examine .noScriptRval in CompileAndExec to set mWantsReturnValue?

That would not allow us to eagerly assert in SetCoerceToString, but we could assert in CompileAndExec that !mCoerceToString || mWantsReturnValue.

>+    // Set the scope chain in which the execution of the code would be in.

"Set the scope chain in which the code should be executed."

>+    ExtractReturnValue(JS::MutableHandle<JS::Value> aRetValue);

This should document that the value will be returned in the compartment of the aGlobal passed to the constructor.

>+    MOZ_MUST_USE nsresult SyncAndExec(void **aOffThreadToken,

Document that the resulting script is returned in aScript?

r=me with the above nits.  Thank you again for being so patient!

Comment 18

[Boris Zbarsky [:bzbarsky]]

> So can we not just examine .noScriptRval in CompileAndExec to set mWantsReturnValue?

Oh, I see.  Some overloads of EvaluateString call setNoScriptRval right now.  So I still think we don't need a SetReturnValue, but we do need to make sure that callers that do NOT want an rval call setNoScriptRval(), ok.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8834892 [details] [diff] [review]
part 2 - Replace nsJSUtils::EvaluateString calls by ExecuteContext scope.

>+++ b/dom/base/nsGlobalWindow.cpp

>       JS::CompileOptions options(aes.cx());

This callsite needs to call setNoScriptRval(true), I believe.

>+++ b/dom/base/nsScriptLoader.cpp

The CompileOptions and FillCompileOptionsForRequest bits can move into the !aRequest->mOffThreadToken branch, right?

>+++ b/dom/jsurl/nsJSProtocolHandler.cpp
>+        if (NS_FAILED(rv)) {
>+            return rv;
>+        }

No, that's not right.  We want to return NS_ERROR_MALFORMED_URI if rv is a failure code here.

This is why we set things up so ExtractReturnValue would do the right thing if a previous *Exec failed.  The code here should look like this:

  exec.CompileAndExec(options, NS_ConvertUTF8toUTF16(script));
  rv = exec.ExtractReturnValue(&v);

imo.

> +    if (!JS_WrapValue(cx, &v)) {

No need; please take this out.  "v" is in the compartment of "globalJSObject", which is innerGlobal->GetGlobalJSObject().  cx comes from AutoEntryScript aes(innerGlobal, ...), which enters the compartment of innerGlobal->GetGlobalJSObject().  So v is already in the right compartment.

r=me with the above fixed.

Comment 20

[Nicolas B. Pierron [:nbp] {away}]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #19)
> Comment on attachment 8834892 [details] [diff] [review]
> part 2 - Replace nsJSUtils::EvaluateString calls by ExecuteContext scope.
> 
> >+++ b/dom/base/nsGlobalWindow.cpp
> 
> >       JS::CompileOptions options(aes.cx());
> 
> This callsite needs to call setNoScriptRval(true), I believe.

Good catch. I will toggle mWantsReturnValue in ExtractReturnValue and ensure that it is false in the ExecutionContext destructor to catch these issues, as mWantsReturnValue is set based on the CompileOptions during CompileAndExec function calls.

Comment 21

[Nicolas B. Pierron [:nbp] {away}]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #19)
> Comment on attachment 8834892 [details] [diff] [review]
> part 2 - Replace nsJSUtils::EvaluateString calls by ExecuteContext scope.
> 
> >+++ b/dom/base/nsScriptLoader.cpp
> 
> The CompileOptions and FillCompileOptionsForRequest bits can move into the
> !aRequest->mOffThreadToken branch, right?
> 

Right, but I will keep it like that for the moment, as I am adding a new branch which would depend on it being lifted to avoid redundant code as part of Bug 900784.

> > +    if (!JS_WrapValue(cx, &v)) {
> 
> No need; please take this out.  "v" is in the compartment of
> "globalJSObject", which is innerGlobal->GetGlobalJSObject().  cx comes from
> AutoEntryScript aes(innerGlobal, ...), which enters the compartment of
> innerGlobal->GetGlobalJSObject().  So v is already in the right compartment.

Would it make sense to add an AssertSameCompartment call instead of the JS_WrapValue then?

Comment 22

[Boris Zbarsky [:bzbarsky]]

> Would it make sense to add an AssertSameCompartment call instead of the JS_WrapValue then?

Yes.  Unfortunately, we don't have a great version of that for Value, so it will require manual checking for v.isObject().

Comment 23

[Nicolas B. Pierron [:nbp] {away}]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f57af00bac049bee999793636ca1fe9a25a5189f

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6a142776fee
part 1 - Reimplement EvaluateString using the ExecutionContext class. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e48165f8c8e
part 2 - Replace nsJSUtils::EvaluateString calls by ExecutionContext scopes. r=bz

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b6a142776fee
https://hg.mozilla.org/mozilla-central/rev/4e48165f8c8e