Closed
Bug 961490
Opened 11 years ago
Closed 11 years ago
More exact rooting in the browser
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: evilpies, Assigned: evilpies)
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•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → evilpies
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Assignee | ||
Comment 2•11 years ago
|
||
Assignee | ||
Comment 3•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8362241 -
Flags: review?(terrence)
Assignee | ||
Updated•11 years ago
|
Attachment #8362244 -
Flags: review?(terrence)
Assignee | ||
Updated•11 years ago
|
Attachment #8362245 -
Flags: review?(continuation)
Assignee | ||
Updated•11 years ago
|
Attachment #8362246 -
Flags: review?(benjamin)
Comment 4•11 years ago
|
||
What's the reason for handlifying this code?
Comment 5•11 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•11 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•11 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•11 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•11 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•11 years ago
|
Attachment #8362241 -
Flags: review?(bugs)
Updated•11 years ago
|
Attachment #8362244 -
Flags: review?(khuey)
Comment 10•11 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•11 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•11 years ago
|
Attachment #8362241 -
Flags: review?(bugs) → review+
Comment 12•11 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•11 years ago
|
Attachment #8362246 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 14•11 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: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in
before you can comment on or make changes to this bug.
Description
•