1152512 - AutoConfig files are not parsed using modern JavaScript

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Mike Kaply [:mkaply]]

AutoConfig files are loaded using JSVERSION_DEFAULT which gives a very old JavaScript parser.

http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/nsXPConnect.cpp#741

This function is only used by autoconfig.

http://mxr.mozilla.org/mozilla-central/ident?i=EvalInSandboxObject

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Switch to using the latest JavaScript version

This accidentally got checked in as part of bug 1001158

Asking for retroactive review.

AutoConfig doesn't really have an owner.

Comment 2

[:Gijs (he/him)]

Comment on attachment 8629993 [details] [diff] [review]
Switch to using the latest JavaScript version

I am very much not the right person to review this. Blake?

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8629993 [details] [diff] [review]
Switch to using the latest JavaScript version

This is inconsistent with what Components.utils.evalInSandbox does with no version parameter, but this is not really used all that much. Please add a comment to nsIXPConnect.idl mentioning that the default version for this case is different than that when calling evalInSandbox from script.

If you wanted to fix the inconsistency by, instead, adding a version parameter to nsIXPConnect::EvalInSandboxObject and passing JSVERSION_LATEST from nsJSConfigTriggers.cpp, that would be cleaner and would gain some amount of clean-code karma, I'm sure.

Comment 4

[Mike Kaply [:mkaply]]

> If you wanted to fix the inconsistency by, instead, adding a version parameter to nsIXPConnect::EvalInSandboxObject and passing JSVERSION_LATEST from nsJSConfigTriggers.cpp, that would be cleaner and would gain some amount of clean-code karma, I'm sure.

That definitely sounds like a much better idea.

Plus it will fix my accidental checkin. I'll do that.

Comment 5

[Mike Kaply [:mkaply]]

Attachment: autoconfig.diff

This patch seems like the right thing to me (except I haven't done the uuid change), but I'm getting this compiler error.

16:21.82 c:/mozilla/inbound/js/xpconnect/src/nsXPConnect.cpp(764) : error C2511: 'nsresult nsXPConnect::EvalInSandboxObj
ect(const nsAString_internal &,const char *,JSContext *,JSObject *,JSVersion,JS::MutableHandleValue)' : overloaded membe
r function not found in 'nsXPConnect'
16:21.84         c:\mozilla\inbound\js\xpconnect\src\xpcprivate.h(243) : see declaration of 'nsXPConnect'

Can you tell me what I'm doing wrong?

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8631543 [details] [diff] [review]
autoconfig.diff

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

::: js/xpconnect/idl/nsIXPConnect.idl
@@ +495,5 @@
>       */
>      [noscript] jsval evalInSandboxObject(in AString source, in string filename,
>                                           in JSContextPtr cx,
> +                                         in JSObjectPtr sandbox,
> +                                         in jsval version);

jsval is for JavaScript values (that is, objects, strings, numbers, etc.) that are manipulated directly by JS code. JSVersion is actually an enum defined at <http://mxr.mozilla.org/mozilla-central/source/js/src/jspubtd.h?rev=ce11a9ce7600#53>. It's probably easiest to take a uint32_t in the idl (and in the .cpp) and comment/assert that callers should pass in a valid JSVersion enum.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #6)
> h?rev=ce11a9ce7600#53>. It's probably easiest to take a uint32_t in the idl

Oops, there's a negative value in that enum. I should have said |int32_t|.

Comment 8

[Mike Kaply [:mkaply]]

Attachment: Add jsversion to evalinsandbox

I went with the comment approach because evalinsandbox does a lot of checking around the jsversion:

http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCComponents.cpp#2468

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8631764 [details] [diff] [review]
Add jsversion to evalinsandbox

This attachment is missing most of the patch :-)

The code that you linked to in comment 8 is only run when this is called directly from JS. The API that you're using is actually <http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/Sandbox.cpp?rev=0e8193deffea#1617>, which doesn't check the JSVersion that gets passed in. You can pretty simply do something like:

#ifdef DEBUG
{
  const char *version = JS_VersionToString(version);
  MOZ_ASSERT(version && strcmp(version, "unknown") != 0, "Illegal JS version passed");
}
#endif

Comment 10

[Mike Kaply [:mkaply]]

Attachment: Let's try this again

OK, here's the correct patch. I had attached the old patch.

I've tested and verified it works.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8632076 [details] [diff] [review]
Let's try this again

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

Looks good. Thanks for taking care of this!

Comment 12

[Mike Kaply [:mkaply]]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/0afe24f16cf44ed53ee5aac3fa1de1982f8aa746
changeset:  0afe24f16cf44ed53ee5aac3fa1de1982f8aa746
user:       Mike Kaply <[e-mail]>
date:       Tue Jul 28 07:13:26 2015 -0500
description:
Bug 1152512 - Use the latest JavaScript version when parsing AutoConfig files. r=mrbkap

Comment 13

[Carsten Book [:Tomcat]]

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=12192526&repo=mozilla-inbound

Comment 14

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68e68e7643e4

Comment 15

[Mike Kaply [:mkaply]]

so unfortunately my local build wasn't debug, so I didn't see the various problems with this:

const char *version = JS_VersionToString(version);

1. I got the name wrong (jsVersion).

2. Even if I fix that:

 0:56.17 c:/mozilla/inbound/js/xpconnect/src/nsXPConnect.cpp(778) : error C2664: 'const char *JS_VersionToString(JSVersi
on)' : cannot convert argument 1 from 'int32_t' to 'JSVersion'
 0:56.17         Conversion to enumeration type requires an explicit cast (static_cast, C-style cast or function-style c
ast)

Today I'm going to learn about try servers :)

Comment 16

[Mike Kaply [:mkaply]]

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

Comment 17

[Mike Kaply [:mkaply]]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/edc5fdb6ea5bd6e4fdab884c5edc8f3448b84890
changeset:  edc5fdb6ea5bd6e4fdab884c5edc8f3448b84890
user:       Mike Kaply <[e-mail]>
date:       Tue Jul 28 13:57:55 2015 -0500
description:
Bug 1152512 - Use the latest JavaScript version when parsing AutoConfig files. r=mrbkap

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/edc5fdb6ea5b