Closed
Bug 891966
Opened 11 years ago
Closed 11 years ago
GC: Remove Handle<T> construction from Heap<T>
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla25
People
(Reporter: jonco, Assigned: jonco)
Details
Attachments
(2 files, 2 obsolete files)
7.61 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
9.73 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
It's been pointed out that this is a bad idea as the a contents of Handle to a Heap member can change unexpectedly. Uses of this should be replaces by re-rooting the value.
Comment 1•11 years ago
|
||
Two notes: 1) This is also a problem for uses of fromMarkedLocation, in theory. 2) The same problem exists with Rooted. Consider this code: Rooted<JSObject*> r(cx, obj); doSomething(r, &r); where doSomething is declared as: void doSomething(HandleObject in, MutableHandleObject out); This pattern used to be safe when doSomething took a JSObject* and a JSObject**, but with handles it's not so safe....
Assignee | ||
Comment 2•11 years ago
|
||
As a first pass, get rid of the constructor and use fromMarkedLocation to create handles in its place. We can replace these uses of fromMarkedLocation by re-rooting, but actually they all look safe to me. And I'm unsure of whether any of these are performance sensitive areas.
Assignee | ||
Comment 3•11 years ago
|
||
This patch removes the Handle<T> constructor that takes a Heap<T> and replaces its use with fromMarkedLocation(). These uses are changed again in the second patch.
Attachment #774035 -
Attachment is obsolete: true
Attachment #777113 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 4•11 years ago
|
||
Re-root heap pointers where they are used, rather than calling fromMarkedLocation to generate a Handle to them.
Attachment #777115 -
Flags: review?(bzbarsky)
Comment 5•11 years ago
|
||
Comment on attachment 777113 [details] [diff] [review] 1 - Remove constructor r=me
Attachment #777113 -
Flags: review?(bzbarsky) → review+
Comment 6•11 years ago
|
||
Comment on attachment 777115 [details] [diff] [review] 2 - Re-root heap pointers >+++ b/content/base/src/nsDocument.cpp >@@ -5184,18 +5184,17 @@ nsDocument::Register(JSContext* aCx, con >- JS::Handle<JSObject*> htmlProto( >- HTMLElementBinding::GetProtoObject(aCx, global)); >+ JS::Rooted<JSObject*> htmlProto(aCx, HTMLElementBinding::GetProtoObject(aCx, global)); Why? The slot GetProtoObject returns a handle to is immutable. No point in this change. >+++ b/content/base/src/nsINode.cpp >@@ -2104,17 +2104,17 @@ nsINode::SizeOfExcludingThis(MallocSizeO >- vp->setObjectOrNull(h ? h->Callable().get() : nullptr); \ >+ vp->setObjectOrNull(h ? h->Callable() : nullptr); \ You'll need to undo this; see below. >+++ b/content/base/src/nsObjectLoadingContent.cpp >@@ -3203,17 +3203,17 @@ nsObjectLoadingContent::SetupProtoChain( >- JS::Handle<JSObject*> my_proto = GetDOMClass(aObject)->mGetProto(aCx, global); >+ JS::Rooted<JSObject*> my_proto(aCx, GetDOMClass(aObject)->mGetProto(aCx, global)); Again, the proto getter returns a handle to an immutable slot. Please undo this change. >+++ b/content/events/src/nsEventListenerService.cpp >- JS::Handle<JSObject*> handler(jsl->GetHandler().Ptr()->Callable()); >+ JS::Rooted<JSObject*> handler(aCx, jsl->GetHandler().Ptr()->Callable()); And this: the callable is immutable. >+++ b/content/html/content/src/HTMLBodyElement.cpp >- vp->setObjectOrNull(h ? h->Callable().get() : nullptr); \ >+ vp->setObjectOrNull(h ? h->Callable() : nullptr); \ And this. >+++ b/content/html/content/src/HTMLFrameSetElement.cpp >- vp->setObjectOrNull(h ? h->Callable().get() : nullptr); \ >+ vp->setObjectOrNull(h ? h->Callable() : nullptr); \ You see the pattern. ;) >+++ b/content/xbl/src/nsXBLProtoImplMethod.cpp >@@ -256,29 +256,29 @@ nsXBLProtoImplMethod::Write(nsIScriptCon >- JS::Handle<JSObject*> method = >- JS::Handle<JSObject*>::fromMarkedLocation(mMethod.AsHeapObject().address()); >+ AutoPushJSContext cx(aContext->GetNativeContext()); >+ JS::Rooted<JSObject*> method(cx, GetCompiledMethod()); And the compiled method is immutable too, no? >@@ -364,16 +364,16 @@ nsXBLProtoImplAnonymousMethod::Write(nsI >- JS::Handle<JSObject*> method = >- JS::Handle<JSObject*>::fromMarkedLocation(mMethod.AsHeapObject().address()); >+ AutoPushJSContext cx(aContext->GetNativeContext()); >+ JS::Rooted<JSObject*> method(cx, GetCompiledMethod()); And here. >+++ b/content/xbl/src/nsXBLProtoImplProperty.cpp >@@ -352,24 +352,24 @@ nsXBLProtoImplProperty::Write(nsIScriptC >- JS::Handle<JSObject*> function = >- JS::Handle<JSObject*>::fromMarkedLocation(mGetter.AsHeapObject().address()); >+ JS::Rooted<JSObject*> function(cx, mGetter.GetJSFunction()); ... >- JS::Handle<JSObject*> function = >- JS::Handle<JSObject*>::fromMarkedLocation(mSetter.AsHeapObject().address()); >+ JS::Rooted<JSObject*> function(cx, mSetter.GetJSFunction()); And these. >+++ b/content/xul/content/src/nsXULElement.cpp >@@ -2376,18 +2376,18 @@ nsXULPrototypeScript::Serialize(nsIObjec >- JS::Handle<JSScript*> script = >- JS::Handle<JSScript*>::fromMarkedLocation(mScriptObject.address()); >+ AutoPushJSContext cx(context->GetNativeContext()); >+ JS::Rooted<JSScript*> script(cx, mScriptObject); Is this one mutable? I have my doubts. >+++ b/content/xul/document/src/XULDocument.cpp >+ AutoJSContext cx; So how did you decide on AutoJSContext vs AutoPushJSContext? >+++ b/dom/bindings/CallbackFunction.h >- JS::Handle<JSObject*> Callable() const >+ JSObject* Callable() const I don't think we should make this change. There's no reason to, and performance to be lost by doing it. >+++ b/dom/bindings/CallbackObject.cpp >+CallbackObject::CallSetup::CallSetup(const CallbackObject& aCallback, No, definitely not. We're about to decouple CallSetup from CallbackObject entirely, fwiw, so this would totally not work. >+++ b/dom/bindings/CallbackObject.h >- JS::Handle<JSObject*> Callback() const >+ JSObject* Callback() const ... >- JS::Handle<JSObject*> CallbackPreserveColor() const >+ JSObject* CallbackPreserveColor() const Again, I don't see a reason for this change. >+++ b/dom/bindings/Codegen.py >- parentProtoType = "Handle" >+ parentProtoType = "Rooted" Please, no. The previous setup was just fine. >- constructorProtoType = "Handle" >+ constructorProtoType = "Rooted" Likewise. >@@ -1872,33 +1872,33 @@ class CGGetPerInterfaceObject(CGAbstract >- 'JS::Handle<JSObject*>', args, inline=True) >+ 'JSObject*', args, inline=True) And here. And so on through this whole file. >+++ b/dom/bindings/DOMJSClass.h >-typedef JS::Handle<JSObject*> (*ProtoGetter)(JSContext* aCx, >- JS::Handle<JSObject*> aGlobal); >+typedef JSObject* (*ProtoGetter)(JSContext* aCx, JS::Handle<JSObject*> aGlobal); And please leave that.
Attachment #777115 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 7•11 years ago
|
||
As discussed, I've added to comments to say why these are safe rather than re-rooting.
Attachment #777115 -
Attachment is obsolete: true
Attachment #778516 -
Flags: review?(bzbarsky)
Comment 8•11 years ago
|
||
Comment on attachment 778516 [details] [diff] [review] 2 - Add comments to fromMarkedLocation callers Awesome. r=me
Attachment #778516 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 9•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b1703dbd7ae https://hg.mozilla.org/integration/mozilla-inbound/rev/fe2ed5eff8e2
Comment 10•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0181f53d20e2 https://hg.mozilla.org/mozilla-central/rev/fe2ed5eff8e2
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in
before you can comment on or make changes to this bug.
Description
•