Closed Bug 901106 Opened 6 years ago Closed 6 years ago

Move non-DOMWindow consumers off of nsIScriptContext

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(11 files, 2 obsolete files)

1.47 KB, patch
smaug
: review+
Details | Diff | Splinter Review
8.17 KB, patch
smaug
: review+
Details | Diff | Splinter Review
7.90 KB, patch
smaug
: review+
Details | Diff | Splinter Review
6.43 KB, patch
smaug
: review+
Details | Diff | Splinter Review
21.03 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
9.60 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
13.61 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
15.04 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
26.11 KB, patch
smaug
: review+
Details | Diff | Splinter Review
11.39 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.91 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
Right now, things like XUL and XBL prototype compilation scopes use nsIScriptContext. This is primarily because a lot of our APIs were designed around using nsIScriptContext to compile and evaluate script.

This presents a roadblock for decxification work, because it means we can't assume a 1-to-1 correspondence between nsJSContexts and outer windows (even though, during normal execution, that's effectively what we have).

So I think the best course of action here is to move all of the generally-useful machinery in nsJSContext into nsJSUtils, and invoke it directly (with the SafeJSContext) from the compilation scopes.
Depends on: 901162
Depends on: 902718
No longer depends on: 901162
Depends on: 903212
The heavy XBL work was done in the dependent bugs. All that remains here is to fix the XUL prototype cache and perhaps some other miscellaneous things, and then we can do this.
This code dates back to 2003, and smaug and I both think it's probably a bug.
In fact, it might cause leaks, because the prototype document would hold the
script alive, the script would hold the document alive, and the document would
hold the prototype document alive in the cache.
Attachment #790544 - Flags: review?(bugs)
For now we hoist the AutoPushJSContext out of nsJSContext, but eventually we'll
remove it, relying on the caller to ensure that the stack-top cx is in the
compartment of the compilation scope. This is a difficult invariant to enforce
statically, so let's proactively add an assertion here.
Attachment #790545 - Flags: review?(bugs)
Given that this is just the phony compilation global we're dealing with, the
only thing relevant about this script context is that it defaults to the right
compartment. But we can do that manually with the SafeJSContext.
Attachment #790546 - Flags: review?(bugs)
This patch is roughly analagous to what I did for XBL compilation scopes in
https://hg.mozilla.org/mozilla-central/rev/26898c90f9cc.
Attachment #790549 - Flags: review?(bugs)
All this stuff is now for nsGlobalWindow's use only.
Attachment #790550 - Flags: review?(mrbkap)
We'll rename the namespace shortly.
Attachment #790551 - Flags: review?(mrbkap)
We also move the runtime out of the namespace, which requires some updates.
Attachment #790552 - Flags: review?(mrbkap)
Comment on attachment 790548 [details] [diff] [review]
Part 5 - Stop implementing nsIScriptGlobalObjectOwner in nsXULPrototypeDocument. v1

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

::: content/xul/document/src/nsXULPrototypeDocument.cpp
@@ +637,5 @@
>  JSObject*
>  nsXULPrototypeDocument::GetCompilationGlobal()
>  {
> +  if (!mGlobalObject)
> +      mGlobalObject = NewXULPDGlobalObject();

{}
Comment on attachment 790549 [details] [diff] [review]
Part 6 - Stop using nsIScriptContext for XUL prototype documents. v1

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

::: content/xul/document/src/nsXULPrototypeDocument.cpp
@@ +746,5 @@
> +  NS_HOLD_JS_OBJECTS(this, nsXULPDGlobalObject);
> +
> +  // Add an owning reference from JS back to us. This'll be
> +  // released when the JSObject is finalized.
> +  ::JS_SetPrivate(mJSObject, this);

Drop the ::

@@ +775,4 @@
>  {
> +  mDestroyed = true;
> +  if (!mJSObject)
> +    return;

{}
Comment on attachment 790552 [details] [diff] [review]
Part 9 - Make nsJSRuntime initialization infallible and do it implicitly in the nsJSContext constructor. v1

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

<3

::: dom/base/nsJSEnvironment.cpp
@@ +2762,4 @@
>  {
>    if (sIsInitialized) {
>      if (!nsContentUtils::XPConnect())
> +      MOZ_CRASH();

{}
Comment on attachment 790553 [details] [diff] [review]
Part 10 - Eliminate the nsJSRuntime namespace. v1

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

::: dom/base/nsJSEnvironment.cpp
@@ +2584,5 @@
>    ++sLikelyShortLivingObjectsNeedingGC;
>  }
>  
>  void
> +mozilla::dom::StartupJSEnvironment()

Drop the prefixes, please
(In reply to :Ms2ger from comment #16)
> >  void
> > +mozilla::dom::StartupJSEnvironment()
> 
> Drop the prefixes, please

And put them in a two namespace { } blocks? Why is that preferred?
Attachment #790544 - Flags: review?(bugs) → review+
Attachment #790545 - Flags: review?(bugs) → review+
Comment on attachment 790546 [details] [diff] [review]
Part 3 - Be more direct in nsXULPrototypeScript::Compile. v1

Why you need
GetScriptGlobalObject()->EnsureScriptEnvironment();
?
Comment on attachment 790546 [details] [diff] [review]
Part 3 - Be more direct in nsXULPrototypeScript::Compile. v1

Oh, I see. GetScriptContext does that.
Attachment #790546 - Flags: review?(bugs) → review+
Comment on attachment 790547 [details] [diff] [review]
Part 4 - Stop passing aGlobal everywhere and make serialization consumers responsible for entering the proper compartment. v1

I would prefer pushing autocompartment thing (which is so easy to forget) 
as close as possible to actual JS API usage.
Doing it somewhere way higher up in the stack is error prone.
Attachment #790547 - Flags: review?(bugs) → review-
Comment on attachment 790548 [details] [diff] [review]
Part 5 - Stop implementing nsIScriptGlobalObjectOwner in nsXULPrototypeDocument. v1


> nsXULPrototypeDocument::GetCompilationGlobal()
> {
>-  GetScriptGlobalObject()->EnsureScriptEnvironment();
>-  return GetScriptGlobalObject()->GetGlobalJSObject();
>+  if (!mGlobalObject)
>+      mGlobalObject = NewXULPDGlobalObject();
{}
Attachment #790548 - Flags: review?(bugs) → review+
Comment on attachment 790547 [details] [diff] [review]
Part 4 - Stop passing aGlobal everywhere and make serialization consumers responsible for entering the proper compartment. v1

> I would prefer pushing autocompartment thing (which is so easy to forget) 
> as close as possible to actual JS API usage.
> Doing it somewhere way higher up in the stack is error prone.

We won't forget it, because of the assertions in comment 3. From a design standpoint, those assertions are equivalent to having the compartment entering happen before the JSAPI call - it just allows us to avoid passing the actual nsXULPrototypeDocument all the way down through the 12 different serialization overloads, which I really don't want to do.
Attachment #790547 - Flags: review- → review?(bugs)
Comment on attachment 790549 [details] [diff] [review]
Part 6 - Stop using nsIScriptContext for XUL prototype documents. v1

Hmm, does this cause nsXULPDGlobalObject to show up in the CC graph all the time.
That might be rather bad for CC times.

Since you probably have a build with these patches, could you try
the addon from bug 726346 (restart after installing).
Run CC few times to so that the graph size stabilizes and try to see if there are
nsXULPDGlobalObject objects there (and if there are, you could  copy the address and
paste it to "Find graph" to see how big subgraph it is possibly keeping alive).

re-ask review if graph size doesn't increase because of this patch - well, just having few nsXULPDGlobalObject objects without
edges from them might be ok too.
Attachment #790549 - Flags: review?(bugs)
Attachment #790550 - Flags: review?(mrbkap) → review+
Comment on attachment 790547 [details] [diff] [review]
Part 4 - Stop passing aGlobal everywhere and make serialization consumers responsible for entering the proper compartment. v1

I would still like to see the compartment handling to be hidden
from the API users.
Attachment #790547 - Flags: review?(bugs) → review-
Comment on attachment 790551 [details] [diff] [review]
Part 8 - Get rid of vestigial nsJSRuntime instance and make it a namespace. v1

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

::: dom/base/nsGlobalWindow.cpp
@@ +1806,5 @@
>  
>    NS_ASSERTION(!GetCurrentInnerWindowInternal(),
>                 "mJSObject is null, but we have an inner window?");
>  
> +  // Ensure that nsJSRuntime has been initialized. This is indempotent.

"idempotent"
Attachment #790551 - Flags: review?(mrbkap) → review+
Comment on attachment 790552 [details] [diff] [review]
Part 9 - Make nsJSRuntime initialization infallible and do it implicitly in the nsJSContext constructor. v1

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

::: dom/base/nsJSEnvironment.h
@@ +89,5 @@
>      IncrementalGC,
>      NonIncrementalGC
>    };
> +  //
> +  // Setup all the statics etc - safe to call multiple times after Startup()

Uber-nit: get rid of the comment marker on the first line.
Attachment #790552 - Flags: review?(mrbkap) → review+
Attachment #790553 - Flags: review?(mrbkap) → review+
(In reply to Olli Pettay [:smaug] from comment #23)
> Since you probably have a build with these patches, could you try
> the addon from bug 726346 (restart after installing).
> Run CC few times to so that the graph size stabilizes and try to see if
> there are
> nsXULPDGlobalObject objects there (and if there are, you could  copy the
> address and
> paste it to "Find graph" to see how big subgraph it is possibly keeping
> alive).
after installing the addon, restart, and load about:cc
Attachment #791010 - Flags: review?(bugs) → review+
According to CC dumps, the gray unmarking during MarkInCCGeneration prevents this
stuff from being traversed during CC.
Attachment #790549 - Attachment is obsolete: true
Attachment #791024 - Flags: review?(bugs)
Attachment #791024 - Flags: review?(bugs) → review+
This used to happen implicitly when we created the first nsJSContext, but that
goes away in the next patch. Without this, we get a couple of odd exception-
related failures in the IPC indexedDB tests.
Attachment #791606 - Flags: review?(mrbkap)
Attachment #791606 - Flags: review?(mrbkap) → review+
Depends on: 907848
Blocks: 907848
No longer depends on: 907848
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.