Fix some rooting hazards in content/

RESOLVED FIXED in mozilla24

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

Trunk
mozilla24
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 748584 [details] [diff] [review]
Patch (v1)
Attachment #748584 - Flags: review?(tschneidereit)
Comment on attachment 748584 [details] [diff] [review]
Patch (v1)

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

JS::RootedObject and friends should not be used outside js/. Use JS::Rooted<JSObject*>, and don't forget to get review from a DOM peer.
Attachment #748584 - Flags: feedback-
(Assignee)

Comment 2

5 years ago
(In reply to comment #1)
> Comment on attachment 748584 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=748584
> Patch (v1)
> 
> Review of attachment 748584 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> JS::RootedObject and friends should not be used outside js/.

Why?

> Use JS::Rooted<JSObject*>, and don't forget to get review from a DOM peer.

Hrm, OK.  Till has been reviewing all my patches so far, but I guess I can ask bz too.
(Assignee)

Updated

5 years ago
Attachment #748584 - Flags: review?(bzbarsky)
Comment on attachment 748584 [details] [diff] [review]
Patch (v1)

The JS folks keep telling us to not use JS::Rooted* and JS::Handle*...  I think they want to remove them or something, not sure.  So JS::Rooted<T> and JS::Handle<T> it is.

>+    JS::RootedObject handler(aCx, jsl->GetHandler().Ptr()->Callable());

I suspect this can be:

  JS::Handle<JSObject*> handler(jsl->GetHandler().Ptr()->Callable());

if you change CallbackFunction::Callable to return a JS::Handle<JSObject*>.  Which it should easily be able to do, since CallbackObject::Callback returns one.

>+  JS::Rooted<JSObject*> value(cx, &v.get().toObject());

Does &v.toObject() not do the right thing?  :(

r=me with the nits fixed.
Attachment #748584 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #3)
> The JS folks keep telling us to not use JS::Rooted* and JS::Handle*...  I
> think they want to remove them or something, not sure.  So JS::Rooted<T> and
> JS::Handle<T> it is.

Hmm, ok. Seems like the JS folks should align their opinions on this, then. ;) I'm sure we'll be able to do that during the next week.
> Seems like the JS folks should align their opinions on this, then

<shrug>.  I was told that had already happened.  Have fun with it at the work week, in any case.  ;)
Oh well, so maybe I'm just not up to speed on what's going on there. That is pretty likely, actually. I'll find out tomorrow, I guess. :)
(Assignee)

Comment 7

5 years ago
FWIW I've been adding JS::Rooted* stuff relentlessly over the past couple of weeks.  I hope that has not made people's lives harder.

Also I do hope that JS::Rooted* stuff don't get removed.  I think they are more beautiful than JS::Rooted<JSObject*> and the like.  :-)
(Assignee)

Comment 8

5 years ago
(In reply to Boris Zbarsky (:bz) from comment #3)
> >+  JS::Rooted<JSObject*> value(cx, &v.get().toObject());
> 
> Does &v.toObject() not do the right thing?  :(

It actually does!
(Assignee)

Comment 9

5 years ago
Created attachment 748836 [details] [diff] [review]
Patch (v2)
Attachment #748584 - Attachment is obsolete: true
Attachment #748584 - Flags: review?(tschneidereit)
Attachment #748836 - Flags: review?(tschneidereit)
(Assignee)

Comment 10

5 years ago
Created attachment 748894 [details] [diff] [review]
Patch (v2)

This fixes the build problem on nsGlobalWindow.cpp.
Attachment #748836 - Attachment is obsolete: true
Attachment #748836 - Flags: review?(tschneidereit)
Attachment #748894 - Flags: review?(tschneidereit)
Comment on attachment 748894 [details] [diff] [review]
Patch (v2)

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

cool

::: content/events/src/nsEventListenerManager.cpp
@@ +857,5 @@
>      JS::CompileOptions options(cx);
>      options.setFileAndLine(url.get(), lineNo)
>             .setVersion(SCRIPTVERSION_DEFAULT);
>  
> +    JS::Rooted<JSObject*> rootedNull(cx, nullptr); // See bug 781070.

You can use JS::NullPtr now. Here and below.

::: content/events/src/nsEventListenerService.cpp
@@ +94,5 @@
>    }
>  
>    nsCOMPtr<nsIJSEventListener> jsl = do_QueryInterface(mListener);
>    if (jsl) {
> +    JS::Handle<JSObject*> handler(jsl->GetHandler().Ptr()->Callable());

Callable() returns a handler, I suppose?
Attachment #748894 - Flags: review?(tschneidereit) → review+
(Assignee)

Comment 13

5 years ago
Forgot to remove the rootedNull local variable: http://hg.mozilla.org/integration/mozilla-inbound/rev/d0d2e13e8b83
(Assignee)

Comment 15

5 years ago
So this patch broke the build like this:

https://tbpl.mozilla.org/php/getParsedLog.php?id=22910497&full=1&branch=mozilla-inbound#error0

I suspect this is the thing that fails to compile (I cannot repro locally when building with either clang or gcc, yikes!):

<http://hg.mozilla.org/mozilla-central/annotate/c80dc6ffe865/dom/base/nsGlobalWindow.cpp#l11797>

Boris, any idea how I should fix up this code now that Callable returns a Handle<JSObject*>?
Flags: needinfo?(bzbarsky)
Perhaps replace this:

  vp->setObjectOrNull(h ? h->Callable() : nullptr); 

with:

  vp->setObjectOrNull(h ? h->Callable().get() : nullptr); 

?

You will want the same in nsINode.cpp, HTMLBodyElement.cpp, and HTMLFrameSetElement.cpp...
Flags: needinfo?(bzbarsky)
https://hg.mozilla.org/mozilla-central/rev/2c347b02cf44
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.