868312 - Rooting fixes for dom

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[David Zbarsky (:dzbarsky)]

Attachment: Root some of nsDOMClassInfo.cpp

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 745015 [details] [diff] [review]
Root some of nsDOMClassInfo.cpp

Review of attachment 745015 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDOMClassInfo.cpp
@@ +3936,5 @@
>    if (JSVAL_IS_PRIMITIVE(v)) {
>      return NS_OK;
>    }
>  
> +  JS::Rooted<JSObject*> dom_obj(cx, v.toObjectOrNull());

Make this &v.toObject() and kill the assertion below.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 745015 [details] [diff] [review]
Root some of nsDOMClassInfo.cpp

> nsWindowSH::InvalidateGlobalScopePolluter(JSContext *cx, JSObject *obj)

The "obj" there is a gc hazard too.  Please fix?

>+    JSObject* funObj = nullptr;

That needs rooting too, no?

> nsNodeSH::NewResolve(nsIXPConnectWrappedNative *wrapper, JSContext *cx,

The changes in this method make no sense.  You want the to put the 'obj' root unconditionally up front and then pass 'obj' to both GetNative and the superclass NewResolve, no?

That said, this looks like dead code to me at first glance... Followup?

> nsHTMLFormElementSH::NewResolve(nsIXPConnectWrappedNative *wrapper,

The id needs rooting too.  Probably similar for the other NewResolve methods.

r=me with the above fixed.

Comment 3

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e00967c5786

Comment 4

[David Zbarsky (:dzbarsky)]

Attachment: dom rooting fixes

Comment 5

[David Zbarsky (:dzbarsky)]

Attachment: dom rooting fixes

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1e00967c5786

Comment 7

[David Zbarsky (:dzbarsky)]

Attachment: Finish rooting nsDOMClassInfo.cpp

Comment 8

[David Zbarsky (:dzbarsky)]

Attachment: dom rooting part 2

Comment 9

[David Zbarsky (:dzbarsky)]

Attachment: dom rooting part 2

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 745359 [details] [diff] [review]
dom rooting fixes

>+++ b/content/events/src/nsEventListenerManager.cpp
>@@ -713,17 +713,18 @@ nsEventListenerManager::SetEventHandler(
>   nsIScriptContext* context = global->GetScriptContext();
...
>+  AutoJSContext cx;
>+  JS::Rooted<JSObject*> scope(cx, global->GetGlobalJSObject());

Just use the JSContext that |context| has?

That CameraControl code makes me said.  File a followup bug to move it to bindings?

>+++ b/dom/camera/CameraRecorderProfiles.cpp
> RecorderVideoProfile::GetJsObject(JSContext* aCx, JSObject** aObject)
>   JSString* s = JS_NewStringCopyZ(aCx, codec);
>   JS::Value v = STRING_TO_JSVAL(s);

Don't those need rooting too?

> RecorderAudioProfile::GetJsObject(JSContext* aCx, JSObject** aObject)
>   JSString* s = JS_NewStringCopyZ(aCx, codec);
>   JS::Value v = STRING_TO_JSVAL(s);

And here.

>@@ -175,18 +175,18 @@ RecorderProfileManager::GetJsObject(JSCo
>     JS::Value v = OBJECT_TO_JSVAL(p);

And this guy.

>+++ b/dom/file/LockedFile.cpp
>+    JS::Rooted<JSObject*> obj(aCx, aValue.toObjectOrNull());

toObject().

>+++ b/dom/indexedDB/IDBObjectStore.cpp
>@@ -815,18 +815,18 @@ public:
>+    JS::Rooted<JSString*> name(aCx,
>+      JS_NewUCStringCopyN(aCx, aData.name.get(), aData.name.Length()));
>     JSObject* date = JS_NewDateObjectMsec(aCx, aData.lastModifiedDate);

Root "date" while you're there, ok?  ;)

>+++ b/dom/mobilemessage/src/SmsManager.cpp
>@@ -178,46 +178,46 @@ SmsManager::Send(const JS::Value& aNumbe

I bet that aNumber is a hazard here too.  ;)

>+++ b/dom/mobilemessage/src/ipc/SmsParent.cpp
>@@ -97,31 +97,31 @@ GetParamsFromSendMmsMessageRequest(JSCon
>     JSObject *obj = MmsAttachmentDataToJSObject(aCx,
>                                                 aRequest.attachments().ElementAt(i));
>     NS_ENSURE_TRUE(obj, false);
>     jsval val = JS::ObjectValue(*obj);

Do we not need to root obj and/or val there?

>+++ b/dom/system/OSFileConstants.cpp
>@@ -656,17 +656,17 @@ bool DefineOSFileConstants(JSContext *cx
>   JSObject *objOS;

That needs rooting too, no?

>   JSObject *objLibc;

And this.

r=me with those nits.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 745484 [details] [diff] [review]
dom rooting part 2

>+++ b/dom/workers/Events.cpp
>@@ -409,17 +409,17 @@ public:
>   Create(JSContext* aCx, JSObject* aParent, JSAutoStructuredCloneBuffer& aData,
>          nsTArray<nsCOMPtr<nsISupports> >& aClonedObjects, bool aMainRuntime)

>     JSObject* obj = JS_NewObject(aCx, clasp, NULL, aParent);

Root that, please.

>@@ -634,17 +635,17 @@ public:
>   Create(JSContext* aCx, JSObject* aParent, JSString* aMessage,
>          JSString* aFilename, uint32_t aLineNumber, bool aMainRuntime)
>     JSObject* obj = JS_NewObject(aCx, clasp, NULL, aParent);

And that.

r=me with that

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 745485 [details] [diff] [review]
dom rooting part 2

Same issues as the other part 2.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 745476 [details] [diff] [review]
Finish rooting nsDOMClassInfo.cpp

>+nsWindowSH::InvalidateGlobalScopePolluter(JSContext *cx,
>+                                          JS::MutableHandle<JSObject*> obj)

This is a bit weird.  Why?  This is not fundamentally be supposed to be changing what the caller is holding...  At the very least, document that behavior clearly in the header?

>+  nsresult Install(JSContext *cx, JS::Handle<JSObject*> target,
>+                   JS::MutableHandle<JS::Value> thisAsVal)

Likewise.  At least scope down the 'v' at the caller so that it obviously isn't used after this call?  But I'd prefer passing in a non-mutable handle and setting up another Rooted inside this method, frankly.

Comment 14

[David Zbarsky (:dzbarsky)]

(In reply to Boris Zbarsky (:bz) from comment #10)
> Comment on attachment 745359 [details] [diff] [review]

> >+++ b/dom/camera/CameraRecorderProfiles.cpp
> > RecorderVideoProfile::GetJsObject(JSContext* aCx, JSObject** aObject)
> >   JSString* s = JS_NewStringCopyZ(aCx, codec);
> >   JS::Value v = STRING_TO_JSVAL(s);
> 
> Don't those need rooting too?
> 

Hmm, they should because JS_SetProperty can GC.  I wonder why the static analysis missed these.

> 
> >+++ b/dom/mobilemessage/src/ipc/SmsParent.cpp
> >@@ -97,31 +97,31 @@ GetParamsFromSendMmsMessageRequest(JSCon
> >     JSObject *obj = MmsAttachmentDataToJSObject(aCx,
> >                                                 aRequest.attachments().ElementAt(i));
> >     NS_ENSURE_TRUE(obj, false);
> >     jsval val = JS::ObjectValue(*obj);
> 
> Do we not need to root obj and/or val there?

I don't think so.  They're not used after the call.  Although I guess JS_SetElement should take handles so we'll need to root here eventually.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 745476 [details] [diff] [review]
Finish rooting nsDOMClassInfo.cpp

The rest looks fine, but please fix the mutable table footguns.

Comment 16

[Boris Zbarsky [:bzbarsky]]

I mean mutable _handle_.

Comment 17

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae645c508e70
https://hg.mozilla.org/integration/mozilla-inbound/rev/059b2743e8d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba8529d2c5ec

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ae645c508e70
https://hg.mozilla.org/mozilla-central/rev/059b2743e8d7
https://hg.mozilla.org/mozilla-central/rev/ba8529d2c5ec

Comment 19

[David Zbarsky (:dzbarsky)]

Attachment: dom/workers

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 747258 [details] [diff] [review]
dom/workers

>+  JS::Rooted<JSObject*> eventTargetProto(aCx,
>+    EventTargetBinding_workers::GetProtoObject(aCx, global));

Could also do:

  JS::Handle<JSObject*> eventTargetProto(
    EventTargetBinding_workers::GetProtoObject(aCx, global));

if it's not assigned to later.

r=me either way.

Comment 21

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22dffe8154e5

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/22dffe8154e5

Comment 23

[David Zbarsky (:dzbarsky)]

Attachment: dom rooting fixes

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 747721 [details] [diff] [review]
dom rooting fixes

> nsJSContext::ExecuteScript(JSScript* aScriptObject,
>+                           JSObject* aScopeObject_)

Do a followup on passing a handle here?


>@@ -352,23 +358,23 @@ DOMCameraCapabilities::GetVideoSizes(JSC
>     JS::Value v = INT_TO_JSVAL(sizes[i].width);

That's a hazard too, right?

r=me

Comment 25

[David Zbarsky (:dzbarsky)]

(In reply to Boris Zbarsky (:bz) from comment #24)
> Comment on attachment 747721 [details] [diff] [review]
> dom rooting fixes
> 
> > nsJSContext::ExecuteScript(JSScript* aScriptObject,
> >+                           JSObject* aScopeObject_)
> 
> Do a followup on passing a handle here?
> 
> 
> >@@ -352,23 +358,23 @@ DOMCameraCapabilities::GetVideoSizes(JSC
> >     JS::Value v = INT_TO_JSVAL(sizes[i].width);
> 
> That's a hazard too, right?
> 
> r=me

Comment 26

[Boris Zbarsky [:bzbarsky]]

I'm not following comment 25.  ;)

Comment 27

[David Zbarsky (:dzbarsky)]

Oops, that's what happens when I try to use bugzilla at 4:30.  
https://hg.mozilla.org/integration/mozilla-inbound/rev/17bc0e8cfa8b

Comment 28

[David Zbarsky (:dzbarsky)]

Attachment: Various fixes

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 748277 [details] [diff] [review]
Various fixes

>+++ b/dom/indexedDB/IDBObjectStore.cpp
> GetAddInfoCallback(JSContext* aCx, void* aClosure)
>+  JS::Rooted<JS::Value> val(aCx, data->mValue);
>+  if (!IDBObjectStore::SerializeValue(aCx, data->mCloneWriteInfo, val)) {

No, this is bad.

In particular, the data->mValue is unrooted between when it's set and just now, so it can get moved and whatnot and then we lose.

You should make the mValue of GetAddInfoClosure a Rooted, make that struct MOZ_STACK_CLASS or whatever our annotation du jour is, and then this will actually be rooted properly.  I filed bug 871075 on the analysis not catching this.

>+++ b/dom/workers/Exceptions.cpp
>   JSString* jsmessage = JS_NewStringCopyZ(aCx, message);

Root this too, while you're here.

r=me with those fixed

Comment 30

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2907df57752d

Comment 31

[David Zbarsky (:dzbarsky)]

Attachment: More fixes

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 748468 [details] [diff] [review]
More fixes

r=me

Comment 33

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/599ab91f67a6

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/mozilla-central/rev/17bc0e8cfa8b
https://hg.mozilla.org/mozilla-central/rev/2907df57752d

Comment 35

[(no longer active)]

See bug 871321.

Comment 36

[David Zbarsky (:dzbarsky)]

Attachment: Fixes

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/599ab91f67a6

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 748670 [details] [diff] [review]
Fixes

>+    JS::Handle<JSObject*> cachedHandler(pWindow->GetCachedXBLPrototypeHandler(this));

You want a Rooted here; more on this below.

>+JS::Handle<JSObject*>
> nsGlobalWindow::GetCachedXBLPrototypeHandler(nsXBLPrototypeHandler* aKey)

This should continue to return a JSObject*.  Remember, a Handle is really a JSObject** under the hood.  A Rooted on the stack is a JSObject* under the hood.  Creating a Handle from it is just taking the address. So your change to this function returns a pointer to the function's stack, which is a bad idea.

r=me with that fixed.

Comment 39

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b73e9c7a5d0

Comment 40

[David Zbarsky (:dzbarsky)]

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/f50a19e8295f

Comment 41

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c4f374baea0a

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c4f374baea0a

Comment 43

[Boris Zbarsky [:bzbarsky]]

Attachment: finale: finish rooting dom/ code.

Olli, please let me know if you want me to split this up into more bite-sized pieces...

Comment 44

[Boris Zbarsky [:bzbarsky]]

Attachment: Passing tests now

Comment 45

[Boris Zbarsky [:bzbarsky]]

Attachment: Now with RootedDictionary to make things saner

Comment 46

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 751197 [details] [diff] [review]
Now with RootedDictionary to make things saner


>+  virtual void trace(JSTracer *trc) MOZ_OVERRIDE {
Nit, { should be in the next line


>diff --git a/dom/devicestorage/test/test_enumerateOptions.html b/dom/devicestorage/test/test_enumerateOptions.html
>--- a/dom/devicestorage/test/test_enumerateOptions.html
>+++ b/dom/devicestorage/test/test_enumerateOptions.html
>@@ -43,17 +43,17 @@ ok(!throws, "enumerate one string parame
> throws = false;
> try {
> var cursor = storage.enumerate("string", "string2");
> } catch(e) {throws = true}
> ok(throws, "enumerate two string parameter");
> 
> throws = false;
> try {
>-var cursor = storage.enumerate("string", {"since": 1});
>+var cursor = storage.enumerate("string", {"since": new Date(1)});
> } catch(e) {throws = true}
> ok(!throws, "enumerate a string and object parameter");
> 
> throws = false;
> try {
> var cursor = storage.enumerate({"path": "a"});
> } catch(e) {throws = true}
> ok(!throws, "enumerate object parameter with path");
>@@ -61,17 +61,17 @@ ok(!throws, "enumerate object parameter 
> throws = false;
> try {
> var cursor = storage.enumerate({}, "string");
> } catch(e) {throws = true}
> ok(throws, "enumerate object then a string");
> 
> throws = false;
> try {
>-var cursor = storage.enumerate({"path": "a", "since": 0});
>+var cursor = storage.enumerate({"path": "a", "since": new Date(0) });
> } catch(e) {throws = true}
> ok(!throws, "enumerate object parameter with path");
Hmm, ok, the old code handling since is just a bit silly



>     XMLHttpRequest::StateData state;
>+    // XXXbz there is no AutoValueRooter anymore?
>+    JS::AutoArrayRooter rooter(aCx, 1, &state.mResponse);
This is ugly, but apparently we don't have anything better.

Comment 47

[Boris Zbarsky [:bzbarsky]]

> Nit, { should be in the next line

Done.

> Hmm, ok, the old code handling since is just a bit silly

You mean the silently ignoring invalid values?  Yeah.  I did check this test change over with dhylands on IRC, for what it's worth.

> This is ugly, but apparently we don't have anything better.

Short of writing yet another subclass of both CustomAutoRooter and StateData, indeed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9fcdf0c1c598

I think it should be fine to resolve this and file new bugs on anything that's left that's not a false positive.

Comment 48

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9fcdf0c1c598