GC: Remove Handle<T> construction from Heap<T>

RESOLVED FIXED in mozilla25

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

unspecified
mozilla25
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
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

5 years ago
Created attachment 774035 [details] [diff] [review]
Remove constructor, and use fromMarkedLocation instead

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

5 years ago
Created attachment 777113 [details] [diff] [review]
1 - Remove constructor

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

5 years ago
Created attachment 777115 [details] [diff] [review]
2 - Re-root heap pointers

Re-root heap pointers where they are used, rather than calling fromMarkedLocation to generate a Handle to them.
Attachment #777115 - Flags: review?(bzbarsky)
Comment on attachment 777113 [details] [diff] [review]
1 - Remove constructor

r=me
Attachment #777113 - Flags: review?(bzbarsky) → review+
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

5 years ago
Created attachment 778516 [details] [diff] [review]
2 - Add comments to fromMarkedLocation callers

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 on attachment 778516 [details] [diff] [review]
2 - Add comments to fromMarkedLocation callers

Awesome.  r=me
Attachment #778516 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/0181f53d20e2
https://hg.mozilla.org/mozilla-central/rev/fe2ed5eff8e2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.