Move non-DOMWindow consumers off of nsIScriptContext

RESOLVED FIXED in mozilla26

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Bobby Holley (parental leave - send mail for anything urgent), Assigned: Bobby Holley (parental leave - send mail for anything urgent))

Tracking

unspecified
mozilla26
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments, 2 obsolete attachments)

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.
Created attachment 790544 [details] [diff] [review]
Part 1 - Deserialize FastLoad scripts into the compilation scope, rather than the scope of the first document created. v1

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)
Created attachment 790545 [details] [diff] [review]
Part 2 - Inline and remove nsIScriptContext::{Serialize,Deserialize}. v1

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)
Created attachment 790546 [details] [diff] [review]
Part 3 - Be more direct in nsXULPrototypeScript::Compile. v1

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)
Created attachment 790547 [details] [diff] [review]
Part 4 - Stop passing aGlobal everywhere and make serialization consumers responsible for entering the proper compartment. v1
Attachment #790547 - Flags: review?(bugs)
Created attachment 790548 [details] [diff] [review]
Part 5 - Stop implementing nsIScriptGlobalObjectOwner in nsXULPrototypeDocument. v1
Attachment #790548 - Flags: review?(bugs)
Created attachment 790549 [details] [diff] [review]
Part 6 - Stop using nsIScriptContext for XUL prototype documents. v1

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)
Created attachment 790550 [details] [diff] [review]
Part 7 - Remove nsIScriptRuntime. v1

All this stuff is now for nsGlobalWindow's use only.
Attachment #790550 - Flags: review?(mrbkap)
Created attachment 790551 [details] [diff] [review]
Part 8 - Get rid of vestigial nsJSRuntime instance and make it a namespace. v1

We'll rename the namespace shortly.
Attachment #790551 - Flags: review?(mrbkap)
Created attachment 790552 [details] [diff] [review]
Part 9 - Make nsJSRuntime initialization infallible and do it implicitly in the nsJSContext constructor. v1

We also move the runtime out of the namespace, which requires some updates.
Attachment #790552 - Flags: review?(mrbkap)
Created attachment 790553 [details] [diff] [review]
Part 10 - Eliminate the nsJSRuntime namespace. v1
Attachment #790553 - Flags: review?(mrbkap)
https://tbpl.mozilla.org/?tree=Try&rev=4b8243332c67
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)

Updated

4 years ago
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+

Updated

4 years ago
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
Created attachment 791010 [details] [diff] [review]
Part 4 - Pass nsXULPrototypeDocuments directly through serialization and use AutoSafeJSContext at the serialization point. v1
Attachment #790547 - Attachment is obsolete: true
Attachment #791010 - Flags: review?(bugs)
Attachment #791010 - Flags: review?(bugs) → review+
Created attachment 791024 [details] [diff] [review]
Part 6 - Stop using nsIScriptContext for XUL prototype documents. v2

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+
https://tbpl.mozilla.org/?tree=Try&rev=9095963a644a
Created attachment 791606 [details] [diff] [review]
Part 9.5 - Make sure sure the nsDOMScriptObjectFactory gets instantiated. v1

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)
https://tbpl.mozilla.org/?tree=Try&rev=c1842c8f39c4

Updated

4 years ago
Attachment #791606 - Flags: review?(mrbkap) → review+
https://tbpl.mozilla.org/?tree=Try&rev=96e595792066
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=73df7c3267d0
https://hg.mozilla.org/mozilla-central/rev/7187d131a093
https://hg.mozilla.org/mozilla-central/rev/14eeaab1e668
https://hg.mozilla.org/mozilla-central/rev/a7f5e4533a8c
https://hg.mozilla.org/mozilla-central/rev/d3b0012435d2
https://hg.mozilla.org/mozilla-central/rev/dfeae8aacc8b
https://hg.mozilla.org/mozilla-central/rev/37fbe12dcb81
https://hg.mozilla.org/mozilla-central/rev/727a41553d2a
https://hg.mozilla.org/mozilla-central/rev/1c84d17d28fb
https://hg.mozilla.org/mozilla-central/rev/3d882d652812
https://hg.mozilla.org/mozilla-central/rev/40042d32e00e
https://hg.mozilla.org/mozilla-central/rev/73df7c3267d0
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26

Updated

4 years ago
Depends on: 907848
Blocks: 907848
No longer depends on: 907848
You need to log in before you can comment on or make changes to this bug.