866450 - Fix some rooting hazards under content/ and dom/

Status: RESOLVED FIXED

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

Comment 1

[David Zbarsky (:dzbarsky)]

Attachment: Part 1

Comment 2

[David Zbarsky (:dzbarsky)]

Attachment: Part 1

Comment 3

[David Zbarsky (:dzbarsky)]

Attachment: Part 2

Comment 4

[David Zbarsky (:dzbarsky)]

Attachment: Part 3

Comment 5

[David Zbarsky (:dzbarsky)]

Attachment: Part 4

Comment 6

[Boris Zbarsky [:bzbarsky]]

I _think_ we're using JS::Rooted<Foo> and JS::Handle<Foo> instead of JS::RootedFoo and JS::HandleFoo....

Comment 7

[Boris Zbarsky [:bzbarsky]]

Also, any rooting changes you make to codegen are probably conflicting with changes I'm making to it in one of the deps of bug 865969.  :(

Comment 8

[David Zbarsky (:dzbarsky)]

Attachment: Part 5

Comment 9

[David Zbarsky (:dzbarsky)]

Attachment: Part 6

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 742742 [details] [diff] [review]
Part 1

Like I said, I think we want Rooted<> and Handle<> instead of the one-word things.  You should be able to just search-and-replace (on the diff?) to fix that.

>+++ b/content/base/src/nsContentList.cpp
> nsContentList::NamedItem(JSContext* cx, const nsAString& name,
>   JS::Value v;

Root this too, while you're here.

>+++ b/content/base/src/nsFrameMessageManager.cpp
>@@ -652,17 +652,17 @@ nsFrameMessageManager::ReceiveMessage(ns
>         JS::Value targetv;

This needs rooting too.

>@@ -696,34 +696,34 @@ nsFrameMessageManager::ReceiveMessage(ns
>+        JS::RootedValue thisValue(ctx, JSVAL_VOID);

s/JSVAL_VOID/JS::UndefinedValue()/

>         JS::Value funval;

And root that?

>@@ -731,17 +731,17 @@ nsFrameMessageManager::ReceiveMessage(ns
>         JS::Value rval = JSVAL_VOID;

That needs to be rooted.

>+          JS::RootedObject thisObject(ctx, JSVAL_TO_OBJECT(thisValue));

&thisValue.toObject(), while you're here.

>+++ b/content/base/src/nsObjectLoadingContent.cpp

There's various missing rooting here but I'll pick it up in bug 867844: it needs codegen fixes.

> nsObjectLoadingContent::GetPluginJSObject(JSContext *cx, JSObject *obj,

Why the changes in this function?

> nsObjectLoadingContent::TeardownProtoChain()
>-  JSObject *proto;

Not sure why you moved this... but in any case it should be rooted.

>+++ b/content/base/src/nsXMLHttpRequest.cpp
>   JS::Value v = JSVAL_NULL;

Root that while your're here?

>+++ b/content/canvas/src/CanvasUtils.h
> DashArrayToJSVal(FallibleTArray<T>& dashes,
>+        if (!JS_SetElement(cx, obj, i, &elt)) {

While you're here, JS_DefineElement, please!

>+++ b/content/events/src/nsDOMMessageEvent.cpp
>+nsDOMMessageEvent::GetData(JSContext* aCx, mozilla::ErrorResult& aRv)
>+  JS::Value data = mData;

That should be rooted.

>+++ b/content/html/content/src/HTMLInputElement.cpp
>@@ -1180,17 +1180,17 @@ HTMLInputElement::ConvertStringToNumber(

This code died, thank goodness.  ;)

>@@ -1370,17 +1370,17 @@ HTMLInputElement::ConvertNumberToString(

And this too.

>@@ -1449,17 +1449,17 @@ HTMLInputElement::GetValueAsDate(JSConte

And this will die in bug 742206.

>+++ b/content/html/content/src/HTMLOptionsCollection.cpp
> HTMLOptionsCollection::NamedItem(JSContext* cx, const nsAString& name,
>   JS::Value v;

Should root this too.

>+++ b/content/html/content/src/HTMLTableElement.cpp
>@@ -227,17 +227,17 @@ TableRowsCollection::NamedItem(JSContext
>         JS::Value v;

And this.

>+++ b/content/html/content/src/nsHTMLFormElement.cpp
> nsFormControlList::NamedItem(JSContext* cx, const nsAString& name,
>   JS::Value v;

And this.

>+++ b/content/html/document/src/nsHTMLDocument.cpp
>@@ -2365,17 +2365,17 @@ nsHTMLDocument::NamedGetter(JSContext* c
>   JS::Value val;

And this.

>+++ b/content/xul/templates/src/nsXULTemplateBuilder.cpp
>@@ -1388,17 +1388,17 @@ nsXULTemplateBuilder::InitHTMLTemplateRo
>     JS::Value v;

And this.

r=me

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 742841 [details] [diff] [review]
Part 2

>+++ b/content/base/src/nsDocument.cpp
>@@ -6582,25 +6582,25 @@ nsIDocument::AdoptNode(nsINode& aAdopted
>       JS::Value v;

Root this while you're here, please?

>+++ b/dom/base/nsGlobalWindow.cpp
>@@ -6575,17 +6575,17 @@ PostMessageReadStructuredClone(JSContext
>         JS::Value val;

And this.

>+++ b/dom/base/nsJSEnvironment.cpp
>@@ -1512,17 +1512,18 @@ nsJSContext::BindCompiledEventHandler(ns
>   JSObject *target = nullptr;

And that.

>+++ b/dom/devicestorage/nsDeviceStorage.cpp
>@@ -918,18 +918,19 @@ InterfaceToJsval(nsPIDOMWindow* aWindow,
>   JS::Value someJsVal;

And that.

>+++ b/dom/file/ArchiveRequest.cpp
>@@ -242,18 +241,19 @@ ArchiveRequest::GetFilesResult(JSContext
>     JS::Value value;

And that.

>+++ b/dom/indexedDB/IDBObjectStore.cpp
>     jsval wrappedFileHandle;

And that.

>@@ -721,18 +722,19 @@ public:
>       jsval wrappedBlob;

Likewise.

>@@ -747,18 +749,19 @@ public:
>     jsval wrappedFile;

And here.

>+++ b/dom/ipc/StructuredCloneUtils.cpp
>     JS::Value wrappedFile;

And this.

>@@ -85,18 +86,19 @@ Read(JSContext* aCx, JSStructuredCloneRe
>     JS::Value wrappedBlob;


And this.

>+++ b/dom/mobilemessage/src/MobileMessageCallback.cpp
>   JS::Value wrappedMessage;

And that.

>+++ b/dom/mobilemessage/src/MobileMessageCursorCallback.cpp
>   JS::Value wrappedResult;

And here.

>+++ b/dom/workers/WorkerPrivate.cpp
>         JS::Value wrappedFile;

And here.

>@@ -338,18 +339,19 @@ struct MainThreadWorkerStructuredCloneCa
>         JS::Value wrappedBlob;

And here.

r=me

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 742868 [details] [diff] [review]
Part 3

Ah, I see.  This roots some of those values from previous patches, ok.

>+++ b/dom/bindings/Codegen.py

Shouldn't be needed anymore.  ;)

r=me with that bit removed.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 742871 [details] [diff] [review]
Part 4

r=me

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 743157 [details] [diff] [review]
Part 5

>+++ b/content/base/src/nsDocument.cpp
> CustomElementConstructor(JSContext *aCx, unsigned aArgc, JS::Value* aVp)
>+  JS::RootedValue calleeVal(aCx, JS_CALLEE(aCx, aVp));

I think it would be better to do:

  JS::CallArgs args = JS::CallArgsFromVp(aArgc, aVp);

and then replace uses of calleeVal with &args.callee() and args.calleev() depending on which is needed.

>@@ -4992,18 +4992,18 @@ CustomElementConstructor(JSContext *aCx,
>+  JS::RootedValue v(aCx);

I think you should just nix v completely and pass args.rval().address() to WrapNative.

>@@ -5058,49 +5058,49 @@ nsDocument::Register(JSContext* aCx, con
>+  JS::RootedObject htmlProto(aCx, HTMLElementBinding::GetProtoObject(aCx, global));

  JS::Handle<JSObject*> htmlProto(HTMLElementBinding::GetProtoObject(aCx, global));

please.

>+++ b/content/events/src/nsEventListenerService.cpp
>+    JS::RootedObject handler(aCx, jsl->GetHandler().Ptr()->Callable());

I think you should make Callable() return a JS::Handle<JSObject*> and declare handler to be of that type here.  Note that CallbackObject::Callback() already returns a handle, so this should be pretty easy.

>+++ b/content/html/content/src/HTMLInputElement.cpp
>@@ -1196,18 +1196,18 @@ HTMLInputElement::ConvertStringToNumber(

This code is gone.

>@@ -1376,20 +1376,20 @@ HTMLInputElement::ConvertNumberToString(

So is this.

>@@ -1521,18 +1521,18 @@ HTMLInputElement::SetValueAsDate(JSConte

And this will die in bug 742206.  Please don't bitrot me?  ;)

r=me with that.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 743158 [details] [diff] [review]
Part 6

r=me

Comment 16

[David Zbarsky (:dzbarsky)]

(In reply to Boris Zbarsky (:bz) from comment #14)
> Comment on attachment 743157 [details] [diff] [review]
> Part 5

> 
> I think you should make Callable() return a JS::Handle<JSObject*> and
> declare handler to be of that type here.  Note that
> CallbackObject::Callback() already returns a handle, so this should be
> pretty easy.
> 

This caused some build failures that I haven't figured out yet so I'll do it in a followup.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad10907da2d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/b514d768d793
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e6a175fc36a
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1e877fa8d67
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5ba9c0dc252
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c0ace2560c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0b51717bb64

Comment 17

[Ed Morley [:emorley]]

Ms2ger landed a followup to fix B2G bustage, but it was still broken after that, so backed the lot out:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9cd2de3d0716
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/abda0ace7aaf
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/872f39ccacf0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/03b95758bc2a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9db41ac6c881
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/654e10bc0a64
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4af79ceb2bf7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a299e38549e7

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=e0b51717bb64
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=b3df3d58f20c

Comment 18

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

I'm looking at fixing the B2G issues.

Comment 19

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/27f8c1c2e09e
https://hg.mozilla.org/integration/mozilla-inbound/rev/173a6deb4792
https://hg.mozilla.org/integration/mozilla-inbound/rev/82dfcc2b5366
https://hg.mozilla.org/integration/mozilla-inbound/rev/b940af7c9a0e
https://hg.mozilla.org/integration/mozilla-inbound/rev/4864e9d9a418
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae0aac5ce54e
https://hg.mozilla.org/integration/mozilla-inbound/rev/478105be6e91

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/27f8c1c2e09e
https://hg.mozilla.org/mozilla-central/rev/173a6deb4792
https://hg.mozilla.org/mozilla-central/rev/82dfcc2b5366
https://hg.mozilla.org/mozilla-central/rev/b940af7c9a0e
https://hg.mozilla.org/mozilla-central/rev/4864e9d9a418
https://hg.mozilla.org/mozilla-central/rev/ae0aac5ce54e
https://hg.mozilla.org/mozilla-central/rev/478105be6e91

Comment 21

[David Zbarsky (:dzbarsky)]

Attachment: Root some of nsDOMClassInfo.cpp