888600 - Move ContentFrameMessageManager to WebIDL

Status: RESOLVED FIXED

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

When this is done, the UnwrapArg bits in GlobalObject::GetAsSupports should go away as well.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Assuming that all our globals will be Web IDL at that point, that is.  Maybe that still won't be the case.

Comment 3

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #2)
> Assuming that all our globals will be Web IDL at that point, that is.  Maybe
> that still won't be the case.

There will still be BackstagePasses and Sandboxes. The former could move to WebIDL if that makes things easier. I'm not wild about doing that for the latter.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

So is anything blocking this?  Or is this just work nobody has done?

Comment 5

[Boris Zbarsky [:bzbarsky]]

The latter.  I'd review patches here in a heartbeat!

Comment 6

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

The latter yes. It is surprisingly lots of work to convert all the message manager stuff to
use webidl,

Comment 7

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

And besides, if anyone wants to fix this, don't dump the reviews to bz. This is my mess and I should either fix this or at least review the patches. :)

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: Part 1: Add infrastructure to expose 'system' names on a WebIDL global

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: Part 2: Various test fixes to prepare for using WebIDL bindings for MessageManager classes

Comment 10

[Peter Van der Beken [:peterv]]

Attachment: Part 3: Add message manager concrete classes for WebIDL

Comment 11

[Peter Van der Beken [:peterv]]

Attachment: Part 3: Add message manager concrete classes for WebIDL

Comment 12

[Peter Van der Beken [:peterv]]

Attachment: Part 4: Convert MessageManager to WebIDL

Comment 13

[Peter Van der Beken [:peterv]]

Attachment: Part 5: Convert MessageManager globals to WebIDL

Comment 14

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=de8d38a9d444ae93d345e09deb427508b85e39c5

Comment 15

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=116e13e01bc0a79e88eb8297dcfbcf25c913be8b

Comment 16

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68ecb11011b8522ec3611322f4c6dcc500376f1a

Comment 17

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d940bba27aadda6732fe88895968d009d7837c1

Comment 18

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=16a13ff8d6806628fb6a08f0eb583851227f16ed

Comment 19

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8692c3b5300ea9d006813c392b41759e785fb5e8

Comment 20

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=50bef2fde4bd003fde2f923cd839778070fdb285

Comment 21

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f4201b03c55db0164016ba63e046230bd8e83ac

Comment 22

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=285e85209653c56bdab94f1a8b60bcd7e03aa7b7

Comment 23

[Peter Van der Beken [:peterv]]

Attachment: Part 1: Add infrastructure to expose 'system' names on a WebIDL global

Comment 24

[Peter Van der Beken [:peterv]]

Attachment: Generated ResolveSystemBinding.cpp

Comment 25

[Peter Van der Beken [:peterv]]

Attachment: Part 2: Various test fixes to prepare for using WebIDL bindings for MessageManager classes

Comment 26

[Peter Van der Beken [:peterv]]

Attachment: Part 3: Add message manager concrete classes for WebIDL

Comment 27

[Peter Van der Beken [:peterv]]

Attachment: Part 4: Convert MessageManager to WebIDL

Comment 28

[Peter Van der Beken [:peterv]]

Attachment: Part 5: Convert MessageManager globals to WebIDL

Comment 29

[Bobby Holley (:bholley)]

So exciting! :-)

Comment 30

[Peter Van der Beken [:peterv]]

Attachment: Part 6: Mark some IDL interfaces as non-scriptable

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8933699 [details] [diff] [review]
Part 1: Add infrastructure to expose 'system' names on a WebIDL global

>+class CGSystemBindingInitIds(CGAbstractStaticMethod):

Please MOZ_ASSERT mainthread in the impl, since we're using statics.

>+              for (uint32_t i = 0; i < ArrayLength(properties); ++i) {

Maybe document that we need the index to call IdString()?

> class CGResolveSystemBinding(CGAbstractMethod):

Assert mainthread, please.

>+class CGMayResolveAsSystemBindingName(CGAbstractMethod):

Again, assert mainthread.

>+            if (!idsInited) {
>+              return true;

Please document that we can't init ids here because we need to be infallible, but we can just claim that we might resolve anything and the only problem will be a performance hit.

That said, we will have that performance hit for all property access on system globals until something calls ResolveSystemBinding or GetSystemBindingNames for the first time.  Seems a bit annoying.  I wonder whether we can actually just init the ids the first time we create a system global and be done with it.  I guess that init could fail, but then we shoud fail system global creation too.

>+class CGGetSystemBindingNames(CGAbstractMethod):

Please assert mainthread.

>+            if (!SystemBindingInitIds(aCx)) {
>+              aRv.Throw(NS_ERROR_FAILURE);

aRv.NoteJSContextException();

>+                if (!aNames.append(property.id)) {
>+                  aRv.Throw(NS_ERROR_FAILURE);

NS_ERROR_OUT_OF_MEMORY?

r=me with those nits

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8933707 [details] [diff] [review]
Part 2: Various test fixes to prepare for using WebIDL bindings for MessageManager classes

>+++ b/dom/indexedDB/test/head.js
>+      }, {once: true, capture: true}, true);

This needs documentation.

>+++ b/dom/presentation/tests/mochitest/test_presentation_dc_receiver_oop.html
>     mm.addMessageListener('check-navigator', function checknavigatorHandler(aSuccess) {

Why doesn't this need to wrapCallback?  I guess the test passes, so it's ok...

>+++ b/dom/quota/test/head.js
>+      }, {once: true, capture: true}, true);

Needs documentation.

>+++ b/testing/marionette/proxy.js
>-    this.mm.removeMessageListener(path, l[1]);
>+    this.mm.removeMessageListener(path, l);

Um... why is this not causing problems right now??

r=me.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Attachment: Review notes on part 3

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8933757 [details] [diff] [review]
Part 3: Add message manager concrete classes for WebIDL

>+class ChildProcessMessageManager : public SyncMessageSender

Shouldn't this be final?

>diff --git a/dom/base/ChromeMessageBroadcaster.cpp b/dom/base/ChromeMessageBroadcaster.cpp
>+ChromeMessageBroadcaster::ChromeMessageBroadcaster(nsFrameMessageManager* aParentManager,
>+  if (mIsProcessManager) {
>+    mozilla::HoldJSObjects(this);

So I have to ask: Why is this here and not in the MessageBroadcaster constructor?  The old logic does HoldJSObjects any time IsBroadcaster() and mIsProcessManager, right?  So why not do it in the place where we know we're a broadcaster?

Of course ChromeMessageBroadcaster is the only subclass I see here of MessageBroadcaster, and MessageBroadcaster has a proteted ctor, so there can't be any other MessageBroadcasters.... but then why are these separate classes?

>+  // This is a bit hackish. When parent manager is global, we want
>+  // to attach the message manager to it immediately.

I know you just copied this, but nothing here checks whether the parent manager is global.  We're just adding because we're a broadcaster.  Again, mabe this should live up in MessageBroadcaster, not ChromeMessageBroadcaster?

>+  // Is it just the frame message manager which waits until the
>+  // content process is running.

I know you just copied this comment.... but it makes no sense.  Do you happen to know what it's trying to say?

>diff --git a/dom/base/ChromeMessageSender.cpp b/dom/base/ChromeMessageSender.cpp
>+  // This is a bit hackish. When parent manager is global, we want
>+  // to attach the message manager to it immediately.
>+  // Is it just the frame message manager which waits until the
>+  // content process is running.

Again, the comments, while just copied, are not very understandable.

>diff --git a/dom/base/ChromeMessageSender.h b/dom/base/ChromeMessageSender.h
>+  ChromeMessageSender(ipc::MessageManagerCallback* aCallback,
>+                      nsFrameMessageManager* aParentManager,
>+                      MessageManagerFlags aFlags=MessageManagerFlags::MM_NONE);

I have a slight worry here: what if someone passes MM_BROADCASTER to this constructor for flags?  Right now no one does, but if someone did we could have a now-impossible situation of a broadcaster not added immediately to its parent, or one that is a process manager but didn't do HoldJSObjects.

I suspect what we might want is for the ChromeMessageSender constructor to strip out the MM_BROADCASTER flag from the passed-in flags.

>+++ b/dom/base/MessageListenerManager.cpp
>+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(MessageListenerManager)
>+NS_INTERFACE_MAP_END_INHERITING(nsFrameMessageManager)
>+NS_IMPL_ADDREF_INHERITED(MessageListenerManager, nsFrameMessageManager)
>+NS_IMPL_RELEASE_INHERITED(MessageListenerManager, nsFrameMessageManager)

NS_IMPL_ISUPPORTS_CYCLE_COLLECTION_INHERITED_0(MessageListenerManager, 
                                               nsFrameMessageManager)

>+NS_IMPL_CYCLE_COLLECTION_CLASS(MessageListenerManager)
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(MessageListenerManager,
>+                                                  nsFrameMessageManager)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mParentManager)
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(MessageListenerManager,
>+                                                nsFrameMessageManager)
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mParentManager)
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_END

NS_IMPL_CYCLE_COLLECTION_INHERITED(MessageListenerManager,
                                   nsFrameMessageManager,
                                   mParentManager)

>+++ b/dom/base/nsFrameMessageManager.h
>+  friend nsresult NS_NewChildProcessMessageManager(nsISyncMessageSender** aResult);

Why does that need to be a friend?  It's invoking only public constructors, right?

r=me with the above nits picked.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8933760 [details] [diff] [review]
Part 4: Convert MessageManager to WebIDL

>+++ b/dom/base/ChromeMessageBroadcaster.cpp
>+  MOZ_ASSERT(xpc::AccessCheck::isChrome(js::GetContextCompartment(aCx)));

I'd somewhat prefer:

  MOZ_ASSERT(nsContentUtils::IsSystemCaller(aCx));

which doesn't involve explicit compartments...

>+  // ProcessScriptLoader

This should mention what's going on with removeDelayedProcessScript.  Presumably the XPCOM signature is OK?

>+  void LoadProcessScript(const nsAString& aUrl, bool aAllowDelayedLoad,
>+  void GetDelayedProcessScripts(JSContext* aCx,

I have to ask.  Was there a reason to not put this on the superclass like GetInitialProcessData instead of duplicating these forwarders a few times?  (I'll believe that there is; I just want to know it!)

>+++ b/dom/base/ChromeMessageSender.cpp
>+  MOZ_ASSERT(xpc::AccessCheck::isChrome(js::GetContextCompartment(aCx)));

  MOZ_ASSERT(nsContentUtils::IsSystemCaller(aCx));

>+++ b/dom/base/ChromeMessageSender.h

Same comments here as for ChromeMessageBroadcaster.

>+++ b/dom/base/MessageBroadcaster.h

Please document that the XPCOM ReleaseCachedProcesses is OK.

>+++ b/dom/base/nsFrameMessageManager.cpp

>+nsFrameMessageManager::AddMessageListener(const nsAString& aMessageName,
>+                                          MessageListener& aListener,
>+                                          bool aListenWhenClosed,
>+                                          ErrorResult& aError)

aError is unused.  Can you not just remove it, and the [Throws] annotation in the IDL?

>+nsFrameMessageManager::RemoveWeakMessageListener(const nsAString& aMessageName,
>+                                                 MessageListener* aListener,

So why does this take a nullable arg while RemoveMessageListener takes a non-nullable one, and both Add methods take non-nullable ones?  Do we have people doing removeWeakMessageListener(null)?  Might be worth documenting in the IDL.

> nsFrameMessageManager::LoadScript(const nsAString& aURL,
>+  ErrorResult rv;

It would be better to sope it to right around the LoadScript call on the child manager, so we never end up passing around an already-failed ErrorResult.

And at that point you can just make it IgnoredErrorResult and drop the manual SuppressException() call.

>+nsFrameMessageManager::GetDelayedScripts(JSContext* aCx,

>   JS::Rooted<JSObject*> pair(aCx);

This looks unused.  Please remove it.

>+    url = JS_NewUCStringCopyN(aCx, mPendingScripts[i].get(),
>+                              mPendingScripts[i].Length());

So... I know that's what the code used to do, but I'd really prefer it if we just used ToJSValue here:

  JS::Rooted<JS::Value> url(aCx);
  if (!ToJSValue(aCx, mPendingScripts[i], &url)) {
    aError.NoteJSContextException(aCx);
    return;
  }

>+static JSObject*
>+CreateJSArrayObject(JSContext* aCx, const nsTArray<JS::Value>& aArray)

Again, I'd prefer just using ToJSValue.  See below.

>+nsresult
>+nsFrameMessageManager::GetDelayedScripts(JSContext* aCx,
>+  nsTArray<nsTArray<JS::Value>> list;
....
>+  JS::Rooted<JSObject*> array(aCx, JS_NewArrayObject(aCx, list.Length()));

I think you can remove all this manual array creation and CreateJSArrayObject stuff.  Once you have "list" built up:

  if (!ToJSValue(aCx, list, aList)) {
    return NS_ERROR_OUT_OF_MEMORY;
  }

  return NS_OK;

>@@ -619,21 +713,84 @@ nsFrameMessageManager::SendMessage(const
>+  JS::Rooted<JSObject*> dataArray(aCx, CreateJSArrayObject(aCx, result));

ToJSValue.

>+nsFrameMessageManager::SendMessage(JSContext* aCx,

The profiler label bit needs to be either in the shared code or at entry into the webidl version in addition to being at entry into the XPCOM version.

The #ifdef FUZZING block from SendMessage seems to be getting lost here.

The AllowMessage check is also getting lost, afaict.

>@@ -922,219 +1103,210 @@ nsFrameMessageManager::ReceiveMessage(ns
>+        if (!object) {
>+          continue;

This part should be happening in the webidl case too, because ObjectOrNull can return null...

>+      argument.mObjects.Construct(cpows);

Might be worth just marking this member required in the dictionaty, if this is the only place it comes up.  Then you won't need the Optional<> in there and can just do:

  argument.mObjects = cpows;

>-      JS::Rooted<JS::Value> json(cx, JS::NullValue());
>+      JS::Rooted<JS::Value> json(cx);

Why the change from null to undefined if !aCloneData?  Seems fishy.

>+      argument.mName.Construct(aMessage);
>+      argument.mPrincipal.Construct(aPrincipal);
>+      argument.mSync.Construct(aIsSync);
>+      argument.mTarget.Construct(aTarget);

Again, could make all those "required".

The principal behavior is changing to set the property to null, not undefined, when aPrincipal is null.  That's probably OK...  However the dictionary member claims to not be nullable, which is fishy in this case.  It should be nullable.

Alternately, you could condition the argument.mPrincipal.Construct() bit on aPrincipal being non-null.  Then we would switch from defining with undefined value to not defining at all (which is what's happenign with "ports" too).  I _think_ that should be ok too...

As far as that goes, is aTarget guaranteed non-null?  If not, the "target" dictionary member should be nullable...

>+        webIDLListener->ReceiveMessage(thisValue, argument, &rval, rv);

You should be holding a strong ref on the stack to webIDLListener while making that call.  Nothing guarantees that our member keeping it alive won't get changed by the call...

>+          JSAutoCompartment tac(cx, thisObject);

Fwiw, given how we set up our AutoEntryScript, I'm 99% sure we're already in that compartment...

>+          if (!JS_CallFunctionValue(cx, thisObject, funval,
>+                                    JS::HandleValueArray(argv), &rval)) {
>             continue;
>           }

Hmm.  So yes, your new code for the webidl case matches these semantics.  And they're OK, because the AES is scoped inside the loop, so this "continue" has the AES report any currently-pending exception on the JSContext... That part is _not_ obvious; may be worth a comment.

>+nsFrameMessageManager::GetInitialProcessData(JSContext* aCx,
>+  aInitialProcessData.set(init.toObjectOrNull());

I don't see what guarantees that "init" can't be undefined or whatever here.  We should either make this return "any" or throw if it's not object-or-null at this point...

>+already_AddRefed<nsIMessageSender>
>+nsFrameMessageManager::GetProcessMessageManager(ErrorResult& aError)

This never fails.  Please remove the Throws annotation.

>+++ b/dom/base/nsFrameMessageManager.h
>+  // ContentProcessMessageManager
>+  void GetInitialProcessData(JSContext* aCx,

Comment should say "GlobalProcessScriptLoader", no?

>+++ b/dom/bindings/Bindings.conf

SyncMessageSender should be 'concrete':False as well, I believe.

>+++ b/dom/webidl/MessageManager.webidl
>+interface ProcessScriptLoader
>+   * Returns all delayed scripts that will be loaded once a (remote)
>+   * frame becomes available. The return value is a list of URLs.

That comment is wrong (also wrong where you copied it from).  It's a list of (url, boolean) pairs, right?  The FrameScriptLoader comments get this right.

>+ChromeMessageBroadcaster implements GlobalProcessScriptLoader;
>+ChromeMessageBroadcaster implements FrameScriptLoader;

Ugh.  It's insane that it implements both but is expected to only be used by one (and if you try to mix them, weird stuff happens).

But I guess the current XPCOM setup has the same insanity due to classinfo.  That is, the classinfo exposes interfaces the underlying object claims to not QI to!!!

Might be worth a followup bug to clean this up and only expose the things that should be exposed, based on whether mIsProcessManager is true.

r=me with the above bits fixed

Comment 36

[:Gijs (he/him)]

Hey bz, Bobby says you're getting rid of all the xpcscriptable machinery in xpconnect once this bug lands - is there a bug on file for that? If not, can you let me know when you file one? I'm looking at doing some xpconnect-related performance stuff by looking at how we (ab)use it in the frontend, and might as well wait with some of that until the removal happens, if it's happening soon.

Comment 37

[Boris Zbarsky [:bzbarsky]]

I just filed bug 1425511.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8933762 [details] [diff] [review]
Part 5: Convert MessageManager globals to WebIDL

>+++ b/devtools/client/netmonitor/test/browser_net_view-source-debugger.js
>-  let wait = waitForDOM(document, "#stack-trace-panel .frame-link", 4);
>+  let wait = waitForDOM(document, "#stack-trace-panel .frame-link", 5);

Please explain this change in the commit message.

>+++ b/dom/base/ContentFrameMessageManager.h

This file could really use a documentation comment explaining what this class if for.

>+++ b/dom/base/MessageManagerGlobal.h

Again, documentation would be good.

>+++ b/dom/base/ProcessGlobal.cpp
>-  NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(ContentProcessMessageManager)

Can you remove the relevant bigts from nsDOMClassInfo.cpp and nsDOMClassInfoID.h?

I fact, I think you could remove the ChromeMessageSender/Broadcaster bits in the previous patch, and the Content*MessageManager bits here. 

>+++ b/dom/base/ProcessGlobal.h
>+  public mozilla::dom::MessageManagerGlobal,

You don't need the "mozilla::dom" here, I think; this code is already in that namespace.

>+++ b/dom/base/nsInProcessTabChildGlobal.cpp
>+nsInProcessTabChildGlobal::GetTabEventTarget(mozilla::ErrorResult& aError)

Why do you need aError here?  None of the GetTabEventTarget() implementations are fallible, as far as I can tell.

>+++ b/dom/ipc/TabChild.cpp
> TabChildGlobal::TabChildGlobal(TabChild* aTabChild)
>-  SetIsNotDOMBinding();

Do we not need to HoldJSObjects here?

Also why do we not need to preserve wrappers for these various globals?  Is that handled somewhere else?

>TabChildGlobal::Init()

Why is this safe to remove?  Now we're doing TelemetryScrollProbe::Create in the ctor, when our refcount is 0, which seems very questionable to me.  I think we should put the init call back, and take a ref before doing the TelemetryScrollProbe::Create bits like we used to.

>+++ b/dom/ipc/TabChild.h
>+class TabChildGlobal : public mozilla::dom::ContentFrameMessageManager,

No need for "mozilla::dom" here.

>+++ b/dom/webidl/MessageManager.webidl
>+  readonly attribute nsIEventTarget tabEventTarget;

This is guaranteed to be non-null?

>+ContentProcessMessageManager implements MessageManagerGlobal;

That's a bit unfortunate.  MessageManagerGlobal inherits from SyncMessageSender, which is a real interface, not a mixin.  This will need to be changed to implemement the mixin changes from webidl...

I guess it's OK to land this for now, and fix it when we do mixins, but a comment here about how it will need to be fixed is not a bad idea.  And possibly a followup bug blocking bug 1414372.

r=me with the above issues (esp. the Init() bit) fixed.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8933763 [details] [diff] [review]
Part 6: Mark some IDL interfaces as non-scriptable

>+++ b/dom/base/nsIMessageManager.idl
>+[builtinclass, uuid(bb5d79e4-e73c-45e7-9651-4d718f4b994c)]

The "builtinclass" is meaningless for a non-scriptable interface.  Same for some other interfaces in this file.

>+++ b/toolkit/components/satchel/FormHistoryStartup.js
>+        if (message.target instanceof MessageListenerManager) {

This isn't ideal, because we'd somewhat like to change the instanceof behavior of WebIDL interfaces at some point, since it looks like no other browser will implement it.

Unfortunately, MessageListenerManager is not a leaf interface, so ChromeUtils.getClassName probably won't work here.

Maybe just file a followup bug on this bit?

r=me with those nits.  Thank you very much for doing this, and I'm sorry the reviews were so slow.

Comment 40

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=37a38f92c251cf5cfd26fe2cab62212e69334f69

Comment 41

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5979b76dd6b79002281721c3373f4db6679c2136

Comment 42

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fbf8277a87b93e77e298444e06db9611353d9fb

Comment 43

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=865545672a77b7df7a8ba1104a6072e837469161

Comment 44

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e57a29e00fe2598652b3a0af6131f90bcc594e84

Comment 45

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=510b0668dc4fa3b3be6868ea50acaf0ee20d00de

Comment 46

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6d02d2696e12ea2b900d1d14a00f38f93e848d2

Comment 47

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=71475fc972c907aafb35a03283c155ee83c634b2

Comment 48

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0949dba396adfcaca5c5d94e2d15203ea62b577e
Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 1: Add infrastructure to expose 'system' names on a WebIDL global. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a109987af625889aadaed85346e0dcec0e1ea1a9
Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 2: Various test fixes to prepare for using WebIDL bindings for MessageManager classes. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/16af80d6cd93d194bc9bd496ed29e053bb234602
Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 3: Add message manager concrete classes for WebIDL. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9eafe225b8048c24f0878e0d0bcb0e091fe1c5a2
Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 4: Convert MessageManager to WebIDL. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b990735f1ad01f4f15380d55771d0eb0fe597b7
Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 5: Convert MessageManager globals to WebIDL. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c8317f4ad481a3376169f7b35c011c2bb64f520
Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 6: Mark some IDL interfaces as non-scriptable. r=bz.

Comment 49

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #31)
> Comment on attachment 8933699 [details] [diff] [review]

> That said, we will have that performance hit for all property access on
> system globals until something calls ResolveSystemBinding or
> GetSystemBindingNames for the first time.  Seems a bit annoying.  I wonder
> whether we can actually just init the ids the first time we create a system
> global and be done with it.  I guess that init could fail, but then we shoud
> fail system global creation too.

I added calls to SystemBindingInitIds in xpc::InitClassesWithNewWrappedGlobal and nsMessageManagerScriptExecutor::InitChildGlobalInternal.

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #32)
> >+++ b/dom/presentation/tests/mochitest/test_presentation_dc_receiver_oop.html
> >     mm.addMessageListener('check-navigator', function checknavigatorHandler(aSuccess) {
> 
> Why doesn't this need to wrapCallback?

IIRC both would work.

> >-    this.mm.removeMessageListener(path, l[1]);
> >+    this.mm.removeMessageListener(path, l);
> 
> Um... why is this not causing problems right now??

I don't know, might be that we ignore the leak?

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #34)
> Of course ChromeMessageBroadcaster is the only subclass I see here of
> MessageBroadcaster, and MessageBroadcaster has a proteted ctor, so there
> can't be any other MessageBroadcasters.... but then why are these separate
> classes?

I just merged MessageBroadcaster into ChromeMessageBroadcaster.

> 
> >+  // This is a bit hackish. When parent manager is global, we want
> >+  // to attach the message manager to it immediately.
> 
> I know you just copied this, but nothing here checks whether the parent
> manager is global.  We're just adding because we're a broadcaster.  Again,
> mabe this should live up in MessageBroadcaster, not ChromeMessageBroadcaster?

I removed the comment, it was incorrect (even before I moved it).

> >+  // Is it just the frame message manager which waits until the
> >+  // content process is running.
> 
> I know you just copied this comment.... but it makes no sense.  Do you
> happen to know what it's trying to say?

Removed the comment, it didn't apply to this case.

> >diff --git a/dom/base/ChromeMessageSender.cpp b/dom/base/ChromeMessageSender.cpp
> >+  // This is a bit hackish. When parent manager is global, we want
> >+  // to attach the message manager to it immediately.
> >+  // Is it just the frame message manager which waits until the
> >+  // content process is running.
> 
> Again, the comments, while just copied, are not very understandable.

Changed it to:

  // This is a bit hackish. We attach to the parent, but only if we have a callback. We
  // don't have a callback for the frame message manager, and for parent process message
  // managers (except the parent in-process message manager). In those cases we wait until
  // the child process is running (see MessageSender::InitWithCallback).

This reflects reality more closely.

> >diff --git a/dom/base/ChromeMessageSender.h b/dom/base/ChromeMessageSender.h
> >+  ChromeMessageSender(ipc::MessageManagerCallback* aCallback,
> >+                      nsFrameMessageManager* aParentManager,
> >+                      MessageManagerFlags aFlags=MessageManagerFlags::MM_NONE);
> 
> I have a slight worry here: what if someone passes MM_BROADCASTER to this
> constructor for flags?  Right now no one does, but if someone did we could
> have a now-impossible situation of a broadcaster not added immediately to
> its parent, or one that is a process manager but didn't do HoldJSObjects.
> 
> I suspect what we might want is for the ChromeMessageSender constructor to
> strip out the MM_BROADCASTER flag from the passed-in flags.

I actually added asserts to public constructors taking flags, like:

    MOZ_ASSERT(!(aFlags & ~(MessageManagerFlags::MM_GLOBAL |
                            MessageManagerFlags::MM_PROCESSMANAGER |
                            MessageManagerFlags::MM_OWNSCALLBACK)));

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #35)
> >+  void LoadProcessScript(const nsAString& aUrl, bool aAllowDelayedLoad,
> >+  void GetDelayedProcessScripts(JSContext* aCx,
> 
> I have to ask.  Was there a reason to not put this on the superclass like
> GetInitialProcessData instead of duplicating these forwarders a few times? 
> (I'll believe that there is; I just want to know it!)

We could put it there, but I prefer to have the implementation on the right class, where it's actually used. The others have to be on nsFrameMessageManager because the globals forward to that.

> >+nsFrameMessageManager::AddMessageListener(const nsAString& aMessageName,
> >+                                          MessageListener& aListener,
> >+                                          bool aListenWhenClosed,
> >+                                          ErrorResult& aError)
> 
> aError is unused.  Can you not just remove it, and the [Throws] annotation
> in the IDL?

The globals can actually throw, and they use the same member.

> I think you can remove all this manual array creation and CreateJSArrayObject stuff. 
> Once you have "list" built up:
> 
>   if (!ToJSValue(aCx, list, aList)) {
>     return NS_ERROR_OUT_OF_MEMORY;
>   }

Had to make some changes to ToJSValue to support this.

> As far as that goes, is aTarget guaranteed non-null?  If not, the "target"
> dictionary member should be nullable...

I went through the callers, I think it's guaranteed non-null.

> >+already_AddRefed<nsIMessageSender>
> >+nsFrameMessageManager::GetProcessMessageManager(ErrorResult& aError)
> 
> This never fails.  Please remove the Throws annotation.

The globals can actually throw, and they use the same member.

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #38)
> Comment on attachment 8933762 [details] [diff] [review]
> Part 5: Convert MessageManager globals to WebIDL

> >+++ b/dom/ipc/TabChild.cpp
> > TabChildGlobal::TabChildGlobal(TabChild* aTabChild)
> >-  SetIsNotDOMBinding();
> 
> Do we not need to HoldJSObjects here?

No, I don't think we do, this doesn't hold any JS objects apart from the binding.

Took care of the other comments.

Comment 50

[Dorel Luca [:dluca]]

Backed out 6 changesets (bug 888600) for Valgrind and build bustge on a CLOSED TREE

Log of the failures:
https://treeherder.mozilla.org/logviewer.html#?job_id=163646071&repo=mozilla-inbound&lineNumber=29123
https://treeherder.mozilla.org/logviewer.html#?job_id=163646048&repo=mozilla-inbound&lineNumber=37333

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4de1162741eacc436221facc3444481e6065390

Push that got backed out:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7c8317f4ad481a3376169f7b35c011c2bb64f520

Comment 51

[Peter Van der Beken [:peterv]]

Sigh. The quiter extension that we use for Valgrind and PGO is checked into the tree as a signed XPI, so when we change Gecko APIs that it uses (like I do here) then we need to get a new signed XPI to check in alongside.

Comment 52

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Peter Van der Beken [:peterv] from comment #51)
> Sigh. The quiter extension that we use for Valgrind and PGO is checked into
> the tree as a signed XPI, so when we change Gecko APIs that it uses (like I
> do here) then we need to get a new signed XPI to check in alongside.

I believe this page has related details and links to signing keys on Mana:

https://wiki.mozilla.org/EngineeringProductivity/HowTo/SignExtensions

Comment 53

[Peter Van der Beken [:peterv]]

Attachment: [e-mail]

Jason, this bug updates the code of the quitter extension that is used in testing, and it needs to be signed with "Mozilla Extensions" (see also bug 1393162). Can you please have the xpi from this attachment signed with "Mozilla Extensions" so that I can land it together with my other patches for this bug?

Comment 54

[Peter Van der Beken [:peterv]]

Hmm, I'll try to do it myself with those instructions.

Comment 55

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aea28ae66fd7eecc643e9a7cde59801cd3a3b038

Comment 56

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f36fa76ea74abbece62ae853044d863827a8996

Comment 57

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=35f92cd8aa147946b087ed3bc194dcc6e4e2cc15

Comment 58

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7bd4c6c9741aca337592b50f8533ef9a30ff28d
Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 1: Add infrastructure to expose 'system' names on a WebIDL global. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/30d568d628dd2be0d8f928905a472a9a27bf2a18
Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 2: Various test fixes to prepare for using WebIDL bindings for MessageManager classes. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/79ef59047e638311f6249f82b528d6ab5d3a7666
Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 3: Add message manager concrete classes for WebIDL. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/af53037819615484e614e7343256f31b237f1bab
Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 4: Convert MessageManager to WebIDL. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2efb9b1753f63748fa547117c0665e8f3e705e08
Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 5: Convert MessageManager globals to WebIDL. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/83c87140dc3da6d521c9380beea85b30cb69c7b5
Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 6: Mark some IDL interfaces as non-scriptable. r=bz.

Comment 59

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/c7bd4c6c9741
https://hg.mozilla.org/mozilla-central/rev/30d568d628dd
https://hg.mozilla.org/mozilla-central/rev/79ef59047e63
https://hg.mozilla.org/mozilla-central/rev/af5303781961
https://hg.mozilla.org/mozilla-central/rev/2efb9b1753f6
https://hg.mozilla.org/mozilla-central/rev/83c87140dc3d

Comment 60

[Ionuț Goldan [:igoldan]]

This improved performance according to our tabpaint test! I noticed this gain showed up on Windows platforms also but only for a short while. Another bug shadowed the improvement from Windows.

== Change summary for alert #11805 (as of Tue, 27 Feb 2018 09:35:06 GMT) ==

Improvements:

  4%  tabpaint linux64 pgo e10s stylo     51.94 -> 49.65

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

Comment 61

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out 6 changesets (bug 888600) for beta simulation failures: build bustage on Linux and Windows opt (bug 1442036) and devtools failure browser_net_view-source-debugger.js (bug 1441961):

https://hg.mozilla.org/mozilla-central/rev/714ff834425c99e5521b7b0e3c3c736bf2f34916

Comment 62

[Alexandre Poirot [:ochameau]]

Note that while working on bug 1440321 (converting Task.jsm to async/await in devtools),
I'm also getting failures on this test. It may just be racy...
I'll report back here once I narrow it down on the Task.jsm front.

Comment 63

[Ionuț Goldan [:igoldan]]

Before the backout from comment 61, we noticed these regressions. Please address them before relanding.

== Change summary for alert #11858 (as of Thu, 01 Mar 2018 09:21:33 GMT) ==

Regressions:

  1%  compiler_metrics num_static_constructors linux32 pgo      134.00 -> 136.00
  1%  compiler_metrics num_static_constructors linux64 pgo      134.00 -> 136.00
  1%  compiler_metrics num_static_constructors linux32 opt      134.00 -> 136.00
  0%  compiler_metrics num_static_constructors linux32 debug    245.00 -> 246.00

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

Comment 64

[Boris Zbarsky [:bzbarsky]]

Ionut, is there any data on what the new static constructors are, exactly?  That would make that regression a lot easier to address...

Comment 65

[Peter Van der Beken [:peterv]]

The Valgrind build log has "addons.xpi	WARN	Add-on [e-mail] is not correctly signed.", so it seems the signing procedure from https://wiki.mozilla.org/EngineeringProductivity/HowTo/SignExtensions doesn't lead to an xpi usable on beta.

Comment 66

[Boris Zbarsky [:bzbarsky]]

Jason, see comment 65?

Comment 67

[Jason Thomas [:jason]]

(In reply to Peter Van der Beken [:peterv] from comment #65)
> The Valgrind build log has "addons.xpi	WARN	Add-on [e-mail] is
> not correctly signed.", so it seems the signing procedure from
> https://wiki.mozilla.org/EngineeringProductivity/HowTo/SignExtensions
> doesn't lead to an xpi usable on beta.

https://mana.mozilla.org/wiki/display/FIREFOX/Internal+Extension+Signing is the documentation for signing Internal Extensions. I've ni'd :wezhou to assist.

Comment 68

[Peter Van der Beken [:peterv]]

Comment on attachment 8953110 [details]
[e-mail]

This is not the right version.

Comment 69

[Peter Van der Beken [:peterv]]

Hmm, the xpi I checked in doesn't contain the META-INF directory. No idea why that worked on trunk.

Comment 70

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Peter Van der Beken [:peterv] from comment #69)
> Hmm, the xpi I checked in doesn't contain the META-INF directory. No idea
> why that worked on trunk.

In my experience, it's quite hard to follow how these in-tree signed extensions are working...  There's several different docs from different points in time.  Some of them only check the signatures on specific release branches, so things can look okay during development but then blow up later.

Overall, it's a frustrating workflow...  If we at least had one clear document that covered all of them, with clear steps to take when making changes to each, that would be great progress.  (For example, this quitter add-on doesn't appear on https://mana.mozilla.org/wiki/display/FIREFOX/Internal+Extension+Signing, so it's unclear if that's really the appropriate mechanism.)

Comment 71

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f12dd61fa1bda04fc20cc7f565d95cc87d7ce7f

Comment 72

[:wezhou]

I'm not sure what info is needed from me at this point. If you still do, please let me know.

Comment 73

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=09c70ab4ba89d773a23e4252aa62ba03df5b2b9f

Comment 74

[Ionuț Goldan [:igoldan]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #64)
> Ionut, is there any data on what the new static constructors are, exactly? 
> That would make that regression a lot easier to address...

I don't know more about this test, so I'm asking the owner.

Comment 75

[Peter Van der Beken [:peterv]]

Looks like they're coming from ResolveSystemBinding.cpp, should be able to fix that.

Comment 76

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6fc0e551a54e6444b61d2aef1ffffafd5ebb11d

Comment 77

[Peter Van der Beken [:peterv]]

The devtools failure is because on beta and release we set javascript.options.asyncstack to false, so there's one less stackframe.

Comment 78

[Peter Van der Beken [:peterv]]

Attachment: [e-mail]

Comment 79

[Peter Van der Beken [:peterv]]

(In reply to :wezhou from comment #72)
> I'm not sure what info is needed from me at this point. If you still do,
> please let me know.

What we need is the [e-mail] signed by "Mozilla Extensions" (see bug 1393162 when this was last done). I've attached the xpi to be signed in attachment 8955474 [details].

Comment 80

[:wezhou]

What kind of addon is [e-mail]?

The kind of addons I can help sign is referred to as "system addons", and they will be signed by the following issuer:

> Issuer: C=US, O=Mozilla Corporation, OU=Mozilla AMO Production Signing Service, CN=production-signing-ca.addons.mozilla.org/emailAddress=[e-mail]

Is this the same as signed by "Mozilla Extensions"?

 
If the said addon is what known as "internal addons", then I believe the process to sign them is in these documents:

* https://mana.mozilla.org/wiki/display/FIREFOX/Internal+Extension+Signing
* https://mana.mozilla.org/wiki/display/SVCOPS/Sign+a+Mozilla+Internal+Extension

Comment 81

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Since Jason was the one who signed the last version (in bug 1393162), maybe he knows the right procedure here?

Comment 82

[Jason Thomas [:jason]]

"Mozilla Extensions" aka Internal Extensions signing is self service. Who ever is the owner of the [e-mail] should request access to sign these sort of add-ons using the process here https://mana.mozilla.org/wiki/display/FIREFOX/Internal+Extension+Signing. 

If you need this add-on signed right away then :wezhou can sign it for you.

Comment 83

[Peter Van der Beken [:peterv]]

Ted, are you the right person to be the owner of the quitter extension? (see comment 82)

Comment 84

[(not currently active) Ted Mielczarek]

I haven't touched it in ages, so probably not.

Comment 85

[Peter Van der Beken [:peterv]]

I don't think these patches should be blocked on finding an owner for that code.

:wezhou, can you sign the extension from attachment 8955474 [details] with "Mozilla Extensions"? Thanks!

Comment 86

[Peter Van der Beken [:peterv]]

I filed bug 1442741 on finding an owner.

Comment 87

[:wezhou]

Perter, do you want it to be signed by -stage or -prod environment?

Please see https://mana.mozilla.org/wiki/display/SVCOPS/Sign+a+Mozilla+Internal+Extension#SignaMozillaInternalExtension-Environments for the difference.

Comment 88

[Peter Van der Beken [:peterv]]

I'm assuming -prod, this needs to run on automation in builds we release. The current version in the tree has:

"issuer: C=US, O=Mozilla Corporation, OU=Mozilla AMO Production Signing Service, CN=signingca1.addons.mozilla.org/emailAddress=[e-mail]"

Comment 89

[:wezhou]

Attachment: signed.8955474.xpi

Hi Peter,

Please see attached for the signed file.

As mentioned above, "Mozilla Extensions" aka Internal Extensions signing is self service. If you often need to sign this kind of extensions, I encourage you to apply for one account to be able to sign them. Reading the doc [1], I don't think it *must* be the extension owners who can apply. 

[1] https://mana.mozilla.org/wiki/display/FIREFOX/Internal+Extension+Signing

Comment 90

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b1113f58d96fe1e7f9767f554dd229905279bfe

Comment 91

[Peter Van der Beken [:peterv]]

Attachment: Part 1a: Make PinnedStringId constructor constexpr

This removes the static constructor that's added when you store a PinnedStringId in a static array.

Comment 92

[Jeff Walden [:Waldo]]

Comment on attachment 8957292 [details] [diff] [review]
Part 1a: Make PinnedStringId constructor constexpr

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

::: js/public/Id.h
@@ +155,5 @@
>  {
>      return (size_t)JSID_BITS(id) == JSID_TYPE_SYMBOL;
>  }
>  
> +constexpr JS_PUBLIC_DATA(const jsid) JSID_VOID = { size_t(JSID_TYPE_VOID) };

I...think this is okay, and will not result in umpteen copies of a JSID_VOID symbol separately allocated, as long as people aren't taking the address of JSID_VOID.  Which they do not, now.  But I'm not 100% certain of that; these aspects of C++ are extraordinarily far from my strong suit.  Forwarding review for a second eye.

Safest thing would be to, instead of JSID_VOID, have

namespace JS {

constexpr jsid
MakeVoidId()
{
  return jsid { size_t(JSID_TYPE_VOID) };
}

} // namespace JS

#define JSID_VOID (JS::MakeVoidId())

(and ideally remove JS::MakeVoidId as followup), but if it isn't necessary, it's not worth the hassle.

Comment 93

[Nathan Froyd [:froydnj]]

Comment on attachment 8957292 [details] [diff] [review]
Part 1a: Make PinnedStringId constructor constexpr

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

This is fine.  Waldo's solution of having a constexpr function would also make it difficult for people to depend on address identity of JSID_VOID, which seems like a good thing.  Followup?

It also seems reasonably to apply the same treatment to the other JSID_* constants.  Good followup?

Comment 94

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ee3ef3aa0bb1623d4a940623069691473801bca

Comment 95

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=42b2bc4124bd1cb381e0c1584606523ae33a8cfe

Comment 96

[Peter Van der Beken [:peterv]]

I had to remove the JS_PUBLIC_DATA for JSID_VOID. And there's now still a problem with generating the binding for JSID_VOID for Rust.

Comment 97

[Jan de Mooij [:jandem]]

(In reply to Peter Van der Beken [:peterv] from comment #96)
> I had to remove the JS_PUBLIC_DATA for JSID_VOID. And there's now still a
> problem with generating the binding for JSID_VOID for Rust.

The SM Rust build is tier 2 and this seems like an important bug, so landing + a follow-up bug would be acceptable IMO.

Moving the following line to WHITELIST_FUNCTIONS might help: https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/js/rust/build.rs#248

Comment 98

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3a9ad856fc9be5962d7d275c1123cfef848086d
Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 1: Add infrastructure to expose 'system' names on a WebIDL global. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4de6aa3e7bff20c247d2f482cc9d53a9b7e998f8
Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 1a: Make PinnedStringId constructor constexpr. r=Waldo/froydnj.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ddffc18a48a16458d3cc25996469824b19ead16d
Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 2: Various test fixes to prepare for using WebIDL bindings for MessageManager classes. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/00cee09c56f50994d4b5c97cdfc270d80a15645d
Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 3: Add message manager concrete classes for WebIDL. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a784023ff96061acd91b51b42e4f713d48caab49
Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 4: Convert MessageManager to WebIDL. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/187b9827d11a6c89a6606ec13bc08e584b6eefe6
Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 5: Convert MessageManager globals to WebIDL. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/243246b2f4406f0b2f118758220db5c7fdd215df
Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 6: Mark some IDL interfaces as non-scriptable. r=bz.

Comment 99

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/a3a9ad856fc9
https://hg.mozilla.org/mozilla-central/rev/4de6aa3e7bff
https://hg.mozilla.org/mozilla-central/rev/ddffc18a48a1
https://hg.mozilla.org/mozilla-central/rev/00cee09c56f5
https://hg.mozilla.org/mozilla-central/rev/a784023ff960
https://hg.mozilla.org/mozilla-central/rev/187b9827d11a
https://hg.mozilla.org/mozilla-central/rev/243246b2f440

Comment 100

[Eric Rahm [:erahm]]

It looks like this caused a 151K regresssion in libxul size.

https://treeherder.mozilla.org/perf.html#/graphs?timerange=7776000&series=autoland,1338582,0,2&series=mozilla-inbound,1299711,1,2&zoom=1519622212079.0208,1520304288236.8694,121656334.52323402,122479857.16060677&selected=mozilla-inbound,1299711,310434,420153959,2

Comment 101

[Eric Rahm [:erahm]]

Sorry, actually 174k when it relanded.

https://treeherder.mozilla.org/perf.html#/graphs?timerange=7776000&series=autoland,1338582,0,2&series=mozilla-inbound,1299711,1,2&zoom=1521641288274.5193,1521651035046.3013,130001619.21093707,130444349.3803149&selected=mozilla-inbound,1299711,318657,435175113,2

Comment 102

[Boris Zbarsky [:bzbarsky]]

Note that this bug added some duplication, of which 30k was then removed in bug 1448850.  And there were also various other cleanups blocked by this bug that removed things.

But fundamentally, it looks like this change added ~100k .text, ~20k .rela.dyn, 8k .rodata, 8k .eh_frame, 11k .data.rel.ro, and some minor bits in .data, .bss, .eh_frame_hdr.  

Looking over the actual symbols, we have lots of code in the binding files... For example, we have two copies of everything in MessageManagerGlobal and the things it inherits from, because it's implemented by two different interfaces (ContentFrameMessageManager and ContentProcessMessageManager).  And these functions are not tiny, unfortunately...

Comment 103

[Bobby Holley (:bholley)]

If there ever was a bug to blow some codesize on, this is probably it.

Comment 104

[Boris Zbarsky [:bzbarsky]]

True, but it's worth thinking a bit about patterns here that make the codesize bigger than it "should" be and addressing them; it might help other cases too.