More exact rooting in the browser

RESOLVED FIXED in mozilla29

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

unspecified
mozilla29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

No description provided.
Posted patch dom/indexedDBSplinter Review
Assignee: nobody → evilpies
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Posted patch dom/file/Splinter Review
Posted patch NPAPISplinter Review
Attachment #8362241 - Flags: review?(terrence)
Attachment #8362244 - Flags: review?(terrence)
Attachment #8362245 - Flags: review?(continuation)
Attachment #8362246 - Flags: review?(benjamin)
What's the reason for handlifying this code?
You should also get a DOM and IDB peer to at least glance at the first three patches, as appropriate.
> 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 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.
Attachment #8362241 - Flags: review?(terrence) → review+
Comment on attachment 8362244 [details] [diff] [review]
dom/indexedDB

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

Nice! r=me
Attachment #8362244 - Flags: review?(terrence) → review+
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!
Attachment #8362245 - Flags: review?(continuation)
Attachment #8362245 - Flags: review?(Jan.Varga)
Attachment #8362245 - Flags: review+
Attachment #8362241 - Flags: review?(bugs)
Attachment #8362244 - Flags: review?(khuey)
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 !
Attachment #8362245 - Flags: review?(Jan.Varga) → review+
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?
Attachment #8362246 - Flags: review?(terrence)
Attachment #8362241 - Flags: review?(bugs) → review+
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.
Attachment #8362246 - Flags: review?(terrence) → review+
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.
Attachment #8362244 - Flags: review?(khuey) → review+
Attachment #8362246 - Flags: review?(benjamin) → review+
Duplicate of this bug: 958259
You need to log in before you can comment on or make changes to this bug.