Move ContentFrameMessageManager to WebIDL

RESOLVED FIXED in Firefox 61

Status

()

defect
P2
normal
RESOLVED FIXED
6 years ago
2 months ago

People

(Reporter: Ms2ger, Assigned: peterv)

Tracking

(Depends on 2 bugs, Blocks 3 bugs)

Trunk
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(11 attachments, 7 obsolete attachments)

15.38 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.13 KB, text/plain
Details
20.94 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
60.70 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
87.92 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
79.59 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
131.36 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.06 KB, text/plain
Details
2.60 KB, application/x-xpinstall
Details
6.63 KB, application/x-xpinstall
Details
2.36 KB, patch
froydnj
: review+
Waldo
: review+
Details | Diff | Splinter Review
No description provided.
Reporter

Updated

6 years ago
Depends on: 890684
When this is done, the UnwrapArg bits in GlobalObject::GetAsSupports should go away as well.
Assuming that all our globals will be Web IDL at that point, that is.  Maybe that still won't be the case.
(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.
So is anything blocking this?  Or is this just work nobody has done?
The latter.  I'd review patches here in a heartbeat!
The latter yes. It is surprisingly lots of work to convert all the message manager stuff to
use webidl,
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. :)
Blocks: 1135220
No longer blocks: 1135220
No longer blocks: 1137739
Assignee

Updated

3 years ago
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Assignee

Updated

2 years ago
Depends on: 1383059
Blocks: 1396856
Blocks: 1397448
Assignee

Comment 11

2 years ago
Attachment #8905160 - Attachment is obsolete: true
Priority: -- → P2
Assignee

Comment 26

2 years ago
Attachment #8908801 - Attachment is obsolete: true
Attachment #8933757 - Flags: review?(bzbarsky)
Assignee

Comment 27

2 years ago
Attachment #8908802 - Attachment is obsolete: true
Attachment #8933760 - Flags: review?(bzbarsky)
Assignee

Comment 28

2 years ago
Attachment #8908803 - Attachment is obsolete: true
Attachment #8933762 - Flags: review?(bzbarsky)
So exciting! :-)
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
Attachment #8933699 - Flags: review?(bzbarsky) → review+
Attachment #8933706 - Attachment mime type: text/x-c++src → text/plain
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.
Attachment #8933707 - Flags: review?(bzbarsky) → review+
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.
Attachment #8933757 - Flags: review?(bzbarsky) → review+
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
Attachment #8933760 - Flags: review?(bzbarsky) → review+

Comment 36

a year ago
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.
Flags: needinfo?(bzbarsky)

Updated

a year ago
Blocks: 1425466
I just filed bug 1425511.
Flags: needinfo?(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.
Attachment #8933762 - Flags: review?(bzbarsky) → review+
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.
Attachment #8933763 - Flags: review?(bzbarsky) → review+
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.
(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.
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.
Flags: needinfo?(peterv)
(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
Posted file quitter@mozilla.org.xpi (obsolete) —
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?
Flags: needinfo?(jthomas)
Hmm, I'll try to do it myself with those instructions.
Flags: needinfo?(jthomas)
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.
See Also: → 1441872
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
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
Status: RESOLVED → REOPENED
Flags: needinfo?(peterv)
Resolution: FIXED → ---
Target Milestone: mozilla60 → ---
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.
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
Ionut, is there any data on what the new static constructors are, exactly?  That would make that regression a lot easier to address...
Flags: needinfo?(igoldan)
The Valgrind build log has "addons.xpi	WARN	Add-on quitter@mozilla.org 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.
Jason, see comment 65?
Flags: needinfo?(jthomas)
(In reply to Peter Van der Beken [:peterv] from comment #65)
> The Valgrind build log has "addons.xpi	WARN	Add-on quitter@mozilla.org 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.
Flags: needinfo?(jthomas) → needinfo?(wezhou)
Comment on attachment 8953110 [details]
quitter@mozilla.org.xpi

This is not the right version.
Attachment #8953110 - Attachment is obsolete: true
Hmm, the xpi I checked in doesn't contain the META-INF directory. No idea why that worked on trunk.
(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 72

a year ago
I'm not sure what info is needed from me at this point. If you still do, please let me know.
Flags: needinfo?(wezhou)
(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.
Flags: needinfo?(igoldan) → needinfo?(nfroyd)
Looks like they're coming from ResolveSystemBinding.cpp, should be able to fix that.
Flags: needinfo?(nfroyd)
The devtools failure is because on beta and release we set javascript.options.asyncstack to false, so there's one less stackframe.
(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 quitter@mozilla.org.xpi signed by "Mozilla Extensions" (see bug 1393162 when this was last done). I've attached the xpi to be signed in attachment 8955474 [details].
Flags: needinfo?(peterv) → needinfo?(wezhou)

Comment 80

a year ago
What kind of addon is quitter@mozilla.org.xpi?

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=services-ops+addonsigning@mozilla.com

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
Flags: needinfo?(wezhou)
Since Jason was the one who signed the last version (in bug 1393162), maybe he knows the right procedure here?
Flags: needinfo?(jthomas)
"Mozilla Extensions" aka Internal Extensions signing is self service. Who ever is the owner of the quitter@mozilla.org.xpi 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.
Flags: needinfo?(jthomas)
Ted, are you the right person to be the owner of the quitter extension? (see comment 82)
Flags: needinfo?(ted)
I haven't touched it in ages, so probably not.
Flags: needinfo?(ted)
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!
Flags: needinfo?(wezhou)
I filed bug 1442741 on finding an owner.

Comment 87

a year ago
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.
Flags: needinfo?(wezhou)
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=foxsec@mozilla.com"

Comment 89

a year ago
Posted file 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
This removes the static constructor that's added when you store a PinnedStringId in a static array.
Attachment #8957292 - Flags: review?(jwalden+bmo)
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.
Attachment #8957292 - Flags: review?(nfroyd)
Attachment #8957292 - Flags: review?(jwalden+bmo)
Attachment #8957292 - Flags: review+
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?
Attachment #8957292 - Flags: review?(nfroyd) → review+
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.
(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
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.
Assignee

Updated

a year ago
Depends on: 1447701
Assignee

Updated

a year ago
Blocks: 1448850

Updated

a year ago
Depends on: 1461078
Depends on: 1457301
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...
If there ever was a bug to blow some codesize on, this is probably it.
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.
Depends on: 1474843
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.