Closed
Bug 873735
Opened 11 years ago
Closed 11 years ago
Fix remaining browser rooting hazards
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(2 files)
30.28 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
61.92 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
Except for the false positives, of course.
Assignee | ||
Updated•11 years ago
|
Whiteboard: [need review]
Updated•11 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [need review] → [need patch]
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #751312 -
Flags: review?(terrence)
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #751313 -
Flags: review?(bobbyholley+bmo)
Assignee | ||
Updated•11 years ago
|
Whiteboard: [need patch] → [need review]
Comment 3•11 years ago
|
||
Can you give me some context as to what this patch is about?
Assignee | ||
Comment 4•11 years ago
|
||
Sure. There was a rooting hazard that looked like this: JSObject* obj; some_holder->GetJSObject(&obj); It's a hazard from the pov of the static analysis because it has no way to tell whether nsIXPConnectJSObjectHolder::GetJSObject can gc, virtual function calls being what they are. The standard way to fix that is to make the JSObject* into a Rooted<JSObject*>, but that requires a JSContext*, and the relevant code does not have one. And the typical ways of getting one can in fact GC, so are not great here. But the fact that nsIXPConnectJSObjectHolder::GetJSObject() uses an outparam to start with is just because it's declared in xpidl and that's the path of least resistance in xpidl. None of the implementations do any interesting throwing. So the patch just changes the XPIDL so that the C++ signature becomes: virtual JSObject* GetJSObject(); and then fixes all the consumers.
Comment 5•11 years ago
|
||
Comment on attachment 751313 [details] [diff] [review] part 2. Change nsIXPConnectJSObjectHolder::GetJSObject to return a JSObject*. Review of attachment 751313 [details] [diff] [review]: ----------------------------------------------------------------- r=bholley ::: dom/base/nsDOMClassInfo.cpp @@ +4136,5 @@ > aProto); > NS_ENSURE_SUCCESS(rv, rv); > > JS::Rooted<JSObject*> proto_obj(cx); > + proto_obj = (*aProto)->GetJSObject(); Fold this into the previous line? @@ +5938,3 @@ > } else { > realObj = obj; > } Maybe fold this all into one ternary like in the above. ::: js/xpconnect/src/XPCWrappedNative.cpp @@ +567,1 @@ > if (xpc::AccessCheck::isChrome(js::GetObjectCompartment(obj)) && Is |obj| isn't needed here at all? We could just pass it directly to js::GetObjectCompartment, though it's not clear to me whether that will eventually need to take a handle. ::: security/manager/ssl/src/nsCrypto.cpp @@ +1921,5 @@ > NS_GET_IID(nsIDOMCrypto), getter_AddRefs(holder)); > NS_ENSURE_SUCCESS(nrv, nrv); > > + script_obj = holder->GetJSObject(); > + NS_ENSURE_STATE(script_obj); Maybe move the declaration of script_obj down here?
Attachment #751313 -
Flags: review?(bobbyholley+bmo) → review+
Comment 6•11 years ago
|
||
Comment on attachment 751312 [details] [diff] [review] part 1. Fix the more or less mechanical browser rooting hazards. Review of attachment 751312 [details] [diff] [review]: ----------------------------------------------------------------- \o/ r=me ::: content/xul/content/src/nsXULElement.h @@ +235,5 @@ > void UnlinkJSObjects(); > > void Set(JSScript* aObject); > > + JS::Handle<JSScript*> GetScriptObject() I guess these (and transitive references) are tied strongly to the lifetime of the script and XULElements cannot be stored in realloc'd or memmoved memory? I guess this will explode pretty thoroughly if that's not true in practice. A comment would still be nice here though, if just to give people the idea that returning Handles isn't safe in general.
Attachment #751312 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 7•11 years ago
|
||
> I guess these (and transitive references) are tied strongly to the lifetime of the script
> and XULElements cannot be stored in realloc'd or memmoved memory?
XULPrototypeScripts, but yes. I'll add a comment explaining why this is safe.
Assignee | ||
Comment 8•11 years ago
|
||
> Fold this into the previous line? Done. > Maybe fold this all into one ternary like in the above. Done. > Is |obj| isn't needed here at all? No. GetObjectCompartment is unlikely to ever need a handle; if it does we can worry about that then. Got rid of the temporary. > Maybe move the declaration of script_obj down here? Done. Thank you for the very fast reviews! Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/6bc3ac8a681e https://hg.mozilla.org/integration/mozilla-inbound/rev/908f7d92b2ac
Flags: in-testsuite-
Whiteboard: [need review]
Target Milestone: --- → mozilla24
Comment 9•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6bc3ac8a681e https://hg.mozilla.org/mozilla-central/rev/908f7d92b2ac
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•