Closed Bug 866450 Opened 7 years ago Closed 7 years ago

Fix some rooting hazards under content/ and dom/

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: dzbarsky, Assigned: dzbarsky)

References

Details

Attachments

(6 files, 2 obsolete files)

No description provided.
Attached patch Part 1 (obsolete) — Splinter Review
Assignee: nobody → dzbarsky
Status: NEW → ASSIGNED
Attachment #742741 - Flags: review?(Ms2ger)
Attached patch Part 1Splinter Review
Attachment #742741 - Attachment is obsolete: true
Attachment #742741 - Flags: review?(Ms2ger)
Attachment #742742 - Flags: review?(Ms2ger)
Attached patch Part 2Splinter Review
Attachment #742841 - Flags: review?(Ms2ger)
Summary: Fix some rooting hazards under content/ → Fix some rooting hazards under content/ and dom/
Attached patch Part 3Splinter Review
Attachment #742868 - Flags: review?(Ms2ger)
Attached patch Part 4Splinter Review
Attachment #742871 - Flags: review?(Ms2ger)
I _think_ we're using JS::Rooted<Foo> and JS::Handle<Foo> instead of JS::RootedFoo and JS::HandleFoo....
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.  :(
Attached patch Part 5Splinter Review
Attachment #743157 - Flags: review?(Ms2ger)
Attached patch Part 6Splinter Review
Attachment #743158 - Flags: review?(Ms2ger)
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
Attachment #742742 - Flags: review?(Ms2ger) → review+
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
Attachment #742841 - Flags: review?(Ms2ger) → review+
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.
Attachment #742868 - Flags: review?(Ms2ger) → review+
Comment on attachment 742871 [details] [diff] [review]
Part 4

r=me
Attachment #742871 - Flags: review?(Ms2ger) → review+
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.
Attachment #743157 - Flags: review?(Ms2ger) → review+
Comment on attachment 743158 [details] [diff] [review]
Part 6

r=me
Attachment #743158 - Flags: review?(Ms2ger) → review+
(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
I'm looking at fixing the B2G issues.
Attached patch Root some of nsDOMClassInfo.cpp (obsolete) — Splinter Review
Attachment #745002 - Flags: review?(bzbarsky)
Attachment #745002 - Attachment is obsolete: true
Attachment #745002 - Flags: review?(bzbarsky)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.