Refactor ContextOptions from a bitfield into a proper struct

RESOLVED FIXED in mozilla27

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bholley, Assigned: ejpbruel)

Tracking

(Depends on: 1 bug, {dev-doc-needed})

unspecified
mozilla27
x86
Mac OS X
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments, 3 obsolete attachments)

24.75 KB, patch
bholley
: feedback+
luke
: feedback+
Details | Diff | Splinter Review
26.78 KB, patch
bholley
: review+
Details | Diff | Splinter Review
16.28 KB, patch
bholley
: review+
Details | Diff | Splinter Review
4.52 KB, patch
bholley
: review+
Details | Diff | Splinter Review
13.38 KB, patch
bholley
: review+
Details | Diff | Splinter Review
6.04 KB, patch
khuey
: review+
Details | Diff | Splinter Review
20.45 KB, patch
khuey
: review+
Details | Diff | Splinter Review
5.22 KB, patch
sfink
: review+
Details | Diff | Splinter Review
783 bytes, patch
bholley
: review+
Details | Diff | Splinter Review
10.37 KB, patch
bholley
: review+
Details | Diff | Splinter Review
4.58 KB, patch
bholley
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
Right now, they live on the JSContext, which makes no sense. When we want to JIT something, we read these flags off whatever JSContext we happen to be using, which often gives us the wrong behavior (see bug 776798).

This is also something that needs to happen for the de-cxification work.
(Reporter)

Updated

5 years ago
Depends on: 880917

Comment 1

5 years ago
Will this allow shell scripts to control JIT options (including eagerness) for each global?  Because that would be awesome for differential testing.
(In reply to Jesse Ruderman from comment #1)
> Will this allow shell scripts to control JIT options (including eagerness)
> for each global?  Because that would be awesome for differential testing.

Eagerness is a kind of "process-wide" setting right now, both in the browser and shell. We could expose a function to the shell to change these values though.

Comment 3

5 years ago
Setting --ion-eager globally, and then turning off ion entirely for one global, would work too.

Comment 4

5 years ago
This would allow me to greatly expand differential testing (see also bug 909997).
(Reporter)

Comment 5

5 years ago
Eddy is going to take this \o/
Assignee: bobbyholley+bmo → ejpbruel
(Assignee)

Updated

5 years ago
Duplicate of this bug: 885786

Comment 7

5 years ago
I was thinking: could the flags instead be on the JSRuntime?  I know we have separate chrome/content options, but we also, inside the JS engine, have cx->runningWithTrustedPrincipals() so the JSRuntime could just have separate ionJitInTrustedCode/ionJitInUntrustedCode options.
(Reporter)

Comment 8

5 years ago
(In reply to Luke Wagner [:luke] from comment #7)
> I was thinking: could the flags instead be on the JSRuntime?  I know we have
> separate chrome/content options, but we also, inside the JS engine, have
> cx->runningWithTrustedPrincipals() so the JSRuntime could just have separate
> ionJitInTrustedCode/ionJitInUntrustedCode options.

I think there are at least some flags that we want to be per-compartment (like strict), at which point we'll need that machinery anyway.

Comment 9

5 years ago
Anything other than strict?  Btw, who is running with strict?
(Reporter)

Comment 10

5 years ago
Well also, I think we _want_ to be able to twiddle the jit flags on a per-compartment basis, for fuzz testing and whatnot.

Comment 11

5 years ago
I know we want to twiddle JIT flags dynamically; I wasn't aware of the desire to do it on a per-compartment basis.  If we do really need to be able to set certain options on an ad hoc per-compartment basis, I still think we should have JSRuntime-global flags but also allow a JSCompartment to optionally override them.  If we wanted to add special (enable|disable)(Ion|Baseline|Strict)InThisCompartment functions, I think that's the behavior that we'd want (i.e., when you flip javascript.options.ion.content, you don't clobber the flag in a compartment that has explicitly set the flag.

My underlying concern here is, based on the bad experience with putting the flags on JSContext, having it be simple to know that we are correctly propagating flags everywhere we should and there are a lot fewer JSRuntimes and places manipulating JSRuntimes than JSCompartments.
(Reporter)

Comment 12

5 years ago
(In reply to Luke Wagner [:luke] from comment #11)
> I know we want to twiddle JIT flags dynamically; I wasn't aware of the
> desire to do it on a per-compartment basis.

See comment 1.

If we do really need to be able
> to set certain options on an ad hoc per-compartment basis, I still think we
> should have JSRuntime-global flags but also allow a JSCompartment to
> optionally override them.  If we wanted to add special
> (enable|disable)(Ion|Baseline|Strict)InThisCompartment functions, I think
> that's the behavior that we'd want (i.e., when you flip
> javascript.options.ion.content, you don't clobber the flag in a compartment
> that has explicitly set the flag.

That sounds reasonable.

> My underlying concern here is, based on the bad experience with putting the
> flags on JSContext, having it be simple to know that we are correctly
> propagating flags everywhere we should and there are a lot fewer JSRuntimes
> and places manipulating JSRuntimes than JSCompartments.

That's a fair point. I think this is much easier to do with JSCompartments/globals than it is with JSContexts (since the former don't fly all over the place), but I'd be fine to put it directly on the runtime too and add overrides wherever we need them.

Updated

5 years ago
Blocks: 862503
(Assignee)

Comment 13

5 years ago
Created attachment 803897 [details] [diff] [review]
patch

As a first step, I want to get rid of the options bitfield on the JSContext, and replace it with a struct with proper getter/setters. Since everything else kind of hinges on this, I'd like to make sure you agree with the approach. Let me know what you think of the proposed patch.

Open questions:
- Do we really need a JS::Options function in JSAPI? If so, does it really need to be in the JS namespace? (I don't know what namespace is supposed to contain what).

- Do we really need these getter/setters on ContextOptions? Currently, they don't have any side effects, but that may change in the future, and having them around can't hurt. Also, setters returning a reference to this allows us to chain setter calls, which improves readability.
Attachment #803897 - Flags: feedback?
(Assignee)

Updated

5 years ago
Attachment #803897 - Flags: feedback? → feedback?(bobbyholley+bmo)
(Reporter)

Comment 14

5 years ago
Comment on attachment 803897 [details] [diff] [review]
patch

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

> Open questions:
> - Do we really need a JS::Options function in JSAPI?

As opposed to what? We need some way to toggle these things. I'm somewhat open to leaving the bitfield for context options (since that's all going away), but the name JS_{Get,Set}Options would probably need to change, at which point we might as well just eliminate the bitfield entirely.

> If so, does it really
> need to be in the JS namespace? (I don't know what namespace is supposed to
> contain what).

I think it should be in namespace JS.

> - Do we really need these getter/setters on ContextOptions? Currently, they
> don't have any side effects, but that may change in the future, and having
> them around can't hurt. Also, setters returning a reference to this allows
> us to chain setter calls, which improves readability.

One option would be to do what CompileOptions does here, and use setters but not getters. I've often wondered what's preferred here. Let's let luke make the call.
Attachment #803897 - Flags: feedback?(luke)
Attachment #803897 - Flags: feedback?(bobbyholley+bmo)
Attachment #803897 - Flags: feedback+

Comment 15

5 years ago
Making ContextOptions similar to CompileOptions is a generally good goal, but I think it was a mistake to give CompileOptions public fields.  I just moved two CompileOptions fields to be private so I could so something interesting.  So let's do what you did: private fields w/ getters/setters.

Also, since we're moving away from "Context" (what *is* a Context anyway?), how about JS::RuntimeOptions (contrasts nicely with JS::CompileOptions)?
(Reporter)

Comment 16

5 years ago
(In reply to Luke Wagner [:luke] from comment #15)
> Making ContextOptions similar to CompileOptions is a generally good goal,
> but I think it was a mistake to give CompileOptions public fields.  I just
> moved two CompileOptions fields to be private so I could so something
> interesting.  So let's do what you did: private fields w/ getters/setters.
> 
> Also, since we're moving away from "Context" (what *is* a Context anyway?),
> how about JS::RuntimeOptions (contrasts nicely with JS::CompileOptions)?

Because, at the moment, some of these options need to live on the context (like JSOPTION_NO_DEFAULT_COMPARTMENT_OBJECT). Eddy is going to make a separate structure for runtime options, and migrate what he can.

Comment 17

5 years ago
Ohhh, I missed that.  So the goal of ContextOptions is for it to eventually be very small or empty?
(Reporter)

Comment 18

5 years ago
(In reply to Luke Wagner [:luke] from comment #17)
> Ohhh, I missed that.  So the goal of ContextOptions is for it to eventually
> be very small or empty?

Yes. Empty may or may not happen before JSContext goes away entirely from the perspective of embedders.
(Assignee)

Comment 19

5 years ago
Created attachment 804657 [details]
Replace the options bitfield on JSContext with a struct

This patch replaces the options bitfield on JSContext with a struct ContextOptions. This struct can be referenced using JS::Options(cx).

I've made sure the old options API forwards to the new one, so we can upgrade consumers one by one.

Once all consumers have been upgraded to use the new options API, the old API can be removed.
Attachment #804657 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 20

5 years ago
Created attachment 804658 [details] [diff] [review]
Upgrade the shell to use the new options API

This patch upgrades the JS shell to use the new options API.
Attachment #804658 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 21

5 years ago
There is a common pattern that we use where we save the old context options, toggle one or more options, then restore the old context options.

Would it make sense to have a helper struct like this:

...
{
    ContextOptionScope scope(cx); // Store both a copy of and a reference to Options(cx)
    scope->setExtraWarnings(true); // operator-> returns the stored reference to Options(cx)
    scope->setWerror(true);
    ... // Do stuff
}
... // Assign the copy of Options(cx) back to the reference to Options(cx)

Updated

5 years ago
Attachment #803897 - Flags: feedback?(luke) → feedback+
(Assignee)

Comment 22

5 years ago
Comment on attachment 804657 [details]
Replace the options bitfield on JSContext with a struct

This patch had some build errors on Linux. I have a new patch coming up that also adds a AutoSetContextOptions RAII class.
Attachment #804657 - Attachment is obsolete: true
Attachment #804657 - Attachment is patch: false
Attachment #804657 - Flags: review?(bobbyholley+bmo)
(Assignee)

Updated

5 years ago
Attachment #804658 - Attachment is obsolete: true
Attachment #804658 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 23

5 years ago
Created attachment 805399 [details] [diff] [review]
Replace options bitfield in JSContext with a struct

New and improved patch which presumably doesnt break on Linux (Linux doesn't like me shadowing getter functions with variables of the same name).
Attachment #805399 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 24

5 years ago
Created attachment 805403 [details] [diff] [review]
Refactor the shell to use the new options API

I've improved this patch to use the AutoSetContextOptions RAII class I added in the previous patch. This makes it less likely for people to forget to reset the options back to their old values. Although not that useful within the shell code, it makes things easier to read in my opinion, and it will allow us to drop a lot of JS_SetOptions(cx, oldOptions) calls in error handling code in xpconnect.
Attachment #805403 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 25

5 years ago
I need to go for a while, but I have more patches coming up later today.
(Reporter)

Comment 26

5 years ago
Comment on attachment 805399 [details] [diff] [review]
Replace options bitfield in JSContext with a struct

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

Nice. ContextOptions is kinda big, and I'm mildly concerned about the impact on compile times given that jsapi gets included everywhere. But given how much this stuff is in flux, I don't think this is the time to optimize that.

I assume there are followup patches to use AutoSaveContextOptions in the browser?
Attachment #805399 - Flags: review?(bobbyholley+bmo) → review+
(Reporter)

Comment 27

5 years ago
Oh, actually - GetContextOptionsRef should be ContextOptionsRef. r=me with that for comment 26.
(Reporter)

Comment 28

5 years ago
Comment on attachment 805403 [details] [diff] [review]
Refactor the shell to use the new options API

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

This is great cleanup. r=bholley with comments addressed.

::: js/src/shell/js.cpp
@@ +1023,5 @@
> +
> +        {
> +            JS::AutoSaveContextOptions asco(cx);
> +            JS::GetContextOptionsRef(cx).setCompileAndGo(compileAndGo)
> +                                        .setNoScriptRval(noScriptRval);

We're using namespace JS here, so no need for the JS:: prefixing. This happens elsewhere in the patch as well - please fix all of them.

@@ +2013,5 @@
> +
> +    {
> +        JS::AutoSaveContextOptions asco(cx);
> +        JS::GetContextOptionsRef(cx).setCompileAndGo(true)
> +                                 .setNoScriptRval(true);

line up the dots?
Attachment #805403 - Flags: review?(bobbyholley+bmo) → review+
(Assignee)

Comment 29

5 years ago
(In reply to Bobby Holley (:bholley) from comment #28)
> Comment on attachment 805403 [details] [diff] [review]
> Refactor the shell to use the new options API
> 
> Review of attachment 805403 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is great cleanup. r=bholley with comments addressed.
> 
> ::: js/src/shell/js.cpp
> @@ +1023,5 @@
> > +
> > +        {
> > +            JS::AutoSaveContextOptions asco(cx);
> > +            JS::GetContextOptionsRef(cx).setCompileAndGo(compileAndGo)
> > +                                        .setNoScriptRval(noScriptRval);
> 
> We're using namespace JS here, so no need for the JS:: prefixing. This
> happens elsewhere in the patch as well - please fix all of them.
> 

That is deliberate. Removing the JS:: prefix causes clang to complain:

error: use of undeclared identifier 'ContextOptionsRef'; did you mean 'JS::ContextOptionsRef

I don't see any using namespace JS declaration in the file either. Should we add one?

For some strange reason, CompileOptions (which is also defined in namespace JS) seems to work without a prefix, but ContextOptions does not. I can't figure out why, so I gathered it is probably best to leave things like this.
(Assignee)

Comment 30

5 years ago
(In reply to Eddy Bruel [:ejpbruel] from comment #29)
> (In reply to Bobby Holley (:bholley) from comment #28)
> > Comment on attachment 805403 [details] [diff] [review]
> > Refactor the shell to use the new options API
> > 
> > Review of attachment 805403 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This is great cleanup. r=bholley with comments addressed.
> > 
> > ::: js/src/shell/js.cpp
> > @@ +1023,5 @@
> > > +
> > > +        {
> > > +            JS::AutoSaveContextOptions asco(cx);
> > > +            JS::GetContextOptionsRef(cx).setCompileAndGo(compileAndGo)
> > > +                                        .setNoScriptRval(noScriptRval);
> > 
> > We're using namespace JS here, so no need for the JS:: prefixing. This
> > happens elsewhere in the patch as well - please fix all of them.
> > 
> 
> That is deliberate. Removing the JS:: prefix causes clang to complain:
> 
> error: use of undeclared identifier 'ContextOptionsRef'; did you mean
> 'JS::ContextOptionsRef
> 
> I don't see any using namespace JS declaration in the file either. Should we
> add one?
> 
> For some strange reason, CompileOptions (which is also defined in namespace
> JS) seems to work without a prefix, but ContextOptions does not. I can't
> figure out why, so I gathered it is probably best to leave things like this.

Nevermind! I just discovered NamespaceImports.h. Not very obvious that I needed to change that though.
(Assignee)

Comment 31

5 years ago
Created attachment 805924 [details] [diff] [review]
Refactor the jsapi-tests to use the new options API

It turns out testSlowScript.cpp is never actually compiled, and uses option flags that no longer exist, so I haven't bothered to refactor that file. Bug 917241 is the followup for that.
Attachment #805924 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 32

5 years ago
Created attachment 806011 [details] [diff] [review]
Refactor xpconnect to use the new options API
Attachment #806011 - Flags: review?(bobbyholley+bmo)
(Reporter)

Updated

5 years ago
Attachment #805924 - Flags: review?(bobbyholley+bmo) → review+
(Reporter)

Comment 33

5 years ago
Comment on attachment 806011 [details] [diff] [review]
Refactor xpconnect to use the new options API

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

This cleanup is amazing. I can't believe we did it that way in mozJSComponentLoader.

All these files should have |using namespace JS;| - please remove the JS::prefixing.

r=bholley with that.
Attachment #806011 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 803897 [details] [diff] [review]
patch

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

::: js/src/jscntxt.h
@@ +434,5 @@
> +        varObjFix_ = varObjFix;
> +        return *this;
> +    }
> +
> +    bool privateIsNSISupports() const { return privateIsNSISupports_; }

Dunno about the case of "nsISupports" here...
(Assignee)

Comment 35

5 years ago
Created attachment 810596 [details] [diff] [review]
Refactor xpconnect shell to use the new options API

I forgot to update the xpconnect shell as well, so here is a separate patch for that.
Attachment #810596 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 36

5 years ago
Created attachment 810598 [details] [diff] [review]
Refactor dom to use the new options API (1/2)

Storing the default options as a member on a nJSContext makes no sense to me, because we always assign them to the options member of mContext. Whenever we need mDefaultJSOptions, we can just grab ContextOptionsRef(mContext) instead.

Moreover, the way we use mDefaultJSOptions makes it a pain to switch to the new options API. The new API isnt a bitfield but a struct, so we can't conveniently or two option sets together (as we do with 'mDefaultJSOptions |= ::JS_GetOptions(mContext);').

Since mDefaultJSOptions seems to be unnecessary to begin with, I've decided to get rid of it. Here's a patch that does that.

Flagging khuey for review because he's familiar with this code iirc. If somebody else needs to review it please be so kind as to flag that person for me.
Attachment #810598 - Flags: review?(khuey)
(Assignee)

Updated

5 years ago
Attachment #810598 - Attachment description: Refactor dom to use the new options API → Refactor dom to use the new options API (1/2)
(Assignee)

Comment 37

5 years ago
Created attachment 810600 [details] [diff] [review]
Refactor dom to use the new options API (2/2)

With that change in place, refactoring the DOM code to use the new options API is a breeze.

Fwiw, I managed to get a green try run for this stuff, so I don't think I broke any invariants with this refactor.
Attachment #810600 - Flags: review?(khuey)
(Assignee)

Comment 38

5 years ago
(In reply to Eddy Bruel [:ejpbruel] from comment #37)
> Created attachment 810600 [details] [diff] [review]
> Refactor dom to use the new options API (2/2)
> 
> With that change in place, refactoring the DOM code to use the new options
> API is a breeze.
> 
> Fwiw, I managed to get a green try run for this stuff, so I don't think I
> broke any invariants with this refactor.

Oh, I should also mention that I removed two assertions because they relied on bitfield comparisons. We could do that with the new options API by overloading the == and < operator for equality and subset, respectively, but I didn't feel like it was worth the effort just so we could keep two assertions.

If you think we really *should* keep those assertions, just let me know.
(Assignee)

Comment 39

5 years ago
Created attachment 810607 [details] [diff] [review]
Refactor jsd to use the new options API

I've been told sfink is the module owner for jsd, so flagging him for review ;-)

jsdContext unfortunately exposes implementation details from the old options API, by allowing consumers to access the options bitfield directly. For backwards compatibility, I've simulated the old options API in terms of the new one. I didn't bother to copy over the comments because this stuff is (hopefully) going to die some time soon anyway.
Attachment #810607 - Flags: review?(sphink)
(Assignee)

Comment 40

5 years ago
Created attachment 810631 [details] [diff] [review]
Refactor crypto to use the new options API

I had to change a single line in nsCrypto.cpp. I'm not sure who to flag for this. https://wiki.mozilla.org/Modules/All says Honza is a peer for security/manager, so I'm trying him.
Attachment #810631 - Flags: review?(bambas)
Comment on attachment 810607 [details] [diff] [review]
Refactor jsd to use the new options API

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

I checked the firebug source code, and it currently doesn't use any of these options.

Still, a firebug extension could be using it, so maintaining compatibility is still good for now. Thanks!

::: js/jsd/jsd_xpc.cpp
@@ +1667,5 @@
> +           | (JS::ContextOptionsRef(mJSCx).baseline() ? JSOPTION_BASELINE : 0)
> +           | (JS::ContextOptionsRef(mJSCx).typeInference() ? JSOPTION_TYPE_INFERENCE : 0)
> +           | (JS::ContextOptionsRef(mJSCx).strictMode() ? JSOPTION_STRICT_MODE : 0)
> +           | (JS::ContextOptionsRef(mJSCx).ion() ? JSOPTION_ION : 0)
> +           | (JS::ContextOptionsRef(mJSCx).asmJS() ? JSOPTION_ASMJS : 0);

You don't want to do

#define JSOPTION_NO_SCRIPT_RVAL 12
...
   | (JS::ContextOptionsRef(mJSCx).noScriptRval() << JSOPTION_NO_SCRIPT_RVAL)
...
?

(Just kidding. Please don't bother.)

@@ +1701,5 @@
>  NS_IMETHODIMP
>  jsdContext::GetPrivateData(nsISupports **_rval)
>  {
>      ASSERT_VALID_EPHEMERAL;
> +    if (JS::ContextOptionsRef(mJSCx).privateIsNSISupports()) 

Trailing space
Attachment #810607 - Flags: review?(sphink) → review+
(Assignee)

Comment 42

5 years ago
Created attachment 810639 [details] [diff] [review]
Remove the old options API

And with that, we can finally get rid of the old options API.

The next step will be to create a RuntimeOptions struct and migrate most options from ContextOptions to there.
Attachment #810639 - Flags: review?(bobbyholley+bmo)
Comment on attachment 810598 [details] [diff] [review]
Refactor dom to use the new options API (1/2)

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

::: dom/base/nsJSEnvironment.cpp
@@ +735,2 @@
>    else
> +    options &= ~JSOPTION_EXTRA_WARNINGS;

Since you're here, brace these ifs.

@@ +780,2 @@
>    else
> +    options &= ~JSOPTION_TYPE_INFERENCE;

and all of these too.

@@ +859,1 @@
>      // Make sure the new context gets the default context options

You should add a comment that the other options will be set when InitContext is called.
Attachment #810598 - Flags: review?(khuey) → review+
Comment on attachment 810600 [details] [diff] [review]
Refactor dom to use the new options API (2/2)

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

::: dom/base/nsJSEnvironment.cpp
@@ +773,5 @@
>  
> +  JS::ContextOptionsRef(cx).setTypeInference(useTypeInference)
> +                           .setBaseline(useBaselineJIT)
> +                           .setIon(useIon)
> +                           .setAsmJS(useAsmJS);

Or you can just get rid of the ifs entirely.  That works too ;-)

::: dom/base/nsJSUtils.cpp
@@ +209,3 @@
>      }
>    }
>    ~AutoDontReportUncaught() {

Please assert here that DontReportUncaught is set.

::: dom/workers/RuntimeService.cpp
@@ +125,5 @@
>  const uint32_t kNoIndex = uint32_t(-1);
>  
> +const JS::ContextOptions kRequiredJSContextOptions =
> +  JS::ContextOptions().setDontReportUncaught(true)
> +                      .setNoScriptRval(true);

It's unfortunate that this will introduce a static constructor.  Can we just move this into LoadJSContextOptions to avoid that?
Attachment #810600 - Flags: review?(khuey) → review+
(Reporter)

Comment 45

5 years ago
Comment on attachment 810596 [details] [diff] [review]
Refactor xpconnect shell to use the new options API

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

::: js/xpconnect/shell/xpcshell.cpp
@@ +597,4 @@
>  static bool
>  Options(JSContext *cx, unsigned argc, jsval *vp)
>  {
> +    jsval *argv = JS_ARGV(cx, vp);

CallArgsFromVp

@@ +627,5 @@
> +                return false;
> +            }
> +
> +            JS_ReportError(cx, msg);
> +            free(msg);

you can just JS_ReportError(cx, someSprintfFormatString, opt.ptr())

instead of doing all that junk.

@@ +645,5 @@
> +    }
> +    if (!names && oldOptions.strictMode()) {
> +        names = JS_sprintf_append(names, "%s%s", found ? "," : "", "strict_mode");
> +        found = true;
> +    }

What's the point of |found| here? Can't we just test |names|? And moreover, what's the point of appending to the string if we _only_ get here when names is null?

I think this stuff could use a bit more thinking through, and end up drastically simpler than the old code.
Attachment #810596 - Flags: review?(bobbyholley+bmo) → review-
(Reporter)

Comment 46

5 years ago
Comment on attachment 810639 [details] [diff] [review]
Remove the old options API

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

\o/
Attachment #810639 - Flags: review?(bobbyholley+bmo) → review+

Updated

5 years ago
Blocks: 916464
(Assignee)

Comment 51

5 years ago
Created attachment 816590 [details] [diff] [review]
Refactor xpconnect shell to use the new options API
Attachment #810596 - Attachment is obsolete: true
Attachment #816590 - Flags: review?(bobbyholley+bmo)
(Reporter)

Updated

5 years ago
Attachment #816590 - Attachment is patch: true
Attachment #816590 - Attachment mime type: text/x-patch → text/plain
(Reporter)

Comment 52

5 years ago
Comment on attachment 816590 [details] [diff] [review]
Refactor xpconnect shell to use the new options API

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

::: js/xpconnect/src/XPCShellImpl.cpp
@@ +549,4 @@
>  static bool
>  Options(JSContext *cx, unsigned argc, jsval *vp)
>  {
> +    JS::CallArgs args = CallArgsFromVp(argc, vp);

No need for the JS:: prefixing in this file.

@@ +574,4 @@
>          }
>      }
> +
> +    char *names = NULL;

Please add some comments here explaining that this function returns a list of options (in string form) that were previously set.

@@ +579,5 @@
> +        names = JS_sprintf_append(names, "%s", "strict");
> +        if (!names) {
> +            JS_ReportOutOfMemory(cx);
> +            return false;
> +        }

Let's just replace this check and the others below with NS_ENSURE_TRUE(names, false);

It won't report OOM on the cx, but returning null without setting an exception indicates OOM anyway, and I'd rather this code be a little more concise.
Attachment #816590 - Flags: review?(bobbyholley+bmo) → review+

Updated

5 years ago
Depends on: 928736
(Reporter)

Comment 54

5 years ago
Eddy, what is the status here? We're branching this week, and I really don't want this bug to be split across releases.
Flags: needinfo?(ejpbruel)
(Reporter)

Updated

5 years ago
Attachment #810631 - Flags: review?(bambas) → review+
(Assignee)

Comment 61

5 years ago
Please leave this bug open for now. I will rename it and split two follow up bugs off from it. It can be closed after that.

Here's the try run for the patches I just landed:
https://tbpl.mozilla.org/?tree=Try&rev=384d652067a4
(Assignee)

Updated

5 years ago
Summary: Move JIT flags to the compartment → Refactor ContextOptions from a bitfield into a proper struct
(Assignee)

Updated

5 years ago
Blocks: 885786
(Reporter)

Comment 63

5 years ago
Eddy, does this still need to be open?
Flags: needinfo?(ejpbruel)
(Assignee)

Comment 64

5 years ago
(In reply to Bobby Holley (:bholley) from comment #63)
> Eddy, does this still need to be open?

Nope.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: needinfo?(ejpbruel)
Resolution: --- → FIXED
Whiteboard: [leave-open]
Target Milestone: --- → mozilla27

Comment 65

5 years ago
dev-doc-info
Eddy, this needs to be documented:

https://developer.mozilla.org/en-US/docs/SpiderMonkey/31

I just got totally confused searching for some JSOPTION in our codebase to double-check naming of it, and I only happened to learn that the entire existing API had been ripped out.  This is going to be pretty close to the top of the list of things JSAPI users are going to need to know to update, I'm sure!
Flags: needinfo?(ejpbruel)

Updated

4 years ago
No longer blocks: 776798
Depends on: 1000254
(Assignee)

Comment 66

4 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #65)
> Eddy, this needs to be documented:
> 
> https://developer.mozilla.org/en-US/docs/SpiderMonkey/31
> 
> I just got totally confused searching for some JSOPTION in our codebase to
> double-check naming of it, and I only happened to learn that the entire
> existing API had been ripped out.  This is going to be pretty close to the
> top of the list of things JSAPI users are going to need to know to update,
> I'm sure!

I'm no longer working on this, and won't be able to spend time on this any time soon. Is it enough if I file a followup bug for this?
Flags: needinfo?(ejpbruel)
You need to log in before you can comment on or make changes to this bug.