Closed
Bug 1480244
Opened 7 years ago
Closed 7 years ago
Get rid of MessageManager globals
Categories
(Core :: DOM: Content Processes, enhancement)
Core
DOM: Content Processes
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: kmag, Assigned: kmag)
References
(Blocks 1 open bug)
Details
(Whiteboard: [overhead:700K])
Attachments
(9 files, 1 obsolete file)
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
tcampbell
:
review+
|
Details |
Bug 1480244: Part 3a - Fix non-strict-mode test code which expects `this` to be bound to its global.
59 bytes,
text/x-review-board-request
|
aswan
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
aswan
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
aswan
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mconley
:
review+
|
Details |
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.
Assignee | ||
Comment 1•7 years ago
|
||
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.
Assignee | ||
Comment 2•7 years ago
|
||
Quick hack says this saves closer to 700K.
Whiteboard: [overhead:>250K] → [overhead:700K]
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 12•7 years ago
|
||
mozreview-review |
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
Attachment #8997688 -
Flags: review?(aswan) → review+
Comment 13•7 years ago
|
||
mozreview-review |
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...
Attachment #8997690 -
Flags: review?(aswan) → review+
Comment 14•7 years ago
|
||
mozreview-review |
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
Attachment #8997689 -
Flags: review?(aswan) → review+
Comment 15•7 years ago
|
||
mozreview-review |
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?
Attachment #8997692 -
Flags: review?(aswan)
![]() |
||
Comment 16•7 years ago
|
||
mozreview-review |
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...
Attachment #8997684 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 17•7 years ago
|
||
mozreview-review-reply |
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•7 years ago
|
||
mozreview-review |
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.
Attachment #8997685 -
Flags: review?(bzbarsky) → review+
![]() |
||
Comment 19•7 years ago
|
||
mozreview-review |
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...
Attachment #8997686 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Updated•7 years ago
|
Attachment #8997692 -
Flags: review?(mconley)
![]() |
||
Comment 20•7 years ago
|
||
mozreview-review |
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()?
Attachment #8997691 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 21•7 years ago
|
||
mozreview-review-reply |
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•7 years ago
|
||
mozreview-review-reply |
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•7 years ago
|
||
mozreview-review |
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.
Attachment #8997692 -
Flags: review?(mconley)
Assignee | ||
Comment 24•7 years ago
|
||
(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•7 years ago
|
||
D'oh, apologies. I'll put back into my review queue.
Updated•7 years ago
|
Attachment #8997692 -
Flags: review?(mconley)
Comment 26•7 years ago
|
||
mozreview-review |
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.
Attachment #8997692 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 27•7 years ago
|
||
mozreview-review-reply |
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•7 years ago
|
||
Part 2 needs some comment updates in js/src/vm/EnvironmentObject.h. Include this or your own phrasing of it.
Comment 29•7 years ago
|
||
Comment on attachment 8998974 [details]
Update environment comment in spidermonkey
Err.. I suck at review board apparently. You already did this!
Attachment #8998974 -
Attachment is obsolete: true
Comment 30•7 years ago
|
||
mozreview-review |
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.
Attachment #8997687 -
Flags: review?(tcampbell)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 40•7 years ago
|
||
mozreview-review |
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•7 years ago
|
||
mozreview-review |
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.
Attachment #8997687 -
Flags: review?(tcampbell) → review+
Assignee | ||
Comment 42•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 43•7 years ago
|
||
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
Assignee | ||
Comment 44•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e72eac515afc77cfc9be68b3a33020cb79330eb9
Bug 1480244: Follow-up: Fix more unified bustage in debug fuzzing build. r=bustage
Assignee | ||
Comment 45•7 years ago
|
||
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•7 years ago
|
||
mozreview-review-reply |
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?
Assignee | ||
Comment 47•7 years ago
|
||
(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•7 years ago
|
||
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•7 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment 50•6 years ago
|
||
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
Assignee | ||
Comment 51•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/376ec8be8307c9bf77ce1fee4c08f79aec3e7404
Bug 1480244: Follow-up: Re-add kungFuDeathGrip for mTabChildMessageManager. r=bz
Assignee | ||
Comment 52•6 years ago
|
||
(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•6 years ago
|
||
bugherder |
Comment 54•6 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•