901106 - Move non-DOMWindow consumers off of nsIScriptContext

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Bobby Holley (:bholley)]

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.

Comment 2

[Bobby Holley (:bholley)]

Attachment: 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.

Comment 3

[Bobby Holley (:bholley)]

Attachment: 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.

Comment 4

[Bobby Holley (:bholley)]

Attachment: 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.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 4 - Stop passing aGlobal everywhere and make serialization consumers responsible for entering the proper compartment. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 5 - Stop implementing nsIScriptGlobalObjectOwner in nsXULPrototypeDocument. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: 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.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 7 - Remove nsIScriptRuntime. v1

All this stuff is now for nsGlobalWindow's use only.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 8 - Get rid of vestigial nsJSRuntime instance and make it a namespace. v1

We'll rename the namespace shortly.

Comment 10

[Bobby Holley (:bholley)]

Attachment: 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.

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 10 - Eliminate the nsJSRuntime namespace. v1

Comment 12

[Bobby Holley (:bholley)]

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

Comment 13

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

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 14

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

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 15

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

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 16

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

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

Comment 17

[Bobby Holley (:bholley)]

(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?

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 790546 [details] [diff] [review]
Part 3 - Be more direct in nsXULPrototypeScript::Compile. v1

Why you need
GetScriptGlobalObject()->EnsureScriptEnvironment();
?

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 790546 [details] [diff] [review]
Part 3 - Be more direct in nsXULPrototypeScript::Compile. v1

Oh, I see. GetScriptContext does that.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

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();
{}

Comment 22

[Bobby Holley (:bholley)]

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.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

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"

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

(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

Comment 28

[Bobby Holley (:bholley)]

Attachment: Part 4 - Pass nsXULPrototypeDocuments directly through serialization and use AutoSafeJSContext at the serialization point. v1

Comment 29

[Bobby Holley (:bholley)]

Attachment: 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.

Comment 30

[Bobby Holley (:bholley)]

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

Comment 31

[Bobby Holley (:bholley)]

Attachment: 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.

Comment 32

[Bobby Holley (:bholley)]

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

Comment 33

[Bobby Holley (:bholley)]

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

Comment 34

[Bobby Holley (:bholley)]

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

Comment 35

[Ed Morley [:emorley]]

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