If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

GC: Fix some more rooting issues found by static analysis

RESOLVED FIXED in mozilla21

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

Trunk
mozilla21
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 706453 [details] [diff] [review]
Proposed fix
Attachment #706453 - Flags: review?(sphink)
Comment on attachment 706453 [details] [diff] [review]
Proposed fix

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

::: js/src/jsstr.cpp
@@ +3143,3 @@
>         CallReceiver call)
>  {
> +    RootedLinearString param(cx, paramArg);

I'd rather you switch the param to a Handle. Most of the callers will pass NullPtr() anyway.

::: js/src/jswrapper.cpp
@@ +418,3 @@
>                                                 PropertyDescriptor *desc, unsigned flags)
>  {
> +    RootedObject wrapper(cx, wrapperArg);

How hard would it be to Handle-ify all the params in jswrapper.cpp? They're used in some funky macros, so I couldn't easily tell. If it's a pain, this is fine for now, but I wanted to ask before giving an r+.

::: js/src/vm/ObjectImpl.cpp
@@ +85,3 @@
>                                      PropDesc *unwrapped) const
>  {
> +    RootedObject obj(cx, objArg);

This should definitely be a Handle too. The callers pass a RootedObject, though the declaration is hidden in a macro.

@@ +132,3 @@
>                     PropDesc *desc) const
>  {
> +    RootedObject obj(cx, objArg);

Same. (Same RootedObjects are passed in, even.)
Attachment #706453 - Flags: review?(sphink)
(Assignee)

Comment 3

5 years ago
Created attachment 707088 [details] [diff] [review]
Proposed fix v2

Thanks for the comments.

I didn't Handleify the jswrapper stuff because it would involve changing the whole API.  I've split that part out from this patch and will attempt that separately.
Attachment #706453 - Attachment is obsolete: true
Attachment #707088 - Flags: review?(sphink)
(Assignee)

Comment 4

5 years ago
Created attachment 707095 [details] [diff] [review]
Also root some out params
Attachment #707095 - Flags: review?(sphink)
Attachment #707088 - Flags: review?(sphink) → review+
Attachment #707095 - Flags: review?(sphink) → review+
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e64eba37805
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6367c0f3f2b
https://hg.mozilla.org/mozilla-central/rev/0e64eba37805
https://hg.mozilla.org/mozilla-central/rev/f6367c0f3f2b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.