855411 - GC: Rooting in xbl

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Tom S. (please needinfo tschuster)]

This should fix all rooting issues in xbl. There are something with Finalize that I didn't touch, because we shouldn't GC there.

Comment 1

[Tom S. (please needinfo tschuster)]

Fun times, just redid all of the patch piece by piece, because the browser was crashing on startup.
All because I wrote JS_DATA_TO_FUNC_PTR(JSPropertyOp, get.address()) instead of JS_DATA_TO_FUNC_PTR(JSPropertyOp, get.get()). I hope we can get rid of this soon.

Comment 2

[Tom S. (please needinfo tschuster)]

Attachment: changedocument

Comment 3

[Tom S. (please needinfo tschuster)]

Attachment: Init[Js]Clas

Comment 4

[Tom S. (please needinfo tschuster)]

Attachment: Lookup*

Comment 5

[Tom S. (please needinfo tschuster)]

Attachment: checkAccess

Comment 6

[Tom S. (please needinfo tschuster)]

Attachment: InstallMember

Comment 7

[Tom S. (please needinfo tschuster)]

Attachment: CompileMember

Comment 8

[Tom S. (please needinfo tschuster)]

Attachment: read

Comment 9

[Tom S. (please needinfo tschuster)]

Attachment: execute

Comment 10

[Tom S. (please needinfo tschuster)]

Attachment: InstallXBLField

Comment 11

[Tom S. (please needinfo tschuster)]

Attachment: InstallAccessor

Comment 12

[Tom S. (please needinfo tschuster)]

Attachment: InstallField

Comment 13

[Tom S. (please needinfo tschuster)]

Attachment: Various stuff

Comment 14

[Tom S. (please needinfo tschuster)]

Attachment: Serialize

Comment 15

[Tom S. (please needinfo tschuster)]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=61de3d3f6f87

Comment 16

[Tom S. (please needinfo tschuster)]

Attachment: ChangeDocument v2

I dropped a method call that produced test failures. Fixed that now.

Comment 17

[Tom S. (please needinfo tschuster)]

Attachment: Init[Js]Clas v2

I am not sure why this was making problems before, probably something else that I fixed during the rewrite.

Comment 18

[Tom S. (please needinfo tschuster)]

Try run is looking good: https://tbpl.mozilla.org/?tree=Try&rev=512525c6cb2d.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 730445 [details] [diff] [review]
Lookup*

r=me

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 730446 [details] [diff] [review]
checkAccess

r=me

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 730447 [details] [diff] [review]
InstallMember

Why did you move the AutoPushJSContext?  It's not exactly clear to me why this is safe.  If you just need the JSContext before that, just get it instead of pushing, which has security-related side-effects and whatnot.

Comment 22

[Tom S. (please needinfo tschuster)]

Comment on attachment 730447 [details] [diff] [review]
InstallMember

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

::: content/xbl/src/nsXBLProtoImpl.cpp
@@ +68,5 @@
>    // class object in the bound document that represents the concrete version of this implementation.
>    // This function also has the side effect of building up the prototype implementation if it has
>    // not been built already.
>    nsCOMPtr<nsIXPConnectJSObjectHolder> holder;
> +  JS::Rooted<JSObject*> targetClassObject(cx, nullptr);

We need the context root this here.

Comment 23

[Terrence Cole [:terrence]]

Comment on attachment 730445 [details] [diff] [review]
Lookup*

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

::: content/xbl/src/nsXBLBinding.h
@@ +87,5 @@
>  
>    /*
>     * Internal version. Requires that aCx is in appropriate xbl scope.
>     */
>    bool LookupMemberInternal(JSContext* aCx, nsString& aName, JS::HandleId aNameAsId,

While you are here, might as well change this to JS::Handle<jsid>.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 730449 [details] [diff] [review]
CompileMember

>-    nsresult rv = curr->CompileMember(context, mClassName, mClassObject);
>+    nsresult rv = curr->CompileMember(context, mClassName, classObject);

Why is this needed?  If mClassObject is already rooted, seems like it should be usable here.  Or is it not rooted yet?  In that case, is it coming up?

r=me either way, I guess

Comment 25

[Boris Zbarsky [:bzbarsky]]

> We need the context root this here.

Yes, I understand that.  The point is you need the context, but do you need the modified JSContext stack and all that jazz?

Comment 26

[Terrence Cole [:terrence]]

Comment on attachment 730450 [details] [diff] [review]
read

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

I don't know what the security implications of pushing a new context are, so I'll defer to :bz for that. The rooting looks fine, assuming that change is good.

Comment 27

[Terrence Cole [:terrence]]

Comment on attachment 730450 [details] [diff] [review]
read

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

I don't know what the security implications of pushing a new context are, so I'll defer to :bz for that. The rooting looks fine, assuming that change is good.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 730450 [details] [diff] [review]
read

Don't push the cx.  Just store it in a JSContext*.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 730451 [details] [diff] [review]
execute

r=me

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 730453 [details] [diff] [review]
InstallXBLField

r=me

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 730454 [details] [diff] [review]
InstallAccessor

r=me

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 730455 [details] [diff] [review]
InstallField

r=me

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 730456 [details] [diff] [review]
Various stuff

r=me

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 730457 [details] [diff] [review]
Serialize

r=me

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 730708 [details] [diff] [review]
Init[Js]Clas v2

r=me

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 730707 [details] [diff] [review]
ChangeDocument v2

r=me

Comment 37

[Terrence Cole [:terrence]]

Comment on attachment 730447 [details] [diff] [review]
InstallMember

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

I can't speak for the context movement, but the rest looks fine to me.

Comment 38

[Tom S. (please needinfo tschuster)]

Attachment: InstallMember - v2

Comment 39

[Tom S. (please needinfo tschuster)]

Attachment: read v2

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 731334 [details] [diff] [review]
read v2

r=me

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 731333 [details] [diff] [review]
InstallMember - v2

> +  JS::Rooted<JSObject*> targetScriptObject(cx);

How did that compile?  cx is declared later....

Comment 42

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 731333 [details] [diff] [review]
InstallMember - v2

Per comment 41.

Comment 43

[Tom S. (please needinfo tschuster)]

(In reply to Boris Zbarsky (:bz) from comment #42)
> Comment on attachment 731333 [details] [diff] [review]
> InstallMember - v2
> 
> Per comment 41.

Let's just pretend this was 
> JS::Rooted<JSObject*> targetScriptObject(context->GetNativeContext());
Okay?

Comment 44

[Tom S. (please needinfo tschuster)]

Attachment: InstallMember - v3

Compiles :)

Comment 45

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 731333 [details] [diff] [review]
InstallMember - v2

r=me in that alternate universe.  ;)

Comment 46

[Tom S. (please needinfo tschuster)]

Yaay
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=969674821d1a

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0f2dc243e4c7
https://hg.mozilla.org/mozilla-central/rev/fea6d161488d
https://hg.mozilla.org/mozilla-central/rev/f7c2fc1b53c4
https://hg.mozilla.org/mozilla-central/rev/35ad506ee9ac
https://hg.mozilla.org/mozilla-central/rev/f066c15b6025
https://hg.mozilla.org/mozilla-central/rev/4bda5dd4a307
https://hg.mozilla.org/mozilla-central/rev/9cb8032812a8
https://hg.mozilla.org/mozilla-central/rev/00040dd1bf44
https://hg.mozilla.org/mozilla-central/rev/8e58ad2f288b
https://hg.mozilla.org/mozilla-central/rev/ddb51ef64845
https://hg.mozilla.org/mozilla-central/rev/1766922c611f
https://hg.mozilla.org/mozilla-central/rev/aee861088527
https://hg.mozilla.org/mozilla-central/rev/969674821d1a