Closed
Bug 961490
Opened 10 years ago
Closed 10 years ago
More exact rooting in the browser
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: evilpie, Assigned: evilpie)
References
Details
Attachments
(4 files)
3.65 KB,
patch
|
terrence
:
review+
smaug
:
review+
|
Details | Diff | Splinter Review |
7.18 KB,
patch
|
terrence
:
review+
khuey
:
review+
|
Details | Diff | Splinter Review |
11.62 KB,
patch
|
mccr8
:
review+
janv
:
review+
|
Details | Diff | Splinter Review |
1.80 KB,
patch
|
benjamin
:
review+
terrence
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → evilpies
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8362241 -
Flags: review?(terrence)
Assignee | ||
Updated•10 years ago
|
Attachment #8362244 -
Flags: review?(terrence)
Assignee | ||
Updated•10 years ago
|
Attachment #8362245 -
Flags: review?(continuation)
Assignee | ||
Updated•10 years ago
|
Attachment #8362246 -
Flags: review?(benjamin)
Comment 4•10 years ago
|
||
What's the reason for handlifying this code?
Comment 5•10 years ago
|
||
You should also get a DOM and IDB peer to at least glance at the first three patches, as appropriate.
Assignee | ||
Comment 6•10 years ago
|
||
> 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•10 years ago
|
||
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 8•10 years ago
|
||
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 9•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8362241 -
Flags: review?(bugs)
Updated•10 years ago
|
Attachment #8362244 -
Flags: review?(khuey)
Comment 10•10 years ago
|
||
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+
Assignee | ||
Comment 11•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8362241 -
Flags: review?(bugs) → review+
Comment 12•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8362246 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 14•10 years ago
|
||
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
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in
before you can comment on or make changes to this bug.
Description
•