876639 - Address unsafe references from JS_ParseJSON()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

JS_ParseJSON is producing a bunch of these warnings.

Convert the function to take a MutableHandleValue for its out parameter, and update callers.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Fix the shell

Make JS_ParseJSON() take a MutableHandleValue and update the shell.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Fix the browser

Update references to JS_ParseJSON() in the browser to pass a handle.

Comment 3

[Jon Coppeard (:jonco)]

Could you suggest reviewer(s) for the second patch?

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 754777 [details] [diff] [review]
Fix the shell

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

::: js/src/jsapi.cpp
@@ +6366,1 @@
>      return true;

Can this just be

return ParseJSONWithReviver(cx, JS::StableCharPtr(chars, len), len, reviver, value);

?

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 754777 [details] [diff] [review]
Fix the shell

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

r=me

::: js/src/jsapi.cpp
@@ +6366,1 @@
>      return true;

It also still needs the AssertHeapIsIdle and CHECK_REQUEST, but what Msger said (assuming he meant to use |vp| directly there).

Comment 6

[Terrence Cole [:terrence]]

Comment on attachment 754777 [details] [diff] [review]
Fix the shell

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

r=me

::: js/src/jsapi.cpp
@@ +6366,1 @@
>      return true;

It also still needs the AssertHeapIsIdle and CHECK_REQUEST, but what Msger said (assuming he meant to use |vp| directly there).

::: js/src/jsapi.h
@@ +4477,5 @@
>  /*
>   * JSON.parse as specified by ES5.
>   */
>  JS_PUBLIC_API(JSBool)
> +JS_ParseJSON(JSContext *cx, const jschar *chars, uint32_t len, JSMutableHandleValue vp);

One other thing occurred to me: we should probably use the external names here so that jsapi.h can serve as a useful example for embedders. So: JS::MutableHandle<JS::Value>.

In either case, we should definitely try to kill off JSMutableHandleFoo and use JS::MutableHandleFoo in all places where it comes up.

Comment 7

[Jon Coppeard (:jonco)]

Updated with review comments and landed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/82024a9a3dfd
https://hg.mozilla.org/integration/mozilla-inbound/rev/7acdb79e4d71

Comment 8

[Ed Morley [:emorley]]

Backed out for B2G breakage:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=7acdb79e4d71

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/94e4cfd71460
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/036890bc3788

Comment 9

[Jon Coppeard (:jonco)]

Fixed B2G issue and re-pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd794d7e4fc1
https://hg.mozilla.org/integration/mozilla-inbound/rev/681e65149dd0

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fd794d7e4fc1
https://hg.mozilla.org/mozilla-central/rev/681e65149dd0