Fix remaining browser rooting hazards

RESOLVED FIXED in mozilla24

Status

()

defect
RESOLVED FIXED
6 years ago
3 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

unspecified
mozilla24
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Except for the false positives, of course.
Whiteboard: [need review]
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [need review] → [need patch]
Whiteboard: [need patch] → [need review]
Can you give me some context as to what this patch is about?
Sure.  There was a rooting hazard that looked like this:

  JSObject* obj;
  some_holder->GetJSObject(&obj);

It's a hazard from the pov of the static analysis because it has no way to tell whether nsIXPConnectJSObjectHolder::GetJSObject can gc, virtual function calls being what they are.

The standard way to fix that is to make the JSObject* into a Rooted<JSObject*>, but that requires a JSContext*, and the relevant code does not have one.  And the typical ways of getting one can in fact GC, so are not great here.

But the fact that nsIXPConnectJSObjectHolder::GetJSObject() uses an outparam to start with is just because it's declared in xpidl and that's the path of least resistance in xpidl.  None of the implementations do any interesting throwing.

So the patch just changes the XPIDL so that the C++ signature becomes:

  virtual JSObject* GetJSObject();

and then fixes all the consumers.
Comment on attachment 751313 [details] [diff] [review]
part 2.  Change nsIXPConnectJSObjectHolder::GetJSObject to return a JSObject*.

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

r=bholley

::: dom/base/nsDOMClassInfo.cpp
@@ +4136,5 @@
>                                            aProto);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    JS::Rooted<JSObject*> proto_obj(cx);
> +  proto_obj = (*aProto)->GetJSObject();

Fold this into the previous line?

@@ +5938,3 @@
>        } else {
>          realObj = obj;
>        }

Maybe fold this all into one ternary like in the above.

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +567,1 @@
>              if (xpc::AccessCheck::isChrome(js::GetObjectCompartment(obj)) &&

Is |obj| isn't needed here at all? We could just pass it directly to js::GetObjectCompartment, though it's not clear to me whether that will eventually need to take a handle.

::: security/manager/ssl/src/nsCrypto.cpp
@@ +1921,5 @@
>                          NS_GET_IID(nsIDOMCrypto), getter_AddRefs(holder));
>    NS_ENSURE_SUCCESS(nrv, nrv);
>  
> +  script_obj = holder->GetJSObject();
> +  NS_ENSURE_STATE(script_obj);

Maybe move the declaration of script_obj down here?
Attachment #751313 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 751312 [details] [diff] [review]
part 1.  Fix the more or less mechanical browser rooting hazards.

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

\o/ r=me

::: content/xul/content/src/nsXULElement.h
@@ +235,5 @@
>      void UnlinkJSObjects();
>  
>      void Set(JSScript* aObject);
>  
> +    JS::Handle<JSScript*> GetScriptObject()

I guess these (and transitive references) are tied strongly to the lifetime of the script and XULElements cannot be stored in realloc'd or memmoved memory? I guess this will explode pretty thoroughly if that's not true in practice. A comment would still be nice here though, if just to give people the idea that returning Handles isn't safe in general.
Attachment #751312 - Flags: review?(terrence) → review+
> I guess these (and transitive references) are tied strongly to the lifetime of the script
> and XULElements cannot be stored in realloc'd or memmoved memory? 

XULPrototypeScripts, but yes.  I'll add a comment explaining why this is safe.
> Fold this into the previous line?

Done.

> Maybe fold this all into one ternary like in the above.

Done.

> Is |obj| isn't needed here at all?

No.  GetObjectCompartment is unlikely to ever need a handle; if it does we can worry about that then.  Got rid of the temporary.

> Maybe move the declaration of script_obj down here?

Done.  

Thank you for the very fast reviews!  Pushed:

  https://hg.mozilla.org/integration/mozilla-inbound/rev/6bc3ac8a681e
  https://hg.mozilla.org/integration/mozilla-inbound/rev/908f7d92b2ac
Flags: in-testsuite-
Whiteboard: [need review]
Target Milestone: --- → mozilla24
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.