The default bug view has changed. See this FAQ.

AutoConfig files are not parsed using modern JavaScript

RESOLVED FIXED in Firefox 42

Status

()

Core
XPConnect
RESOLVED FIXED
2 years ago
a month ago

People

(Reporter: mkaply, Assigned: mkaply)

Tracking

Trunk
mozilla42
Points:
---

Firefox Tracking Flags

(firefox40 affected, firefox42 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

2 years ago
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
(Assignee)

Comment 1

2 years ago
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.
Assignee: nobody → mozilla
Attachment #8629993 - Flags: review?(gijskruitbosch+bugs)

Updated

2 years ago
Component: Preferences → XPConnect
Product: Toolkit → Core

Comment 2

2 years ago
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?
Attachment #8629993 - Flags: review?(gijskruitbosch+bugs) → review?(mrbkap)
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.
Attachment #8629993 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 4

2 years ago
> 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.
(Assignee)

Comment 5

2 years ago
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?
Flags: needinfo?(mrbkap)
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.

Updated

2 years ago
Flags: needinfo?(mrbkap)
(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|.
(Assignee)

Comment 8

2 years ago
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
Attachment #8629993 - Attachment is obsolete: true
Attachment #8631543 - Attachment is obsolete: true
Attachment #8631764 - Flags: review?(mrbkap)
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
Attachment #8631764 - Flags: review?(mrbkap)
(Assignee)

Comment 10

2 years ago
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.
Attachment #8631764 - Attachment is obsolete: true
Attachment #8632076 - Flags: review?(mrbkap)
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!
Attachment #8632076 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 12

2 years ago
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
sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=12192526&repo=mozilla-inbound

Comment 14

2 years ago
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68e68e7643e4
(Assignee)

Comment 15

2 years ago
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 :)
(Assignee)

Comment 16

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4395453dae28
(Assignee)

Comment 17

2 years ago
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
https://hg.mozilla.org/mozilla-central/rev/edc5fdb6ea5b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Depends on: 1338971
You need to log in before you can comment on or make changes to this bug.