Closed
Bug 1152512
Opened 9 years ago
Closed 9 years ago
AutoConfig files are not parsed using modern JavaScript
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla42
People
(Reporter: mkaply, Assigned: mkaply)
References
Details
Attachments
(1 file, 3 obsolete files)
4.47 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
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•9 years ago
|
Component: Preferences → XPConnect
Product: Toolkit → Core
Comment 2•9 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 3•9 years ago
|
||
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•9 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•9 years ago
|
||
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 6•9 years ago
|
||
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•9 years ago
|
Flags: needinfo?(mrbkap)
Comment 7•9 years ago
|
||
(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•9 years ago
|
||
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 9•9 years ago
|
||
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•9 years ago
|
||
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 11•9 years ago
|
||
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•9 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
Comment 13•9 years ago
|
||
sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=12192526&repo=mozilla-inbound
Comment 14•9 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/68e68e7643e4
Assignee | ||
Comment 15•9 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•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4395453dae28
Assignee | ||
Comment 17•9 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
Comment 18•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/edc5fdb6ea5b
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in
before you can comment on or make changes to this bug.
Description
•