Closed
Bug 833396
Opened 11 years ago
Closed 11 years ago
Fix some rooting issues found by static analysis
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: jonco, Assigned: jonco)
References
Details
Attachments
(1 file, 1 obsolete file)
32.45 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #704923 -
Flags: review?(sphink)
Comment 2•11 years ago
|
||
Comment on attachment 704923 [details] [diff] [review] Fix Review of attachment 704923 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but I'd like to skim it one more time after the Handlification changes. ::: js/src/jsdbgapi.cpp @@ +931,5 @@ > + if (!js_AddRoot(cx, &pd[i].id, NULL)) > + goto bad; > + if (!js_AddRoot(cx, &pd[i].value, NULL)) > + goto bad; > + RootedShape shape(cx, const_cast<Shape *>(&r.front())); Hoist this declaration out of the loop, to save a push/pop on every iteration. ::: js/src/jsobj.cpp @@ +1639,3 @@ > { > + RootedObject from(cx, fromArg); > + RootedObject to(cx, toArg); Why root here? CopySlots should take Handles. In fact, you're already rooting the only unrooted thing that gets passed into it, a little later. @@ +1730,4 @@ > TradeGutsReserved &reserved) > { > + RootedObject a(cx, aArg); > + RootedObject b(cx, bArg); These are rooted again a little later. You just need to pull these roots up above the AutoCompartment (it's what can gc.) ::: js/src/json.cpp @@ +328,5 @@ > > /* Step 4. */ > if (vp.get().isObject()) { > + RootedObject obj(cx, &vp.get().toObject()); > + if (ObjectClassIs(*obj, ESClass_Number, cx)) { I could look it up, but I'm lazy: what's the rooting hazard here? ::: js/src/jsopcode.cpp @@ +4764,5 @@ > sn = js_GetSrcNote(cx, jp->script, pc); > if (sn && SN_TYPE(sn) == SRC_GENEXP) { > BindingVector *outerLocalNames; > + JSScript *inner; > + RootedScript outer(cx); This file is a very good demonstration of why the static analysis is good. :-) ::: js/src/jsscope.cpp @@ +734,5 @@ > return shape; > } > > /* static */ UnrootedShape > +JSObject::changeProperty(JSContext *cx, HandleObject obj, RawShape shapeArg, unsigned attrs, unsigned mask, I did a quick scan, and it looks like using a HandleShape for ::changeProperty and ::changePropertyAttributes would be straightforward.
Attachment #704923 -
Flags: review?(sphink)
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #704923 -
Attachment is obsolete: true
Attachment #705360 -
Flags: review?(sphink)
Comment 4•11 years ago
|
||
Comment on attachment 705360 [details] [diff] [review] Fix with review comments addressed Review of attachment 705360 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsscope.cpp @@ +740,2 @@ > { > + RootedShape shape(cx, shapeArg); Seems like an unnecessary root. AFAICT, shape is never assigned, so why not use the HandleShape param directly?
Attachment #705360 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 5•11 years ago
|
||
(In reply to Steve Fink [:sfink] from comment #4) > Seems like an unnecessary root. AFAICT, shape is never assigned, so why not > use the HandleShape param directly? Yes, that's better, I must have missed that. Cheers!
Assignee | ||
Comment 6•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7227a6cb2e2e
Comment 7•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7227a6cb2e2e
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in
before you can comment on or make changes to this bug.
Description
•