961490 - More exact rooting in the browser

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S. (please needinfo tschuster)]

Attachment: nsEventListenerService

Comment 1

[Tom S. (please needinfo tschuster)]

Attachment: dom/indexedDB

Comment 2

[Tom S. (please needinfo tschuster)]

Attachment: dom/file/

Comment 3

[Tom S. (please needinfo tschuster)]

Attachment: NPAPI

Comment 4

[Andrew McCreight [:mccr8]]

What's the reason for handlifying this code?

Comment 5

[Andrew McCreight [:mccr8]]

You should also get a DOM and IDB peer to at least glance at the first three patches, as appropriate.

Comment 6

[Tom S. (please needinfo tschuster)]

> What's the reason for handlifying this code?
In general, almost all code should use handles these days. Especially if it's not performance critical or outside the SpiderMonkey.
> You should also get a DOM and IDB peer to at least glance at the first three patches, as appropriate.
Ok.

Comment 7

[Terrence Cole [:terrence]]

Comment on attachment 8362241 [details] [diff] [review]
nsEventListenerService

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

Awesome! r=me

::: dom/events/nsEventListenerService.cpp
@@ -74,5 @@
>  }
>  
>  NS_IMPL_ISUPPORTS1(nsEventListenerService, nsIEventListenerService)
>  
> -// Caller must root *aJSVal!

Replacing a comment with a compile-time type assertion == win.

Comment 8

[Terrence Cole [:terrence]]

Comment on attachment 8362244 [details] [diff] [review]
dom/indexedDB

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

Nice! r=me

Comment 9

[Andrew McCreight [:mccr8]]

Comment on attachment 8362245 [details] [diff] [review]
dom/file/

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

bz points out that the analysis doesn't cover every build configuration we ship, so that sounds like a compelling reason to me.

This looks fine to me.  janv should also review, as he is a peer of this code.

::: dom/file/LockedFile.cpp
@@ -1062,3 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> -  JS::Rooted<JS::Value> rval(aCx);

It is nice to get rid of these Rooteds!

Comment 10

[Jan Varga [:janv]]

Comment on attachment 8362245 [details] [diff] [review]
dom/file/

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

Thanks for doing this, extra bonus for the MOZ_OVERRIDEs !

Comment 11

[Tom S. (please needinfo tschuster)]

Comment on attachment 8362246 [details] [diff] [review]
NPAPI

Actually there is something I just realized, this won't root anything if we decide to disable exact rooting again. Is that a problem or when can we start to expect it?

Comment 12

[Terrence Cole [:terrence]]

Comment on attachment 8362246 [details] [diff] [review]
NPAPI

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

(In reply to Tom Schuster [:evilpie] from comment #11)
> Comment on attachment 8362246 [details] [diff] [review]
> NPAPI
> 
> Actually there is something I just realized, this won't root anything if we
> decide to disable exact rooting again. Is that a problem or when can we
> start to expect it?

The current plan is to keep the conservative scanner around until after the next ESR uplift; at least 3 different embedders have independently requested this, so I think it's a reasonable burden.

That said, what here would not work with the conservative scanner? All of it seems okay to me under both configurations, but maybe I'm missing something? r=me in the assumption that NPAPI isn't actively sabotaging itself some other way.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8362244 [details] [diff] [review]
dom/indexedDB

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

::: dom/indexedDB/IDBKeyRange.cpp
@@ +27,5 @@
>  
>  namespace {
>  
>  inline nsresult
> +GetKeyFromJSVal(JSContext* aCx, JS::Handle<JS::Value> aVal, Key& aKey,

I would prefer that you did not collapse the lines here.

Comment 14

[Tom S. (please needinfo tschuster)]

Thanks guys for the quick reviews!
try: https://tbpl.mozilla.org/?tree=Try&rev=f1691fc80327 (bc orange was an unrelated patch)

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7c3b5796373b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2a93f0989941
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4e559e11306b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/16805a1975cd

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7c3b5796373b
https://hg.mozilla.org/mozilla-central/rev/2a93f0989941
https://hg.mozilla.org/mozilla-central/rev/4e559e11306b
https://hg.mozilla.org/mozilla-central/rev/16805a1975cd