Closed Bug 902718 Opened 7 years ago Closed 7 years ago

Move XBL off of nsIScriptContext

Categories

(Core :: XBL, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(11 files, 2 obsolete files)

1.12 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
2.08 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
8.86 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.05 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
7.86 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.77 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.18 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
6.24 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
11.36 KB, patch
bzbarsky
: review+
mccr8
: review+
Details | Diff | Splinter Review
3.16 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
38.06 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
This is the first part of bug 901106. I have working patches.
Apparently the current setup is incidentally safe, but far from kosher.
Attachment #787269 - Flags: review?(continuation)
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.
Attachment #787273 - Flags: review?(bzbarsky)
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.
Attachment #787274 - Flags: review?(bzbarsky)
Flagging bz for general review, mccr8 on the new CC setup.
Attachment #787279 - Flags: review?(continuation)
Attachment #787279 - Flags: review?(bzbarsky)
Attachment #787269 - Flags: review?(continuation) → review+
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.)
Attachment #787279 - Flags: review?(continuation) → review+
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
Attachment #787270 - Flags: review?(bzbarsky) → review+
Comment on attachment 787271 [details] [diff] [review]
Part 3 - Stop taking an nsIScriptContext in XBL serialization routines. v1

r=me
Attachment #787271 - Flags: review?(bzbarsky) → review+
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 on attachment 787274 [details] [diff] [review]
Part 6 - Fix up nsXBLDocGlobalObject::GetGlobalJSObject() to force lazy global creation. v1

r=me
Attachment #787274 - Flags: review?(bzbarsky) → review+
Comment on attachment 787275 [details] [diff] [review]
Part 7 - Use the SafeJSContext for XBL serialization and deserialization. v1

r=me
Attachment #787275 - Flags: review?(bzbarsky) → review+
Comment on attachment 787276 [details] [diff] [review]
Part 8 - Use the SafeJSContext for prototype member compilation. v1

r-me
Attachment #787276 - Flags: review?(bzbarsky) → review+
Comment on attachment 787280 [details] [diff] [review]
Part 12 - Fold EnsureScriptEnvironment into GetCompilationGlobal. v1

r=me
Attachment #787280 - Flags: review?(bzbarsky) → review+
Comment on attachment 787277 [details] [diff] [review]
Part 9 - Stop using nsIScriptGlobalObjectOwner to grab the compilation global. v1

r=me
Attachment #787277 - Flags: review?(bzbarsky) → review+
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
Attachment #787278 - Flags: review?(bzbarsky) → review+
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 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 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.
Attachment #787272 - Attachment is obsolete: true
Attachment #787272 - Flags: review?(bzbarsky)
Attachment #787273 - Attachment is obsolete: true
Attachment #787273 - Flags: review?(bzbarsky)
Attachment #787665 - Flags: review?(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
Attachment #787665 - Flags: review?(bzbarsky) → review+
Comment on attachment 787279 [details] [diff] [review]
Part 11 - Stop using nsIScriptContext for XBL compilation. v1

Seems reasonable.
Attachment #787279 - Flags: review?(bzbarsky) → review+
(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.
You need to log in before you can comment on or make changes to this bug.