Fix various rooting hazards

RESOLVED FIXED in mozilla23

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Tracking

unspecified
mozilla23
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments)

No description provided.
Attachment #743963 - Flags: review?(terrence)
Posted patch Root toolkit/Splinter Review
Attachment #743982 - Flags: review?(terrence)
Comment on attachment 743982 [details] [diff] [review]
Root toolkit/

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

Looks good.
Attachment #743982 - Flags: review?(terrence) → review+
Comment on attachment 743963 [details] [diff] [review]
Root xpcshell.cpp

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

Excellent.
Attachment #743963 - Flags: review?(terrence) → review+
Attachment #744401 - Flags: review?(terrence)
Posted patch webrtcSplinter Review
Attachment #744418 - Flags: review?(terrence)
Posted patch capsSplinter Review
Attachment #744421 - Flags: review?(terrence)
Posted patch gfxSplinter Review
Attachment #744423 - Flags: review?(terrence)
Posted patch storageSplinter Review
Attachment #744425 - Flags: review?(terrence)
Posted patch xpfeSplinter Review
Attachment #744426 - Flags: review?(terrence)
Attachment #744401 - Flags: review?(terrence) → review+
Attachment #744418 - Flags: review?(terrence) → review+
Comment on attachment 744421 [details] [diff] [review]
caps

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

r=me

::: caps/src/nsScriptSecurityManager.cpp
@@ +1590,5 @@
>      {
>  #ifdef DEBUG
>          {
> +            JS_ASSERT(JS_ObjectIsFunction(aCx, rootedFunObj));
> +            JSFunction *fun = JS_GetObjectFunction(rootedFunObj);

|fun| should probably be rooted here too, even if it doesn't matter in practice at this site: if it is done down below, it will look odd if it isn't done here too.
Attachment #744421 - Flags: review?(terrence) → review+
Comment on attachment 744423 [details] [diff] [review]
gfx

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

r=me
Attachment #744423 - Flags: review?(terrence) → review+
Comment on attachment 744425 [details] [diff] [review]
storage

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

r=me

::: storage/src/mozStorageAsyncStatementJSHelper.cpp
@@ +54,5 @@
>      );
>      NS_ENSURE_SUCCESS(rv, rv);
>    }
>  
> +  JS::Rooted<JSObject*> obj(aCtx, nullptr);

I think the preferred browser style is to leave off explicit null initialization as it happens automatically now in all cases.

::: storage/src/mozStorageStatementJSHelper.cpp
@@ +107,5 @@
>      );
>      NS_ENSURE_SUCCESS(rv, rv);
>    }
>  
> +  JS::Rooted<JSObject*> obj(aCtx, nullptr);

ditto

@@ +146,5 @@
>      );
>      NS_ENSURE_SUCCESS(rv, rv);
>    }
>  
> +  JS::Rooted<JSObject*> obj(aCtx, nullptr);

ditto
Attachment #744425 - Flags: review?(terrence) → review+
Comment on attachment 744426 [details] [diff] [review]
xpfe

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

r=me

Thanks for the help with our long tail of rooting hazards! These are by far the scariest hazards for us to deal with and we greatly appreciate your help looking into them.
Attachment #744426 - Flags: review?(terrence) → review+
Posted patch ipcSplinter Review
Attachment #745324 - Flags: review?(terrence)
Posted patch netwerkSplinter Review
Attachment #745335 - Flags: review?(terrence)
Comment on attachment 745324 [details] [diff] [review]
ipc

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

r=me
Attachment #745324 - Flags: review?(terrence) → review+
Comment on attachment 745335 [details] [diff] [review]
netwerk

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

r=me
Attachment #745335 - Flags: review?(terrence) → review+
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.