902718 - Move XBL off of nsIScriptContext

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Bobby Holley (:bholley)]

This is the first part of bug 901106. I have working patches.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Defer XPCOM release from the XBL compilation scope finalizer. v1

Apparently the current setup is incidentally safe, but far from kosher.

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Add a mechanism to assert that the stack-top cx is an XBL compilation scope. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Stop taking an nsIScriptContext in XBL serialization routines. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 4 - Switch to nsJUtils::EvaluateString in InstallField. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 5 - Rip out nsIScriptContext from the lion's share of XBL code. v1

This kind of has to be done all at once. The primary correctness concern here is
making sure that we continue to operate in the correct compartment. So the basic
strategy here is as follows.

For most anything that previously took a script context, we remove the parameter
entirely and assert that the stack-top cx is in the compartment of the
compilation scope. We then bubble this up through all the callers until we hit
a caller that finds the scx via some means other than its parameter list. At
these points, we push the scx, making it stack-top.

This puts us in a good position to switch wholesale to the SafeJSContext in
subsequent patches.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 6 - Fix up nsXBLDocGlobalObject::GetGlobalJSObject() to force lazy global creation. v1

Currently we always pull the global off the nsIScriptContext, and trigger
a call to EnsureScriptEnvironment in the GetContext() call. So as we move away
from nsIScriptContext, let's make sure that GetGlobalJSObject continues to work
correctly.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 7 - Use the SafeJSContext for XBL serialization and deserialization. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 8 - Use the SafeJSContext for prototype member compilation. v1

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 9 - Stop using nsIScriptGlobalObjectOwner to grab the compilation global. v1

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 10 - Remove nsIScriptGlobalObjectOwner from XBL. v1

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 11 - Stop using nsIScriptContext for XBL compilation. v1

Flagging bz for general review, mccr8 on the new CC setup.

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 12 - Fold EnsureScriptEnvironment into GetCompilationGlobal. v1

Comment 13

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=c4ce62a2b82b

Comment 14

[Andrew McCreight [:mccr8]]

Comment on attachment 787279 [details] [diff] [review]
Part 11 - Stop using nsIScriptContext for XBL compilation. v1

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

r=me on the CC stuff

::: content/xbl/src/nsXBLDocumentInfo.cpp
@@ +57,5 @@
>  
>    static JSBool doCheckAccess(JSContext *cx, JS::Handle<JSObject*> obj,
>                                JS::Handle<jsid> id, uint32_t accessType);
>  
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(nsXBLDocGlobalObject)

Please move this up after to the NS_DECL_CYCLE_COLLECTING_ISUPPORTS so it is easier to find.

@@ +353,5 @@
>    if (tmp->mBindingTable) {
>      tmp->mBindingTable->Enumerate(TraverseProtos, &cb);
>    }
>    NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "mGlobalObject");
> +  cb.NoteXPCOMChild(tmp->mGlobalObject);

Please replace this line and the previous NOTE_EDGE_NAME one with just
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mGlobalObject)
(This could have been done before your patches, too.)

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 787270 [details] [diff] [review]
Part 2 - Add a mechanism to assert that the stack-top cx is an XBL compilation scope. v1

r=me

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 787271 [details] [diff] [review]
Part 3 - Stop taking an nsIScriptContext in XBL serialization routines. v1

r=me

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 787272 [details] [diff] [review]
Part 4 - Switch to nsJUtils::EvaluateString in InstallField. v1

>-  AutoPushJSContext cx(aContext->GetNativeContext());
>+  AutoJSContext cx;

Why is this change ok?

>+  rv = nsJSUtils::EvaluateString(cx, nsDependentString(mFieldText,
>+                                                       mFieldTextLength),
>+                               wrappedNode, compileOptions, evalOptions,
>                                result.address());

Please fix the indent.  

Again, is this going to not screw up the operation callback stuff?

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 787274 [details] [diff] [review]
Part 6 - Fix up nsXBLDocGlobalObject::GetGlobalJSObject() to force lazy global creation. v1

r=me

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 787275 [details] [diff] [review]
Part 7 - Use the SafeJSContext for XBL serialization and deserialization. v1

r=me

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 787276 [details] [diff] [review]
Part 8 - Use the SafeJSContext for prototype member compilation. v1

r-me

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 787280 [details] [diff] [review]
Part 12 - Fold EnsureScriptEnvironment into GetCompilationGlobal. v1

r=me

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 787277 [details] [diff] [review]
Part 9 - Stop using nsIScriptGlobalObjectOwner to grab the compilation global. v1

r=me

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 787278 [details] [diff] [review]
Part 10 - Remove nsIScriptGlobalObjectOwner from XBL. v1

>-  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIScriptGlobalObjectOwner)

This needs to become:

   NS_INTERFACE_MAP_ENTRY(nsISupports)

r=me with that

Comment 24

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

Comment on attachment 787270 [details] [diff] [review]
Part 2 - Add a mechanism to assert that the stack-top cx is an XBL compilation scope. v1

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

::: content/xbl/src/nsXBLDocumentInfo.h
@@ +72,5 @@
>  
> +#ifdef DEBUG
> +void AssertInCompilationScope();
> +#else
> +#define AssertInCompilationScope() {}

This looks fishy... Should probably be

  void AssertInCompilationScope() {}

or

  #define AssertInCompilationScope()

Comment 25

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

Comment on attachment 787272 [details] [diff] [review]
Part 4 - Switch to nsJUtils::EvaluateString in InstallField. v1

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

::: content/xbl/src/nsXBLProtoImplField.cpp
@@ +426,5 @@
> +                .setVersion(JSVERSION_LATEST);
> +  nsJSUtils::EvaluateOptions evalOptions;
> +  rv = nsJSUtils::EvaluateString(cx, nsDependentString(mFieldText,
> +                                                       mFieldTextLength),
> +                               wrappedNode, compileOptions, evalOptions,

Indentation

Comment 26

[Bobby Holley (:bholley)]

Comment on attachment 787272 [details] [diff] [review]
Part 4 - Switch to nsJUtils::EvaluateString in InstallField. v1

(In reply to Boris Zbarsky (:bz) (vacation Aug 10-19) from comment #17)
> Comment on attachment 787272 [details] [diff] [review]
> Part 4 - Switch to nsJUtils::EvaluateString in InstallField. v1
> 
> >-  AutoPushJSContext cx(aContext->GetNativeContext());
> >+  AutoJSContext cx;
> 
> Why is this change ok?

You're right. I thought at the time that this was the XBL nsIScriptContext, but it's actually the one from the associated document, which makes this change incorrect and orthogonal to my goals in this bug. I'll toss this patch and rebase the next one.

Comment 27

[Bobby Holley (:bholley)]

Attachment: Part 4 & 5 - Rip out nsIScriptContext from the lion's share of XBL code. v2

Comment 28

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=076e8918aebd

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 787665 [details] [diff] [review]
Part 4 & 5 - Rip out nsIScriptContext from the lion's share of XBL code. v2

>+++ b/content/xbl/src/nsXBLProtoImplField.cpp
>@@ -402,38 +402,35 @@ nsXBLProtoImplField::InstallField(nsIScriptContext* aContext,
>-  nsCOMPtr<nsIScriptContext> context = aContext;

We're sure either caller has a strong ref or evaluation of the field can't destroy aContext for some other reason?

r=me modulo that

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 787279 [details] [diff] [review]
Part 11 - Stop using nsIScriptContext for XBL compilation. v1

Seems reasonable.

Comment 31

[Bobby Holley (:bholley)]

(In reply to Vacation until Aug 19.  Do not ask for review. from comment #29)
> We're sure either caller has a strong ref or evaluation of the field can't
> destroy aContext for some other reason?

Yeah, the caller has a strong ref.

Comment 32

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=c588b8f7b26e

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/73ead14de387
https://hg.mozilla.org/mozilla-central/rev/cc6f5065e2fa
https://hg.mozilla.org/mozilla-central/rev/dcce29a469bd
https://hg.mozilla.org/mozilla-central/rev/1c9c29452c75
https://hg.mozilla.org/mozilla-central/rev/3f5f91d29bb3
https://hg.mozilla.org/mozilla-central/rev/76069d28109b
https://hg.mozilla.org/mozilla-central/rev/aa7282f72c70
https://hg.mozilla.org/mozilla-central/rev/4a9a068669cb
https://hg.mozilla.org/mozilla-central/rev/183f357bc3eb
https://hg.mozilla.org/mozilla-central/rev/26898c90f9cc
https://hg.mozilla.org/mozilla-central/rev/c588b8f7b26e