Last Comment Bug 1152512 - AutoConfig files are not parsed using modern JavaScript
: AutoConfig files are not parsed using modern JavaScript
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla42
Assigned To: Mike Kaply [:mkaply]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 1338971
Blocks:
  Show dependency treegraph
 
Reported: 2015-04-08 13:16 PDT by Mike Kaply [:mkaply]
Modified: 2017-02-12 20:09 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
fixed


Attachments
Switch to using the latest JavaScript version (1.04 KB, patch)
2015-07-06 08:42 PDT, Mike Kaply [:mkaply]
mrbkap: review+
Details | Diff | Splinter Review
autoconfig.diff (3.65 KB, patch)
2015-07-09 05:47 PDT, Mike Kaply [:mkaply]
no flags Details | Diff | Splinter Review
Add jsversion to evalinsandbox (1.04 KB, patch)
2015-07-09 12:33 PDT, Mike Kaply [:mkaply]
no flags Details | Diff | Splinter Review
Let's try this again (4.47 KB, patch)
2015-07-10 05:20 PDT, Mike Kaply [:mkaply]
mrbkap: review+
Details | Diff | Splinter Review

Description User image Mike Kaply [:mkaply] 2015-04-08 13:16:12 PDT
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 User image Mike Kaply [:mkaply] 2015-07-06 08:42:51 PDT
Created attachment 8629993 [details] [diff] [review]
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 User image :Gijs 2015-07-06 08:44:58 PDT
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 User image Blake Kaplan (:mrbkap) 2015-07-06 14:48:53 PDT
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 User image Mike Kaply [:mkaply] 2015-07-06 15:04:01 PDT
> 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 User image Mike Kaply [:mkaply] 2015-07-09 05:47:53 PDT
Created attachment 8631543 [details] [diff] [review]
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 User image Blake Kaplan (:mrbkap) 2015-07-09 09:03:50 PDT
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 User image Blake Kaplan (:mrbkap) 2015-07-09 10:49:01 PDT
(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 User image Mike Kaply [:mkaply] 2015-07-09 12:33:23 PDT
Created attachment 8631764 [details] [diff] [review]
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 User image Blake Kaplan (:mrbkap) 2015-07-09 14:28:45 PDT
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 User image Mike Kaply [:mkaply] 2015-07-10 05:20:51 PDT
Created attachment 8632076 [details] [diff] [review]
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 User image Blake Kaplan (:mrbkap) 2015-07-14 15:46:33 PDT
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 User image Mike Kaply [:mkaply] 2015-07-28 05:14:01 PDT
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/0afe24f16cf44ed53ee5aac3fa1de1982f8aa746
changeset:  0afe24f16cf44ed53ee5aac3fa1de1982f8aa746
user:       Mike Kaply <mozilla@kaply.com>
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 User image Carsten Book [:Tomcat] 2015-07-28 05:39:40 PDT
sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=12192526&repo=mozilla-inbound
Comment 15 User image Mike Kaply [:mkaply] 2015-07-28 09:25:29 PDT
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 17 User image Mike Kaply [:mkaply] 2015-07-29 06:22:33 PDT
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/edc5fdb6ea5bd6e4fdab884c5edc8f3448b84890
changeset:  edc5fdb6ea5bd6e4fdab884c5edc8f3448b84890
user:       Mike Kaply <mozilla@kaply.com>
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 User image Ryan VanderMeulen [:RyanVM] 2015-07-29 11:00:26 PDT
https://hg.mozilla.org/mozilla-central/rev/edc5fdb6ea5b

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