1480244 - Get rid of MessageManager globals

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, enhancement)

Description

[Kris Maglione [:kmag]]

Current measurements suggest that getting rid of message manager globals will save about 100K per message manager. For a base content process with one tab, that's one for the process script global, one for the content frame message manager.

However, I suspect that number will be significantly higher when we take wrapper overhead, which is difficult to quantify, into account.

Once bug 1472491 lands, most of our major frame scripts will just be stubs, that delegate actual work to actors loaded from JSMs. Most of the remaining frame scripts are only used by tests.

Given that, I'm leaning towards an initial step of continuing to support frame scripts, but loading them into NSVOs on top of MessageManager objects created in the shared JSM global. That will probably be *slightly* slower than the current NSVO-in-MessageManager-global approach, but it will also make JSM message manager access much faster, and will mostly affect only tests, so I think it should be fine.

Longer term, we should aim to convert the remaining frame scripts into explicit actors, but that can happen gradually. The biggest wins will come from getting rid of the globals, and moving the majority of frame script code to lazily-instantiated JSMs.

Comment 1

[Kris Maglione [:kmag]]

Actually, upon further consideration, we can probably just drop the NSVOs at that point. Given the number of consumers, they probably won't actually be all that useful, and getting rid of them will allow us to also get rid of a lot of complexity in the non-syntactic scope handling code.

Comment 2

[Kris Maglione [:kmag]]

Quick hack says this saves closer to 700K.

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1480244: Part 1a - Rename ProcessGlobal to ContentProcessMessageManager.

After these patches, these objects will no longer be globals, which would make
their current names misleading. These three patches give the bindings which
will cease being globals more appropriate names.

Review commit: https://reviewboard.mozilla.org/r/261392/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261392/

Comment 4

[Kris Maglione [:kmag]]

Attachment: Bug 1480244: Part 1b - Rename nsInProcessTabChildGlobal to InProcessTabChildMessageManager.

Review commit: https://reviewboard.mozilla.org/r/261394/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261394/

Comment 5

[Kris Maglione [:kmag]]

Attachment: Bug 1480244: Part 1c - Rename TabChildGlobal to TabChildMessageManager.

Review commit: https://reviewboard.mozilla.org/r/261396/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261396/

Comment 6

[Kris Maglione [:kmag]]

Attachment: Bug 1480244: Part 2 - Replace ExecuteInGlobalAndReturnScope with ExecuteInScopeChainAndReturnNewScope.

This patch series replaces message manager globals with ordinary JS objects
which live in the shared JSM global. Once that happens,
ExecuteInGlobalAndReturnScope will no longer have useful behavior for them,
since the base global has none of the methods that they rely on, and it
provides no way to insert another plain object into the scope chain.

This patch changes the scope chain for frame scripts to instead look like:

 -+- Shared JSM global
  |
  +- NonSyntacticVariablesObject
  |
  +- WithEnvironmentObject[target=messageManager]
  |
  +- LexicalEnvironment[this=messageManager]

Where lexical assignments end up on the lexical scope, and both qualified and
unqualified assignments wind up on the NSVO.

This has some slight behavioral differences from the previous model, in that
properties defined on the message manager can mask properties on the NSVO. But
those differences are minor, and probably not worth worrying about, since
frame scripts are being deprecated as part of the Fission project.

Review commit: https://reviewboard.mozilla.org/r/261398/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261398/

Comment 7

[Kris Maglione [:kmag]]

Attachment: Bug 1480244: Part 3a - Fix non-strict-mode test code which expects `this` to be bound to its global.

A lot of the ad-hoc frame scripts we execute for tests does not run in strict
mode, and therefore has its functions' `this` objects set to the global when
they are called without a target object.

At the moment, this gives them a MessageManager global. Once message managers
become non-global objects, however, it will give them the shared JSM global,
which is not what they expect.

This patches changes scripts which rely on this to explicitly capture or set
the appropriate `this` object for their calls.

Review commit: https://reviewboard.mozilla.org/r/261400/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261400/

Comment 8

[Kris Maglione [:kmag]]

Attachment: Bug 1480244: Part 3b - Fix tests which rely on bad scoping assumptions for frame scripts.

With the new loading model for frame scripts, lexical variables defined in a
global frame script are not available to other frame scripts.

Additionally, scripts loaded into a context object by the subscript loader
should not depend on being able to access properties of the message manager as
if they were globals.

Review commit: https://reviewboard.mozilla.org/r/261402/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261402/

Comment 9

[Kris Maglione [:kmag]]

Attachment: Bug 1480244: Part 3c - Fix GC test with bad assumptions.

This failure starts showing up after these changes by chance. Some aspect of
the environment or other causes a map() call to be baseline-optimized, which
causes its IC stubs to hold its function environemnt alive, which causes it to
hold the context we're checking alive.

Forcing a shrinking GC makes the IC stub go away sooner, and therefore allows
the test to pass.

Review commit: https://reviewboard.mozilla.org/r/261404/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261404/

Comment 10

[Kris Maglione [:kmag]]

Attachment: Bug 1480244: Part 4 - Make child message managers non-global objects.

This is mostly self-explanatory. However, the patch also contains some minor
changes to frame scripts which expect to be able to call message manager
methods with a null target object, which stops working when they stop being
global objects.

Review commit: https://reviewboard.mozilla.org/r/261406/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261406/

Comment 11

[Kris Maglione [:kmag]]

Attachment: Bug 1480244: Part 5 - Run most framescripts in shared scope.

Creating non-shared scopes for frame scripts is fairly expensive. After these
changes it's even more expensive. However, many frame scripts have no use for
the shared scopes at all. Run-once scripts which execute in closures, for
instance, make no use of them. And after bug 1472491, neither do most of our
default frame scripts.

Review commit: https://reviewboard.mozilla.org/r/261408/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261408/

Comment 12

[Andrew Swan [:aswan]]

Comment on attachment 8997688 [details]
Bug 1480244: Part 3a - Fix non-strict-mode test code which expects `this` to be bound to its global.

https://reviewboard.mozilla.org/r/261400/#review268598

Comment 13

[Andrew Swan [:aswan]]

Comment on attachment 8997690 [details]
Bug 1480244: Part 3c - Fix GC test with bad assumptions.

https://reviewboard.mozilla.org/r/261404/#review268600

Rubber-stamp really, just taking Kris's explanation at face value...

Comment 14

[Andrew Swan [:aswan]]

Comment on attachment 8997689 [details]
Bug 1480244: Part 3b - Fix tests which rely on bad scoping assumptions for frame scripts.

https://reviewboard.mozilla.org/r/261402/#review268624

Comment 15

[Andrew Swan [:aswan]]

Comment on attachment 8997692 [details]
Bug 1480244: Part 5 - Run most framescripts in shared scope.

https://reviewboard.mozilla.org/r/261408/#review268632

Everything in here makes sense (except the line comment below) but I don't think I have enough background to evalute this.

::: browser/components/extensions/test/browser/browser_ext_find.js:145
(Diff revision 1)
>    await extension.unload();
>  
>    let {selectedBrowser} = gBrowser;
>  
>    let frameScriptUrl = `data:,(${frameScript}).call(this)`;
> -  selectedBrowser.messageManager.loadFrameScript(frameScriptUrl, false);
> +  selectedBrowser.messageManager.loadFrameScript(frameScriptUrl, true);

Did you mean to add a third argument instead of just changing the second?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8997684 [details]
Bug 1480244: Part 1a - Rename ProcessGlobal to ContentProcessMessageManager.

https://reviewboard.mozilla.org/r/261392/#review268636

::: commit-message-a05ad:4
(Diff revision 1)
> +Bug 1480244: Part 1a - Rename ProcessGlobal to ContentProcessMessageManager. r?bz
> +
> +After these patches, these objects will no longer be globals, which would make
> +their current names misleading. These three patches give the bindings which

"These three patches" == parts 1a,b,c, right?  Might want to clarify that.

::: dom/base/ContentProcessMessageManager.h:9
(Diff revision 1)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> -#ifndef mozilla_dom_ProcessGlobal_h
> -#define mozilla_dom_ProcessGlobal_h
> +#ifndef mozilla_dom_ContentProcessMessageManager_h
> +#define mozilla_dom_ContentProcessMessageManager_h
>  

It would be nice to have a doc comment here describing the purpose of this class.  I know its lack is preexisting...

::: dom/base/nsCCUncollectableMarker.cpp:140
(Diff revision 1)
>  
>  static void
>  MarkMessageManagers()
>  {
>    if (nsFrameMessageManager::GetChildProcessManager()) {
> -    // ProcessGlobal's MarkForCC marks also ChildProcessManager.
> +    // ContentProcessMessageManager's MarkForCC marks also ChildProcessManager.

"also marks"

::: dom/base/nsContentUtils.cpp:217
(Diff revision 1)
>  #include "nsIWindowProvider.h"
>  #include "nsWrapperCacheInlines.h"
>  #include "nsXULPopupManager.h"
>  #include "xpcprivate.h" // nsXPConnect
>  #include "HTMLSplitOnSpacesTokenizer.h"
> +#include "InProcessTabChildMessageManager.h"

This should be in 1b, right?

::: dom/base/nsWrapperCache.h:19
(Diff revision 1)
>  #include "js/RootingAPI.h"
>  #include "js/TracingAPI.h"
>  
>  namespace mozilla {
>  namespace dom {
> +class ContentProcessMessageManager;

Followup to remove this stuff?  It's leftover from when we needed to friend these classes...

Comment 17

[Kris Maglione [:kmag]]

Comment on attachment 8997684 [details]
Bug 1480244: Part 1a - Rename ProcessGlobal to ContentProcessMessageManager.

https://reviewboard.mozilla.org/r/261392/#review268636

> This should be in 1b, right?

Oh, yeah. I had to change this after I rebased on top of your nsIContentFrameMessageManager patches, and it wound up in the wrong part.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8997685 [details]
Bug 1480244: Part 1b - Rename nsInProcessTabChildGlobal to InProcessTabChildMessageManager.

https://reviewboard.mozilla.org/r/261394/#review268680

::: commit-message-2be10:1
(Diff revision 1)
> +Bug 1480244: Part 1b - Rename nsIInProcessTabChildGlobal to InProcessTabChildMessageManager. r?bz

nsInProcessTabChildGlobal, not nsIInProcessTabChildGlobal

::: dom/base/nsFrameLoader.h:109
(Diff revision 1)
>    nsresult ReallyStartLoading();
>    void StartDestroy();
>    void DestroyDocShell();
>    void DestroyComplete();
>    nsIDocShell* GetExistingDocShell() { return mDocShell; }
> -  nsInProcessTabChildGlobal* GetTabChildGlobal() const
> +  mozilla::dom::InProcessTabChildMessageManager* GetTabChildGlobal() const

We should probably rename this method too.  Separate changeset is fine.

::: dom/base/InProcessTabChildMessageManager.h:27
(Diff revision 1)
>  #include "nsIRunnable.h"
>  #include "nsIGlobalObject.h"
>  #include "nsIScriptObjectPrincipal.h"
>  #include "nsWeakReference.h"
>  
>  namespace mozilla {

Would be good to have a comment here explaining what this class is for.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8997686 [details]
Bug 1480244: Part 1c - Rename TabChildGlobal to TabChildMessageManager.

https://reviewboard.mozilla.org/r/261396/#review268682

::: dom/base/nsFrameMessageManager.cpp:812
(Diff revision 1)
>  
>        JS::Rooted<JS::Value> thisValue(cx, JS::UndefinedValue());
>  
>        if (JS::IsCallable(object)) {
>          // A small hack to get 'this' value right on content side where
> -        // messageManager is wrapped in TabChildGlobal.
> +        // messageManager is wrapped in TabChildMessageManager.

It's not clear to me what "wrapped in" means here in a world where TabChildMessageManager is not a global...

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8997691 [details]
Bug 1480244: Part 4 - Make child message managers non-global objects.

https://reviewboard.mozilla.org/r/261406/#review268820

::: dom/base/ContentFrameMessageManager.cpp:24
(Diff revision 1)
> +  JS::RootedValue val(jsapi.cx());
> +  if (!GetOrCreateDOMReflectorNoWrap(jsapi.cx(), this, &val)) {
> +    return nullptr;
> +  }
> +  MOZ_ASSERT(val.isObject());
> +  return &val.toObject();

This will assert isObject() already, so no need to separately assert it.

::: dom/base/ContentProcessMessageManager.cpp:43
(Diff revision 1)
> -  nsCOMPtr<nsIGlobalObject> service = do_GetService(NS_CHILDPROCESSMESSAGEMANAGER_CONTRACTID);
> +  nsCOMPtr<nsIMessageSender> service = do_GetService(NS_CHILDPROCESSMESSAGEMANAGER_CONTRACTID);
>    if (!service) {
>      return nullptr;
>    }
>    ContentProcessMessageManager* global = static_cast<ContentProcessMessageManager*>(service.get());
>    if (global) {

Preexisting, but "global" can't be null here, right?

::: dom/base/ContentProcessMessageManager.cpp:127
(Diff revision 1)
> +
> +  JS::RootedValue val(jsapi.cx());
> +  if (!GetOrCreateDOMReflectorNoWrap(jsapi.cx(), this, &val)) {
> +    return nullptr;
>    }
> -  return ok;
> +  MOZ_ASSERT(val.isObject());

Again, no obvious need for this assert.

::: dom/base/nsFrameMessageManager.cpp:1454
(Diff revision 1)
>  }
>  
>  bool
> -nsMessageManagerScriptExecutor::InitChildGlobalInternal(const nsACString& aID)
> +nsMessageManagerScriptExecutor::Init()
>  {
> -  AutoSafeJSContext cx;
> +  DidCreateWrapper();

It's not quite clear to me what Init() or DidCreateWrapper() has to do with wrappers...  How are they related?

::: dom/ipc/TabChild.cpp:237
(Diff revision 1)
>              rv.SuppressException();
>              return;
>          }
>      }
>  
> -    JS::Rooted<JSObject*> kungFuDeathGrip(cx, mTabChildGlobal->GetWrapper());
> +    JS::Rooted<JSObject*> kungFuDeathGrip(cx, mTabChildMessageManager->GetWrapper());

Should this be a GetOrCreateWrapper() call?  If GetWrapper() returns null, how is this useful?

::: dom/ipc/TabChild.cpp:1368
(Diff revision 1)
>    TABC_LOG("Handling double tap at %s with %p %p\n",
>      Stringify(aPoint).c_str(),
> -    mTabChildGlobal ? mTabChildGlobal->GetWrapper() : nullptr,
> -    mTabChildGlobal.get());
> +    mTabChildMessageManager ? mTabChildMessageManager->GetWrapper() : nullptr,
> +    mTabChildMessageManager.get());
>  
> -  if (!mTabChildGlobal || !mTabChildGlobal->GetWrapper()) {
> +  if (!mTabChildMessageManager || !mTabChildMessageManager->GetWrapper()) {

Again, why is GetWrapper() the right thing here, not GetOrCreateWrapper()?

Comment 21

[Kris Maglione [:kmag]]

Comment on attachment 8997691 [details]
Bug 1480244: Part 4 - Make child message managers non-global objects.

https://reviewboard.mozilla.org/r/261406/#review268820

> This will assert isObject() already, so no need to separately assert it.

Yeah, I went back and forth over this. I ended up adding the explicit assert to make it more obvious that we expect this to always be an object, and the isObject() check wasn't just missing. But I'd be just as happy to remove it.

> Preexisting, but "global" can't be null here, right?

You're right, it can't. What a weird check...

> It's not quite clear to me what Init() or DidCreateWrapper() has to do with wrappers...  How are they related?

Oh, I meant to rename that to something DidCreateScriptLoader(). It was originally called DidCreateGlobal(), but the global was never what we really cared about here.

> Should this be a GetOrCreateWrapper() call?  If GetWrapper() returns null, how is this useful?

I actaully think the line should just be removed. I couldn't think of a good reason for it to be there to begin with, so I also couldn't think of a good reason to force us to create a wrapper in order to initialize it.

It looks like those were initially added in [1], where they replaced a `nsCOMPtr<nsIXPConnectJSObjectHolder>` grip. And while the former probably made sense, I think it's just been cargo culted along since then.

[1]: https://searchfox.org/mozilla-central/diff/7a82b9da0e4574487ffd6d31be79ac13b53bc6f5/dom/ipc/TabChild.cpp#2239

> Again, why is GetWrapper() the right thing here, not GetOrCreateWrapper()?

Hm. So, the initial check was, I believe, meant to bail out if we hadn't already initialized the message manager. But clearly checking GetWrapper() does not have the same effect, since the cached wrapper can go away. I just wasn't thinking about it clearly enough. GetOrCreateWrapper() also wouldn't have any useful effect, since it will always create a wrapper unless we OOM.

So I think we just want to check `!mTabChildMessageManager` at this point.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8997691 [details]
Bug 1480244: Part 4 - Make child message managers non-global objects.

https://reviewboard.mozilla.org/r/261406/#review268820

> Oh, I meant to rename that to something DidCreateScriptLoader(). It was originally called DidCreateGlobal(), but the global was never what we really cared about here.

Something like DidCreateScriptLoader() sounds good.

> I actaully think the line should just be removed. I couldn't think of a good reason for it to be there to begin with, so I also couldn't think of a good reason to force us to create a wrapper in order to initialize it.
> 
> It looks like those were initially added in [1], where they replaced a `nsCOMPtr<nsIXPConnectJSObjectHolder>` grip. And while the former probably made sense, I think it's just been cargo culted along since then.
> 
> [1]: https://searchfox.org/mozilla-central/diff/7a82b9da0e4574487ffd6d31be79ac13b53bc6f5/dom/ipc/TabChild.cpp#2239

> I actaully think the line should just be removed.

Might be a good idea to replace it with a stack ref to the mTabChildGlobal, since we are passing it into a random can-run-script function.

> Hm. So, the initial check was, I believe, meant to bail out if we hadn't already initialized the message manager. But clearly checking GetWrapper() does not have the same effect, since the cached wrapper can go away. I just wasn't thinking about it clearly enough. GetOrCreateWrapper() also wouldn't have any useful effect, since it will always create a wrapper unless we OOM.
> 
> So I think we just want to check `!mTabChildMessageManager` at this point.

> So I think we just want to check !mTabChildMessageManager at this point.

I think that makes sense, yes.

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8997692 [details]
Bug 1480244: Part 5 - Run most framescripts in shared scope.

https://reviewboard.mozilla.org/r/261408/#review268944

Like aswan, I'm kinda confused here. We're adding a new argument to loadFrameScript, but it's not clear to me what that argument does. I went through the patches to try to find out, but I can't actually find a patch that changes the loadFrameScript function signature.

On the off-chance it was somehow included in the patches in bug 1472491, I checked there too, but no dice.

Is there a patch missing from this series that adds a new option to loadFrameScript? Or am I missing something else? Clearing review until I understand better what's happening here.

Comment 24

[Kris Maglione [:kmag]]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #23)
> Like aswan, I'm kinda confused here. We're adding a new argument to
> loadFrameScript, but it's not clear to me what that argument does. I went
> through the patches to try to find out, but I can't actually find a patch
> that changes the loadFrameScript function signature.

This isn't a new argument. It's been there for years. :) https://searchfox.org/mozilla-central/rev/f0c15db995198a1013e1c5f5b5bea54ef83f1049/dom/chrome-webidl/MessageManager.webidl#391-400

Comment 25

[Mike Conley (:mconley) (:⚙️)]

D'oh, apologies. I'll put back into my review queue.

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8997692 [details]
Bug 1480244: Part 5 - Run most framescripts in shared scope.

https://reviewboard.mozilla.org/r/261408/#review268972

Thanks for the patch - this looks fine, although if we weren't moving away from framescripts, I'd be worried about non-obvious collisions down the road with all of these scripts running in the same scope. However, because bug 1472491 is going to move us quite far along in eliminating frame scripts, and using the Actor modules to keep things scoped, I think this is fine as an intermediary step.

::: browser/components/extensions/test/browser/browser_ext_find.js:145
(Diff revision 1)
>    await extension.unload();
>  
>    let {selectedBrowser} = gBrowser;
>  
>    let frameScriptUrl = `data:,(${frameScript}).call(this)`;
> -  selectedBrowser.messageManager.loadFrameScript(frameScriptUrl, false);
> +  selectedBrowser.messageManager.loadFrameScript(frameScriptUrl, true);

I misunderstood aswan's original comment here in my previous review of the patch, but now I get it, and I have the same question - is this supposed to now be running in the global scope? If so, we need to add a new argument and not change the delayed load argument.

Comment 27

[Kris Maglione [:kmag]]

Comment on attachment 8997692 [details]
Bug 1480244: Part 5 - Run most framescripts in shared scope.

https://reviewboard.mozilla.org/r/261408/#review268972

I tried to be careful and only make this change to scripts which were unlikely to conflict. Most of the test ones were already wrapped in IIFEs. The others were the ones that I'd already reduced to stubs in bug 1472491.

> I misunderstood aswan's original comment here in my previous review of the patch, but now I get it, and I have the same question - is this supposed to now be running in the global scope? If so, we need to add a new argument and not change the delayed load argument.

Yeah, sorry, that was a typo.

Comment 28

[Ted Campbell [:tcampbell]]

Attachment: Update environment comment in spidermonkey

Part 2 needs some comment updates in js/src/vm/EnvironmentObject.h. Include this or your own phrasing of it.

Comment 29

[Ted Campbell [:tcampbell]]

Comment on attachment 8998974 [details]
Update environment comment in spidermonkey

Err.. I suck at review board apparently. You already did this!

Comment 30

[Ted Campbell [:tcampbell]]

Comment on attachment 8997687 [details]
Bug 1480244: Part 2 - Replace ExecuteInGlobalAndReturnScope with ExecuteInScopeChainAndReturnNewScope.

https://reviewboard.mozilla.org/r/261398/#review269126

I realize now that there are two different things going on here.

1) Changing the environment chain to add a WithEnvironment. This seems reasonable given all the constraints you explained.
2) Using the lexical environment cache in a novel way. If we can be more explicit about what we are doing here (comments below), I can live with this.

s/ExecuteInScopeChainAndReturnNewScope/ExecuteInFrameScriptEnvironment/
Time for us to stop pretending this is a general purpose function :)

|evalAndReturnScope| probably needs a new name. |evalFrameScriptAndReturnScope| temporarily?

::: commit-message-dffba:11
(Diff revision 1)
> +since the base global has none of the methods that they rely on, and it
> +provides no way to insert another plain object into the scope chain.
> +
> +This patch changes the scope chain for frame scripts to instead look like:
> +
> + -+- Shared JSM global

Mention the LexicalEnvironment[this=global] here (you have it in EnvironmentObjects.h already).

::: js/src/builtin/Eval.cpp:476
(Diff revision 1)
> +    RootedObject env(cx);
> +    if (!js::CreateObjectsForEnvironmentChain(cx, envChain, varEnv, &env))
> +        return false;
> +
> +    // Create lexical environment with |this| == objArg.
>      // NOTE: This is required behavior for Gecko FrameScriptLoader

Should mention that in that case, objArg will be the MessageManager.

::: js/src/vm/Realm.cpp:198
(Diff revision 1)
>  } // namespace (anonymous)
>  
>  #endif // JSGC_HASH_TABLE_CHECKS
>  
> -LexicalEnvironmentObject*
> -ObjectRealm::getOrCreateNonSyntacticLexicalEnvironment(JSContext* cx, HandleObject enclosing)
> +bool
> +ObjectRealm::createNonSyntacticLexicalEnvironments(JSContext* cx)

With setNonSyn... gone, this isn't needed anymore.

::: js/src/vm/Realm.cpp:216
(Diff revision 1)
> -        nonSyntacticLexicalEnvironments_ = std::move(map);
> +    nonSyntacticLexicalEnvironments_ = std::move(map);
> +    return true;
> +}
> +
> +LexicalEnvironmentObject*
> +ObjectRealm::getOrCreateNonSyntacticLexicalEnvironment(JSContext* cx, HandleObject enclosing)

1) We should change this to take |enclosing|, |key|, and |thisv|. Excessive but explicit.

2) getNonSyntactic... should rename it's argument to |key|.

::: js/src/vm/Realm.cpp:224
(Diff revision 1)
>      // If a wrapped WithEnvironmentObject was passed in, unwrap it, as we may
>      // be creating different WithEnvironmentObject wrappers each time.
>      RootedObject key(cx, enclosing);
>      if (enclosing->is<WithEnvironmentObject>()) {
>          MOZ_ASSERT(!enclosing->as<WithEnvironmentObject>().isSyntactic());
>          key = &enclosing->as<WithEnvironmentObject>().object();

Let's pull this out to the callers since they are the ones that make the justifications about keys.
The comment about |thisv| below can probably be hoisted out to specific functions or removed.

::: js/src/vm/Realm.cpp:275
(Diff revision 1)
>          return nullptr;
>      return &lexicalEnv->as<LexicalEnvironmentObject>();
>  }
>  
>  bool
> +ObjectRealm::setNonSyntacticLexicalEnvironment(JSContext* cx, JSObject* enclosing, JSObject* env)

Let's remove this and fix getOrCreateNonSyntacticLexicalEnvironment instead.

Comment 31

[Kris Maglione [:kmag]]

Comment on attachment 8997684 [details]
Bug 1480244: Part 1a - Rename ProcessGlobal to ContentProcessMessageManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261392/diff/1-2/

Comment 32

[Kris Maglione [:kmag]]

Comment on attachment 8997685 [details]
Bug 1480244: Part 1b - Rename nsInProcessTabChildGlobal to InProcessTabChildMessageManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261394/diff/1-2/

Comment 33

[Kris Maglione [:kmag]]

Comment on attachment 8997686 [details]
Bug 1480244: Part 1c - Rename TabChildGlobal to TabChildMessageManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261396/diff/1-2/

Comment 34

[Kris Maglione [:kmag]]

Comment on attachment 8997687 [details]
Bug 1480244: Part 2 - Replace ExecuteInGlobalAndReturnScope with ExecuteInScopeChainAndReturnNewScope.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261398/diff/1-2/

Comment 35

[Kris Maglione [:kmag]]

Comment on attachment 8997688 [details]
Bug 1480244: Part 3a - Fix non-strict-mode test code which expects `this` to be bound to its global.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261400/diff/1-2/

Comment 36

[Kris Maglione [:kmag]]

Comment on attachment 8997689 [details]
Bug 1480244: Part 3b - Fix tests which rely on bad scoping assumptions for frame scripts.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261402/diff/1-2/

Comment 37

[Kris Maglione [:kmag]]

Comment on attachment 8997690 [details]
Bug 1480244: Part 3c - Fix GC test with bad assumptions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261404/diff/1-2/

Comment 38

[Kris Maglione [:kmag]]

Comment on attachment 8997691 [details]
Bug 1480244: Part 4 - Make child message managers non-global objects.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261406/diff/1-2/

Comment 39

[Kris Maglione [:kmag]]

Comment on attachment 8997692 [details]
Bug 1480244: Part 5 - Run most framescripts in shared scope.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261408/diff/1-2/

Comment 40

[Code Review Bot [:reviewbot]]

Comment on attachment 8997686 [details]
Bug 1480244: Part 1c - Rename TabChildGlobal to TabChildMessageManager.

https://reviewboard.mozilla.org/r/261396/#review269292


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/ipc/TabChild.cpp:3532
(Diff revision 2)
>  : ContentFrameMessageManager(new nsFrameMessageManager(aTabChild)),
>    mTabChild(aTabChild)
>  {
>  }
>  
> -TabChildGlobal::~TabChildGlobal()
> +TabChildMessageManager::~TabChildMessageManager()

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

TabChildMessageManager::~TabChildMessageManager()
                        ^

Comment 41

[Ted Campbell [:tcampbell]]

Comment on attachment 8997687 [details]
Bug 1480244: Part 2 - Replace ExecuteInGlobalAndReturnScope with ExecuteInScopeChainAndReturnNewScope.

https://reviewboard.mozilla.org/r/261398/#review269290

::: js/src/builtin/Eval.cpp:480
(Diff revision 2)
> +    ObjectRealm& realm = ObjectRealm::get(varEnv);
> +    RootedObject lexEnv(cx, realm.getOrCreateNonSyntacticLexicalEnvironment(cx, env, varEnv, objArg));
>      if (!lexEnv)
>          return false;

Let's match https://searchfox.org/mozilla-central/rev/088938b1495aeac586bff94234c9382a4b8ca6da/js/src/builtin/Eval.cpp#537-539 for consistency.

::: js/src/vm/Realm.cpp:231
(Diff revision 2)
> +
> +    return &lexicalEnv->as<LexicalEnvironmentObject>();
> +}
> +
> +LexicalEnvironmentObject*
> +ObjectRealm::getOrCreateNonSyntacticLexicalEnvironment(JSContext* cx, HandleObject enclosing)

Thanks for refactoring this.

Comment 42

[Kris Maglione [:kmag]]

Comment on attachment 8997691 [details]
Bug 1480244: Part 4 - Make child message managers non-global objects.

https://reviewboard.mozilla.org/r/261406/#review268820

> > I actaully think the line should just be removed.
> 
> Might be a good idea to replace it with a stack ref to the mTabChildGlobal, since we are passing it into a random can-run-script function.

Well, I tried this, and it worked fine locally, but apparently our clang plugin forbids kungFuDeathGrips created from RefPtr member functions.

Comment 43

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee6026ef734262f58502034ecff213603ffdb2b4
Bug 1480244: Part 0 - Fix unified build bustage. r=me

https://hg.mozilla.org/integration/mozilla-inbound/rev/bfd2f668e16cf0794f06e0349185c47a88a4f0b4
Bug 1480244: Part 1a - Rename ProcessGlobal to ContentProcessMessageManager. r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/6760912e7b0330c082654ed2c3039321150fde9b
Bug 1480244: Part 1b - Rename nsInProcessTabChildGlobal to InProcessTabChildMessageManager. r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/2aac9238b59e5e7b1422d82d7f71e3a001465300
Bug 1480244: Part 1c - Rename TabChildGlobal to TabChildMessageManager. r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/809dc9048fea945398495e575a7d7cc9f71fd5cc
Bug 1480244: Part 2 - Replace ExecuteInGlobalAndReturnScope with ExecuteInScopeChainAndReturnNewScope. r=tcampbell

https://hg.mozilla.org/integration/mozilla-inbound/rev/d17af8236abe8b1a53674955d2baee603abc2b62
Bug 1480244: Part 3a - Fix non-strict-mode test code which expects `this` to be bound to its global. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/7aa3ada140f74e48947a4c69a1fd53b84b39a51e
Bug 1480244: Part 3b - Fix tests which rely on bad scoping assumptions for frame scripts. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d6c6e052339d90767d18403826f0a316521074f
Bug 1480244: Part 3c - Fix GC test with bad assumptions. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7a263321e999b1806b8facd31177ca026a7a33f
Bug 1480244: Part 4 - Make child message managers non-global objects. r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/b712d41d474554d13447594ebc0ce8b2cccd0f47
Bug 1480244: Part 5 - Run most framescripts in shared scope. r=mconley

Comment 44

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e72eac515afc77cfc9be68b3a33020cb79330eb9
Bug 1480244: Follow-up: Fix more unified bustage in debug fuzzing build. r=bustage

Comment 45

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7cd543332d858e28197f3e6221023df787875ba5
Bug 1480244: Follow-up: Disable racy tests on Mac and Linux. r=me,test-only DONTBUILD

Comment 46

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8997691 [details]
Bug 1480244: Part 4 - Make child message managers non-global objects.

https://reviewboard.mozilla.org/r/261406/#review268820

> Well, I tried this, and it worked fine locally, but apparently our clang plugin forbids kungFuDeathGrips created from RefPtr member functions.

Hrm.  What's the error message it gave?

Comment 47

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] (vacation Aug 16-27) from comment #46)
> > Well, I tried this, and it worked fine locally, but apparently our clang plugin forbids kungFuDeathGrips created from RefPtr member functions.
> 
> Hrm.  What's the error message it gave?

/builds/worker/workspace/build/src/dom/ipc/TabChild.cpp:236:5: error: Unused "kungFuDeathGrip" 'RefPtr<mozilla::dom::TabChildMessageManager>' objects constructed from members are prohibited

Comment 48

[Boris Zbarsky [:bzbarsky]]

Oh, right, the unused bit is key.

What you'd want is:


    RefPtr<TabChildMessageManager> kungFuDeathGrip = mTabChildMessageManager;
    // Let the BrowserElementScrolling helper (if it exists) for this
    // content manipulate the frame state.
    RefPtr<nsFrameMessageManager> mm = mTabChildMessageManager->GetMessageManager();
    mm->ReceiveMessage(static_cast<EventTarget*>(kungFuDeathGrip), ...)

etc, to make it clear why we're holding the grip: because we want to stay alive across the call to ReceiveMessage().

Comment 49

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ee6026ef7342
https://hg.mozilla.org/mozilla-central/rev/bfd2f668e16c
https://hg.mozilla.org/mozilla-central/rev/6760912e7b03
https://hg.mozilla.org/mozilla-central/rev/2aac9238b59e
https://hg.mozilla.org/mozilla-central/rev/809dc9048fea
https://hg.mozilla.org/mozilla-central/rev/d17af8236abe
https://hg.mozilla.org/mozilla-central/rev/7aa3ada140f7
https://hg.mozilla.org/mozilla-central/rev/7d6c6e052339
https://hg.mozilla.org/mozilla-central/rev/c7a263321e99
https://hg.mozilla.org/mozilla-central/rev/b712d41d4745
https://hg.mozilla.org/mozilla-central/rev/e72eac515afc
https://hg.mozilla.org/mozilla-central/rev/7cd543332d85

Comment 50

[Joel Maher ( :jmaher ) (UTC -8)]

this shows some talos improvements:
== Change summary for alert #14974 (as of Tue, 14 Aug 2018 22:54:25 GMT) ==

Improvements:

  6%  tps linux64-qr opt e10s stylo     10.30 -> 9.64
  6%  tabpaint windows10-64-qr opt e10s stylo51.02 -> 47.99
  6%  tabpaint windows10-64-qr opt e10s stylo50.51 -> 47.69
  2%  cpstartup content-process-startup linux64 pgo e10s stylo168.61 -> 165.23

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=14974

Comment 51

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/376ec8be8307c9bf77ce1fee4c08f79aec3e7404
Bug 1480244: Follow-up: Re-add kungFuDeathGrip for mTabChildMessageManager. r=bz

Comment 52

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] (vacation Aug 16-27) from comment #48)
> Oh, right, the unused bit is key.
> 
> What you'd want is:
> 
> 
>     RefPtr<TabChildMessageManager> kungFuDeathGrip = mTabChildMessageManager;
>     // Let the BrowserElementScrolling helper (if it exists) for this
>     // content manipulate the frame state.
>     RefPtr<nsFrameMessageManager> mm =
> mTabChildMessageManager->GetMessageManager();
>     mm->ReceiveMessage(static_cast<EventTarget*>(kungFuDeathGrip), ...)
> 
> etc, to make it clear why we're holding the grip: because we want to stay
> alive across the call to ReceiveMessage().

Fixed. I also removed the comment while I was here. It was apparently copied from Embedlite, and seems to refer to something that no longer exists.

Comment 53

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/376ec8be8307

Comment 54

[Joel Maher ( :jmaher ) (UTC -8)]

we also see memory improvements with these changes:
== Change summary for alert #14973 (as of Tue, 14 Aug 2018 22:54:25 GMT) ==

Improvements:

 10%  Base Content JS linux64 opt stylo     5,996,628.00 -> 5,423,402.67
  9%  Base Content JS linux64-qr opt stylo  5,957,305.33 -> 5,431,964.00
  4%  JS windows7-32 pgo stylo              74,308,955.21 -> 71,222,007.98
  4%  Base Content Explicit linux64 opt stylo18,096,128.00 -> 17,359,360.00
  4%  Base Content Explicit linux64-qr opt stylo18,076,160.00 -> 17,377,450.67

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=14973