Closed Bug 961490 Opened 11 years ago Closed 11 years ago

More exact rooting in the browser

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: evilpies, Assigned: evilpies)

References

Details

Attachments

(4 files)

No description provided.
Assignee: nobody → evilpies
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Attached patch dom/file/Splinter Review
Attached 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+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: