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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

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

[Boris Zbarsky [:bzbarsky]]

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....

Comment 2

[Jon Coppeard (:jonco)]

Attachment: 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.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: 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.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: 2 - Re-root heap pointers

Re-root heap pointers where they are used, rather than calling fromMarkedLocation to generate a Handle to them.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 777113 [details] [diff] [review]
1 - Remove constructor

r=me

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: 2 - Add comments to fromMarkedLocation callers

As discussed, I've added to comments to say why these are safe rather than re-rooting.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 778516 [details] [diff] [review]
2 - Add comments to fromMarkedLocation callers

Awesome.  r=me

Comment 9

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b1703dbd7ae
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe2ed5eff8e2

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0181f53d20e2
https://hg.mozilla.org/mozilla-central/rev/fe2ed5eff8e2