867459 - Fix various rooting hazards

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[David Zbarsky (:dzbarsky)]

Attachment: Root xpcshell.cpp

Comment 1

[David Zbarsky (:dzbarsky)]

Attachment: Root toolkit/

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 743982 [details] [diff] [review]
Root toolkit/

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

Looks good.

Comment 3

[Terrence Cole [:terrence]]

Comment on attachment 743963 [details] [diff] [review]
Root xpcshell.cpp

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

Excellent.

Comment 4

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e1fe36ce89d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b1b9e61db7b

Comment 5

[David Zbarsky (:dzbarsky)]

Attachment: More toolkit fixes

Comment 6

[David Zbarsky (:dzbarsky)]

Attachment: webrtc

Comment 7

[David Zbarsky (:dzbarsky)]

Attachment: caps

Comment 8

[David Zbarsky (:dzbarsky)]

Attachment: gfx

Comment 9

[David Zbarsky (:dzbarsky)]

Attachment: storage

Comment 10

[David Zbarsky (:dzbarsky)]

Attachment: xpfe

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e1fe36ce89d1
https://hg.mozilla.org/mozilla-central/rev/9b1b9e61db7b

Comment 12

[Terrence Cole [:terrence]]

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.

Comment 13

[Terrence Cole [:terrence]]

Comment on attachment 744423 [details] [diff] [review]
gfx

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

r=me

Comment 14

[Terrence Cole [:terrence]]

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

Comment 15

[Terrence Cole [:terrence]]

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.

Comment 16

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8786f0e0226
https://hg.mozilla.org/integration/mozilla-inbound/rev/de74752f65e7
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ecd038b7b0d
https://hg.mozilla.org/integration/mozilla-inbound/rev/164a107f2202
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec1fb05b0095
https://hg.mozilla.org/integration/mozilla-inbound/rev/354940bc7ff8

Comment 17

[David Zbarsky (:dzbarsky)]

Bustage fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/c0f1553c1476

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b8786f0e0226
https://hg.mozilla.org/mozilla-central/rev/de74752f65e7
https://hg.mozilla.org/mozilla-central/rev/7ecd038b7b0d
https://hg.mozilla.org/mozilla-central/rev/164a107f2202
https://hg.mozilla.org/mozilla-central/rev/ec1fb05b0095
https://hg.mozilla.org/mozilla-central/rev/354940bc7ff8
https://hg.mozilla.org/mozilla-central/rev/c0f1553c1476

Comment 19

[David Zbarsky (:dzbarsky)]

Attachment: ipc

Comment 20

[David Zbarsky (:dzbarsky)]

Attachment: netwerk

Comment 21

[Terrence Cole [:terrence]]

Comment on attachment 745324 [details] [diff] [review]
ipc

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

r=me

Comment 22

[Terrence Cole [:terrence]]

Comment on attachment 745335 [details] [diff] [review]
netwerk

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

r=me

Comment 23

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb47a4845d4f
https://hg.mozilla.org/integration/mozilla-inbound/rev/b97f5f39fd8b

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/eb47a4845d4f
https://hg.mozilla.org/mozilla-central/rev/b97f5f39fd8b