Closed Bug 776832 Opened 12 years ago Closed 12 years ago

Add a scriptable API to assert app permissions through messagemanager

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla18
Project Flags:
blocking-basecamp +

People

(Reporter: cjones, Assigned: philikon)

References

Details

(Whiteboard: [WebAPI:P1][LOE:M])

Attachments

(1 file, 9 obsolete files)

v5
(deleted), patch
Details | Diff | Splinter Review
Attached patch API sketch (obsolete) (deleted) — — Splinter Review 
What I have in mind is the attached patch.  I can implement this API for frame-scoped message managers, but not for the process ones because I need to see how bug 776825 resolves.
I don't understand what capabilities this is about.
blocking-basecamp: --- → +
Assignee: nobody → philipp
Blocks: 783392
(In reply to Olli Pettay [:smaug] from comment #1)
> I don't understand what capabilities this is about.

Capabilities a.k.a. permissions as managed by the permission manager. Basically, the chrome process should only respond to requests coming from content process that actually have the permission to receive the data. For example, if a content process requests that a telephony call be placed, we should not only verify the telephony permission at the time the Web API is access in the content process, but also when the IPC message is received in the chrome process.
Attached patch WIP 1 (obsolete) (deleted) — — Splinter Review 
So here's a WIP based on cjones's API sketch. In order to actually get to the ContentParent object, I'm going to have refactor nsFrameMessageManager a bit to keep track of the process (right now it just stores a "callback data" void ptr, ick). It will take a lot of strength for me not to explode nsFrameMessageManager entirely (which is overdue, but probably not very wise given time constraints.)
Attachment #645208 - Attachment is obsolete: true
It would be better to grab the TabParent from nsFrameLoader for frame mm's, but that's more of a nice-to-have, doesn't fundamentally add any security.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> It would be better to grab the TabParent from nsFrameLoader for frame mm's,
> but that's more of a nice-to-have, doesn't fundamentally add any security.

Right. That's how it will work for frame-based message managers. But for process-based message managers, we need remember the process (ContentParent object) in the constructor.
Absolutely, yep.
Blocks: 770778
No longer blocks: 782488
Blocks: 776672
Attached patch WIP 2 (obsolete) (deleted) — — Splinter Review 
This is about the most minimal implementation I could come up with. It's ugly and unclean, I know. I had to fight urges to refactor nsFrameMessageManager entirely pretty hard.

Calling it a WIP 2 since I haven't actually tested it yet, neither manually nor in an automated fashion. All I know is it compiles and Firefox starts up.
Attachment #654267 - Attachment is obsolete: true
What is ugly with the approach? 
(The patch seems to miss the in-process-frame-message-manager case.)
A bit simpler approach would be to let non-broadcasting mm to have nsRefPtr<ContentParent>
member variable. If it is not there, we're same process mm, that member variable would be null, 
and capability allowed.

fmm could be cleaned up a bit sure. Callback data could be changed from void* to some
real type and various boolean parameters to the ctor could be change to bit flags, so that
one could create fmm
new nsFrameMessageManager(callbackObjectWithCallbacks, parent, cx,
                          MM_CHROME | MM_GLOBAL | MM_BROADCASTER)
Whiteboard: [WebAPI:P2]
Whiteboard: [WebAPI:P2] → [WebAPI:P1]
(In reply to Olli Pettay [:smaug] from comment #8)
> What is ugly with the approach?

Mostly the heuristics about how to cast (void*) back to something meaningful.

> (The patch seems to miss the in-process-frame-message-manager case.)

Hmm, you're right. Unfortunately my heuristics about mCallbackData will no longer work because of this case. Also, should I be worried that the callback data for nsFrameLoader's message managers can be null in some cases, but the callback (SendAsyncMessageToChild) always expects to be able to cast the callback data to nsFrameLoader? https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameLoader.cpp#2357

> A bit simpler approach would be to let non-broadcasting mm to have
> nsRefPtr<ContentParent>
> member variable. If it is not there, we're same process mm, that member
> variable would be null, and capability allowed.

That would only work for process managers, not for frame managers. For frame managers we'd need an nsRefPtr<PBrowserParent>. And so we'd need two more arguments in the constructor. Oof.

> fmm could be cleaned up a bit sure. Callback data could be changed from
> void* to some
> real type and various boolean parameters to the ctor could be change to bit
> flags, so that
> one could create fmm
> new nsFrameMessageManager(callbackObjectWithCallbacks, parent, cx,
>                           MM_CHROME | MM_GLOBAL | MM_BROADCASTER)

This is pretty much what I had in mind.
(In reply to Philipp von Weitershausen [:philikon] from comment #9)
> That would only work for process managers, not for frame managers. For frame
> managers we'd need an nsRefPtr<PBrowserParent>. And so we'd need two more
> arguments in the constructor. Oof.
Why couldn't fmm take ContentParent? Basically pass the manager of PBrowserParent to fmm.
Target Milestone: --- → mozilla17
Version: Trunk → Other Branch
sorry I accidentally modified version and milestone.
Target Milestone: mozilla17 → ---
Version: Other Branch → Trunk
(In reply to Olli Pettay [:smaug] from comment #10)
> Why couldn't fmm take ContentParent? Basically pass the manager of
> PBrowserParent to fmm.

It could, but nsFrameLoader creates the ContentParent after it creates the message manager, for instance. I think at this point it's just easier to do the callback object that we talked about in comment 8 and 9.
Attached patch WIP 3 (obsolete) (deleted) — — Splinter Review 
I ragequit the minimal way and ended up going with the refactoring that smaug and I had in mind. It's not complete yet, but the path I'm going down should be pretty obvious from this. So I thought this is a good place to stop and see if smaug still agrees before I keep on making the compiler happy.
Attachment #658676 - Attachment is obsolete: true
Attachment #659883 - Flags: feedback?(bugs)
Comment on attachment 659883 [details] [diff] [review]
WIP 3

>+
>+namespace ipc {
>+
>+struct MessageListenerInfo
>+{
>+  nsCOMPtr<nsIMessageListener> mListener;
>+  nsCOMPtr<nsIAtom> mMessage;
>+};
Why do we need this here?


>+
>+typedef enum {
>+  MM_CHROME = 1,
>+  MM_GLOBAL = 2,
>+  MM_PROCESSMANAGER = 4,
>+  MM_BROADCASTER = 8
>+} MessageManagerFlags;
>+
>+class MessageManagerCallback
>+{
>+  NS_INLINE_DECL_REFCOUNTING(MessageManagerCallback)
>+
>+public:
>+  virtual bool LoadScript(const nsAString& aURL)
>+  {
>+    return true;
>+  }
>+
>+  virtual bool SendSyncMessage(const nsAString& aMessage,
>+                               const mozilla::dom::StructuredCloneData& aData,
>+                               InfallibleTArray<nsString>* aJSONRetVal)
>+  {
>+    return true;
>+  }
>+
>+  virtual bool SendAsyncMessage(const nsAString& aMessage,
>+                                const mozilla::dom::StructuredCloneData& aData)
>+  {
>+    return true;
>+  }
>+
>+  virtual bool HasCapability(const nsAString& aCapability)
>+  {
>+    return false;
>+  }
>+};
f+ for this
(r- for the patch :) )



> class nsFrameMessageManager MOZ_FINAL : public nsIContentFrameMessageManager,
>                                         public nsIMessageBroadcaster,
>-                                        public nsIFrameScriptLoader
>+                                        public nsIFrameScriptLoader,
>+                                        public nsICapabilitiesChecker
> {
>   typedef mozilla::dom::StructuredCloneData StructuredCloneData;
> public:
>-  nsFrameMessageManager(bool aChrome,
>-                        nsSyncMessageCallback aSyncCallback,
>-                        nsAsyncMessageCallback aAsyncCallback,
>-                        nsLoadScriptCallback aLoadScriptCallback,
>-                        void* aCallbackData,
>+  nsFrameMessageManager(mozilla::dom::ipc::MessageManagerCallback* aCallback,
>                         nsFrameMessageManager* aParentManager,
>                         JSContext* aContext,
>-                        bool aGlobal = false,
>-                        bool aProcessManager = false,
>-                        bool aBroadcaster = false)
>-  : mChrome(aChrome),
>-    mGlobal(aGlobal),
>-    mIsProcessManager(aProcessManager),
>-    mIsBroadcaster(aBroadcaster),
>+                        mozilla::dom::ipc::MessageManagerFlags aFlags)
>+  : mChrome(aFlags & mozilla::dom::ipc::MM_CHROME),
>+    mGlobal(aFlags & mozilla::dom::ipc::MM_GLOBAL),
>+    mIsProcessManager(aFlags & mozilla::dom::ipc::MM_PROCESSMANAGER),
>+    mIsBroadcaster(aFlags & mozilla::dom::ipc::MM_BROADCASTER),
Nit, I think I'd prefer !!(aFlags & some_value) to ensure that the value is either 0 or 1


>+  nsRefPtr<mozilla::dom::ipc::MessageManagerCallback> mCallback;
this is a bit worrisome. MM owns the callback, and at least in some cases callback owns the mm,
yet the callback object is not cycle collected. What breaks the cycle.
Attachment #659883 - Flags: feedback?(bugs) → feedback+
Blocks: 776663
(In reply to Olli Pettay [:smaug] from comment #14)
> >+namespace ipc {
> >+
> >+struct MessageListenerInfo
> >+{
> >+  nsCOMPtr<nsIMessageListener> mListener;
> >+  nsCOMPtr<nsIAtom> mMessage;
> >+};
> Why do we need this here?

Oh I just moved nsMessageListenerInfo from the global namespace to mozilla::dom::ipc. It's a somewhat unrelated clean up. Maybe it's not even a clean up in your eyes, which is fair. I can revert it if you want.

> >+  nsRefPtr<mozilla::dom::ipc::MessageManagerCallback> mCallback;
> this is a bit worrisome. MM owns the callback, and at least in some cases
> callback owns the mm,
> yet the callback object is not cycle collected. What breaks the cycle.

Hmm good point. Should we require that whoever owns the MM should also own the callback? Basically, require the consumer to make sure refcounting is done right. This would work ok in for the nsFrameLoader situation when nsFrameLoader simply passes itself as the callback. For process message managers, we would move the callback creation into ContentParent: https://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#725. ContentParent would simply have to make sure to hold on to the callback object. Or, we could do the same thing as with nsFrameLoader and make ContentParent the callback object itself.

Thoughts?
Whiteboard: [WebAPI:P1] → [WebAPI:P1][LOE:M]
Attached patch WIP 4 (obsolete) (deleted) — — Splinter Review 
Alright I went ahead with what I outlined in comment 15. Tests are still WIP and crashy, I'm working on debugging them.
Attachment #659883 - Attachment is obsolete: true
Attachment #662297 - Flags: feedback?(bugs)
Comment on attachment 662297 [details] [diff] [review]
WIP 4


>+++ b/content/base/src/nsCCUncollectableMarker.cpp
>@@ -115,21 +115,21 @@ MarkMessageManagers()
>       nsCOMPtr<nsIMessageListenerManager> childMM;
>       windowMM->GetChildAt(j, getter_AddRefs(childMM));
>       if (!childMM) {
>         continue;
>       }
>       nsCOMPtr<nsIMessageSender> tabMM = do_QueryInterface(childMM);
>       tabMM->MarkForCC();
>       //XXX hack warning, but works, since we know that
>-      //    callback data is frameloader.
>-      void* cb = static_cast<nsFrameMessageManager*>(tabMM.get())->
>-        GetCallbackData();
>-      nsFrameLoader* fl = static_cast<nsFrameLoader*>(cb);
>-      if (fl) {
>+      //    callback is frameloader.
>+      mozilla::dom::ipc::MessageManagerCallback* cb
>+        = static_cast<nsFrameMessageManager*>(tabMM.get())->GetCallback();
= should be in the previous line

>   if (ShouldUseRemoteProcess()) {
>-    mMessageManager = new nsFrameMessageManager(true, /* aChrome */
>-                                                nullptr,
>-                                                SendAsyncMessageToChild,
>-                                                LoadScript,
>-                                                mRemoteBrowserShown ? this : nullptr,
>-                                                static_cast<nsFrameMessageManager*>(parentManager.get()),
>-                                                cx);
>+    mMessageManager = new nsFrameMessageManager(mRemoteBrowserShown ? this : nullptr, /* aCallback */
>+                                                static_cast<nsFrameMessageManager*>(parentManager.get()), /* aParentManager */
>+                                                cx, /* aContext */
>+                                                MM_CHROME);

I don't think /**/ comments make this any easier to understand.




> NS_NewGlobalMessageManager(nsIMessageBroadcaster** aResult)
> {
>   NS_ENSURE_TRUE(IsChromeProcess(), NS_ERROR_NOT_AVAILABLE);
>-  nsFrameMessageManager* mm = new nsFrameMessageManager(true /* aChrome */,
>-                                                        nullptr,
>-                                                        nullptr,
>-                                                        nullptr,
>-                                                        nullptr,
>-                                                        nullptr,
>-                                                        nullptr,
>-                                                        true /* aGlobal */,
>-                                                        false /* aProcessManager */,
>-                                                        true /* aBroadcaster */);
>+  nsFrameMessageManager* mm = new nsFrameMessageManager(nullptr, /* aCallback */
>+                                                        nullptr, /* aParentManager */
>+                                                        nullptr, /* aContext */
>+                                                        MM_CHROME | MM_GLOBAL | MM_BROADCASTER);
>   NS_ENSURE_TRUE(mm, NS_ERROR_OUT_OF_MEMORY);
Want to remove this useless OOM check
Attachment #662297 - Flags: feedback?(bugs) → feedback+
(In reply to Olli Pettay [:smaug] from comment #17)
> >+    mMessageManager = new nsFrameMessageManager(mRemoteBrowserShown ? this : nullptr, /* aCallback */
> >+                                                static_cast<nsFrameMessageManager*>(parentManager.get()), /* aParentManager */
> >+                                                cx, /* aContext */
> >+                                                MM_CHROME);
> 
> I don't think /**/ comments make this any easier to understand.

They're mostly for my benefit, I can rip them out in the final patch.

Thanks for looking over the patch! I should be done with the tests today, will submit for review then.
Attached patch v1 (obsolete) (deleted) — — Splinter Review 
Hookay, I think this is finally ready. I've tested it myself on B2G with the patch in bug 776663 and some additional manual testing. That said, the automated test that's included in this patch doesn't want to pass for some reason. Given that the patch in bug 776663 works, it seems to be a test set up problem and not a problem with the implementation. I will investigate, but I don't think this should block the review any further.

Side note: I discovered that

  messageManager.hasCapability("someperm");

has an interesting side effect. If the respective content process does *not* have the "someperm" permission, it gets *killed*. I did not implement this, this must be part of the AppProcessPermission helpers. While I was a bit surprised at first, I think this side-effect makes perfect sense. It ensures a good balance between only asking for permissions that we really require from the content process, and absolute defense in the deep against content processes, even when they're just coded sloppily. (Sloppy code can lead to security vulnerabilities.)
Attachment #662297 - Attachment is obsolete: true
Attachment #663256 - Flags: review?(bugs)
(In reply to Philipp von Weitershausen [:philikon] from comment #19)
> Created attachment 663256 [details] [diff] [review]
> v1
> 
>   messageManager.hasCapability("someperm");
> 

It's "hasCapability()"?  Would recommend "hasPermission()", for consistency with the AppProcessPermissions.

AppProcessPermissions is the last line of defense against bugs/malfeasance.  It's not the primary permission check; that happens in the DOM in content processes.  HasPermission() failing in the master process is evidence of something not behaving properly.  Safest thing to do is machine-gun things that are acting up.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #20)
> (In reply to Philipp von Weitershausen [:philikon] from comment #19)
> > Created attachment 663256 [details] [diff] [review]
> > v1
> > 
> >   messageManager.hasCapability("someperm");
> > 
> 
> It's "hasCapability()"?

Yes, according to your API sketch in attachment 645208 [details] [diff] [review].


> Would recommend "hasPermission()", for consistency with the AppProcessPermissions.

Fine by me!

> AppProcessPermissions is the last line of defense against bugs/malfeasance. 
> It's not the primary permission check; that happens in the DOM in content
> processes.

Yes, fully aware of that.

> HasPermission() failing in the master process is evidence of
> something not behaving properly.  Safest thing to do is machine-gun things
> that are acting up.

Exactly. In the best case it's sloppy programming in DOM code, in the worst case it's an actual content process being pwned.
(In reply to Philipp von Weitershausen [:philikon] from comment #21)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #20)
> > (In reply to Philipp von Weitershausen [:philikon] from comment #19)
> > > Created attachment 663256 [details] [diff] [review]
> > > v1
> > > 
> > >   messageManager.hasCapability("someperm");
> > > 
> > 
> > It's "hasCapability()"?
> 
> Yes, according to your API sketch in attachment 645208 [details] [diff] [review]
> [review].
> 

D'oh!  That's what the AppProcess* stuff was originally called too, until we went through a few rounds of bikesheddeding.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #22)
> > > It's "hasCapability()"?
> > 
> > Yes, according to your API sketch in attachment 645208 [details] [diff] [review]
> > [review].
> > 
> 
> D'oh!  That's what the AppProcess* stuff was originally called too, until we
> went through a few rounds of bikesheddeding.

Heh ok. I'll change it to hasPermission().
Uh, hasPermission or hasCapability method shouldn't cause any crashes. The method name doesn't
indicate anything like that. Method should just return true or false and have no side
effects.
Do we need another API, ensurePermission or some such, where the name reasonable clearly indicates
that one really needs to have the permissions.
(In reply to Olli Pettay [:smaug] from comment #24)
> Uh, hasPermission or hasCapability method shouldn't cause any crashes.

Crashes is a strong word that seems to indicate erratic behaviour. Terminating the content process is quite deliberate in this case.

> Do we need another API, ensurePermission or some such, where the name
> reasonable clearly indicates that one really needs to have the permissions.

I don't care about the exact name. I don't think we need a separate API. I think the semantics that are implemented right now are what we want. Calling it `ensurePermission` seems fine to me.
I just want to be sure what we really want.
So the API isn't about checking capabilities, like the summary of this bug says, 
but about some sort of assertions to make sure that the client process has the
permissions to do X.
Actually, perhaps assertPermission would be even better name.
I want the method name to hint clearly that if it fails, something fatal happens (meaning, 
the client will be terminated).
(In reply to Olli Pettay [:smaug] from comment #26)
> I just want to be sure what we really want.
> So the API isn't about checking capabilities, like the summary of this bug
> says, but about some sort of assertions to make sure that the client process has
> the permissions to do X.

Yes, I guess it should be "check and enforce", not just "check".

> Actually, perhaps assertPermission would be even better name.

I will paint this bikeshed whichever colour you want.
Comment on attachment 663256 [details] [diff] [review]
v1


>+[scriptable, builtinclass, uuid(5f552699-01a2-4f17-833b-ddb3fa0d98b2)]
>+interface nsICapabilitiesChecker : nsISupports
So this should be called something else now.
Perhaps nsIPermissionChecker


>+{
>+
>+  /**
>+   * Return true iff our "remote" content has |aCapability|.  This is
>+   * intended to be used by JS implementations of cross-process DOM
>+   * APIs, like so
>+   *
>+   *   recvFooRequest: function(message) {
>+   *     if (!message.target.hasCapability("foo")) {
>+   *       return false;
>+   *     }
>+   *     // service foo request
>+   *
>+   * This interface only returns meaningful data when our content is
>+   * in a separate process.  If it shares the same OS process as us,
>+   * then applying this capability check doesn't add any security,
>+   * though it doesn't hurt anything either.
>+   */
>+  boolean hasCapability(in DOMString aCapability);

And this would be then
boolean assertPermission() and document what failure causes.


>+/**
>+ * Send messages to an imaginary child process in a single-process scenario.
>+ */
>+class SameChildProcessMessageManagerCallback : public MessageManagerCallback
Please add MOZ_COUNT_CTOR and MOZ_COUNT_DTOR so that it is easier to detect leaks.

>+/**
>+ * Send messages to the imaginary parent process in a single-process scenario.
>+ */
>+class SameParentProcessMessageManagerCallback : public MessageManagerCallback
Please add MOZ_COUNT_CTOR and MOZ_COUNT_DTOR so that it is easier to detect leaks.


> // This creates the global parent process message manager.
> nsresult
> NS_NewParentProcessMessageManager(nsIMessageBroadcaster** aResult)
> {
>   NS_ASSERTION(!nsFrameMessageManager::sParentProcessManager,
>                "Re-creating sParentProcessManager");
>   NS_ENSURE_TRUE(IsChromeProcess(), NS_ERROR_NOT_AVAILABLE);
>-  nsRefPtr<nsFrameMessageManager> mm = new nsFrameMessageManager(true /* aChrome */,
>-                                                                 nullptr,
>-                                                                 nullptr,
>-                                                                 nullptr,
>-                                                                 nullptr,
>-                                                                 nullptr,
>-                                                                 nullptr,
>-                                                                 false, /* aGlobal */
>-                                                                 true /* aProcessManager */,
>-                                                                 true /* aBroadcaster */);
>+  nsRefPtr<nsFrameMessageManager> mm = new nsFrameMessageManager(nullptr, /* aCallback */
>+                                                                 nullptr, /* aParentManager */
>+                                                                 nullptr, /* aContext */
>+                                                                 MM_CHROME | MM_PROCESSMANAGER | MM_BROADCASTER);
>   NS_ENSURE_TRUE(mm, NS_ERROR_OUT_OF_MEMORY);
>   nsFrameMessageManager::sParentProcessManager = mm;
>   nsFrameMessageManager::NewProcessMessageManager(nullptr); // Create same process message manager.
>   return CallQueryInterface(mm, aResult);
> }
> 
>+
> nsFrameMessageManager*
> nsFrameMessageManager::NewProcessMessageManager(mozilla::dom::ContentParent* aProcess)
> {
>   if (!nsFrameMessageManager::sParentProcessManager) {
>      nsCOMPtr<nsIMessageBroadcaster> dummy;
>      NS_NewParentProcessMessageManager(getter_AddRefs(dummy));
>   }
> 
>-  nsFrameMessageManager* mm = new nsFrameMessageManager(true /* aChrome */,
>-                                                        nullptr,
>-                                                        aProcess ? SendAsyncMessageToChildProcess
>-                                                                 : SendAsyncMessageToSameProcessChild,
>-                                                        nullptr,
>-                                                        aProcess ? static_cast<void*>(aProcess)
>-                                                                 : static_cast<void*>(&nsFrameMessageManager::sChildProcessManager),
>-                                                        nsFrameMessageManager::sParentProcessManager,
>-                                                        nullptr,
>-                                                        false, /* aGlobal */
>-                                                        true /* aProcessManager */);
>+  MessageManagerCallback* cb;
>+  if (aProcess) {
>+    cb = aProcess;
>+  } else {
>+    cb = new SameChildProcessMessageManagerCallback();
>+  }
>+
>+  nsFrameMessageManager* mm = new nsFrameMessageManager(cb, /* aCallback */
>+                                                        nsFrameMessageManager::sParentProcessManager, /* aParentManager */
>+                                                        nullptr, /* aContext */
>+                                                        MM_CHROME | MM_PROCESSMANAGER);
Who owns the callback cb? Who will delete it?


> 
> nsresult
> NS_NewChildProcessMessageManager(nsISyncMessageSender** aResult)
> {
>   NS_ASSERTION(!nsFrameMessageManager::sChildProcessManager,
>                "Re-creating sChildProcessManager");
>-  bool isChrome = IsChromeProcess();
>-  nsFrameMessageManager* mm = new nsFrameMessageManager(false /* aChrome */,
>-                                                        isChrome ? SendSyncMessageToSameProcessParent
>-                                                                 : SendSyncMessageToParentProcess,
>-                                                        isChrome ? SendAsyncMessageToSameProcessParent
>-                                                                 : SendAsyncMessageToParentProcess,
>-                                                        nullptr,
>-                                                        &nsFrameMessageManager::sChildProcessManager,
>-                                                        nullptr,
>-                                                        nullptr,
>-                                                        false /* aGlobal */,
>-                                                        true /* aProcessManager */);
>+
>+  MessageManagerCallback* cb;
>+  if (IsChromeProcess()) {
>+    cb = new SameParentProcessMessageManagerCallback();
>+  } else {
>+    cb = new ParentProcessMessageManagerCallback();
>+  }
>+  nsFrameMessageManager* mm = new nsFrameMessageManager(cb, /* aCallback */
>+                                                        nullptr, /* aParentManager */
>+                                                        nullptr, /* aContext */
>+                                                        MM_PROCESSMANAGER);
Who owns the callbacks? Who will delete them?


Also, do you have naming wrong. You create ParentProcessMessageManagerCallback in NS_NewChildProcessMessageManager


>+namespace ipc {
>+
>+struct MessageListenerInfo
>+{
>+  nsCOMPtr<nsIMessageListener> mListener;
>+  nsCOMPtr<nsIAtom> mMessage;
>+};
Don't move MessageListenerInfo here.
It ends up being in wrong namespace. It isn't an ipc thingie.




>+  virtual bool CheckCapability(const nsAString& aCapability)
>+  {
>+    return false;
>+  }
AssertPermission

> #endif
>   InitTabChildGlobal();
>   NS_WARN_IF_FALSE(NS_SUCCEEDED(rv),
>                    "Couldn't initialize nsInProcessTabChildGlobal");
>-  mMessageManager = new nsFrameMessageManager(false, /* aChrome */
>-                                              SendSyncMessageToParent,
>-                                              SendAsyncMessageToParent,
>-                                              nullptr,
>-                                              this,
>-                                              nullptr,
>-                                              mCx);
>+  mMessageManager = new nsFrameMessageManager(this, /* aCallback */
>+                                              nullptr, /* aParentManager */
>+                                              mCx, /* aContext */
>+                                              0);
Could you add a flag for child too. Its value could be 0.



> void
> TabChildGlobal::Init()
> {
>   NS_ASSERTION(!mMessageManager, "Re-initializing?!?");
>-  mMessageManager = new nsFrameMessageManager(false, /* aChrome */
>-                                              SendSyncMessageToParent,
>-                                              SendAsyncMessageToParent,
>-                                              nullptr,
>-                                              mTabChild,
>-                                              nullptr,
>-                                              mTabChild->GetJSContext());
>+  mMessageManager = new nsFrameMessageManager(mTabChild, /* aCallback */
>+                                              nullptr, /* aParentManager */
>+                                              mTabChild->GetJSContext(), /* aContext */
>+                                              0);
> }
Could you add a flag for child too. Its value could be 0.



r-, because if I read the code correctly, it leaks those callbacks.
Attachment #663256 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #28)
> >+  MessageManagerCallback* cb;
> >+  if (IsChromeProcess()) {
> >+    cb = new SameParentProcessMessageManagerCallback();
> >+  } else {
> >+    cb = new ParentProcessMessageManagerCallback();
> >+  }
> >+  nsFrameMessageManager* mm = new nsFrameMessageManager(cb, /* aCallback */
> >+                                                        nullptr, /* aParentManager */
> >+                                                        nullptr, /* aContext */
> >+                                                        MM_PROCESSMANAGER);
> Who owns the callbacks? Who will delete them?

As per comment 15, whoever owns the nsFrameMessageManager instance also needs to own the callback. In both cases where you've asked this question, the nsFrameMessageManager instance gets assigned to a static variable (sChildProcessManager, sSameProcessParentManager). I see now that these are cleaned up in ~nsFrameMessageManager(), so I'll add a cleanup for the callback there as well.

> Also, do you have naming wrong. You create
> ParentProcessMessageManagerCallback in NS_NewChildProcessMessageManager

Heh, I see how that's confusing, yes. I basically renamed Send[Async|Sync]MessagetoParentProcess() to ParentProcessMessageManagerCallback without giving it much further thought. I'll make it less confusing.

Thanks for the review! I'll have an updated patch shortly.
Attached patch v2 (obsolete) (deleted) — — Splinter Review 
Addressed review comments and also made tests pass. \o/

Try build: https://tbpl.mozilla.org/?tree=Try&rev=7c4f676b16e5
Attachment #663256 - Attachment is obsolete: true
Attachment #663607 - Flags: review?(bugs)
Attached patch v3 (obsolete) (deleted) — — Splinter Review 
Now with 100% less leaks!

SameParentProcessMessageManagerCallback was leaking because Disconnect() would set it mCallback to null before the dtor could free it. bent suggested using an nsAutoPtr in case nsFrameMessageManager actually owns the callback, which is in only two cases, SameParentProcessMessageManagerCallback being one of them.

New try build: https://tbpl.mozilla.org/?tree=Try&rev=339d31272b5d
Attachment #663607 - Attachment is obsolete: true
Attachment #663607 - Flags: review?(bugs)
Attachment #663629 - Flags: review?(bugs)
Comment on attachment 663629 [details] [diff] [review]
v3

MessageManagerCallback shouldn't be in mozilla::dom::ipc namespace but in
mozilla::dom

>+[scriptable, builtinclass, uuid(5f552699-01a2-4f17-833b-ddb3fa0d98b2)]
>+interface nsIPermissionChecker : nsISupports
>+{
>+
>+  /**
>+   * Return true iff the "remote" process has |aPermission|.  This is
>+   * intended to be used by JS implementations of cross-process DOM
>+   * APIs, like so
>+   *
>+   *   recvFooRequest: function(message) {
>+   *     if (!message.target.hasCapability("foo")) {
assertPermission("foo")


>+NS_IMETHODIMP
>+nsFrameMessageManager::AssertPermission(const nsAString& aPermission, bool* canHazPermission)
aHasPermission


>+  virtual bool DoSendAsyncMessage(const nsAString& aMessage,
>+                                const StructuredCloneData& aData)
align parameters

>+    mOwnedCallback(nullptr),
No need to initialize nsAutoPtr to null.
Attachment #663629 - Flags: review?(bugs) → review+
I like "assertPermission()".  Please file a followup for changing AppProcessPermissions, or we can just do the trivial patch here.  (It's very important that the APIs are the same, moreso than the actual API itself.)
Depends on: 793479
(In reply to Chris Jones [:cjones] [:warhammer] from comment #33)
> I like "assertPermission()".  Please file a followup for changing
> AppProcessPermissions, or we can just do the trivial patch here.  (It's very
> important that the APIs are the same, moreso than the actual API itself.)

Filed bug 793479.
https://tbpl.mozilla.org/?tree=Try&rev=339d31272b5d doesn't look like unicorns and dandelions. The reason is the following:

dom/browser-element/mochitest/test_browserElement_{inproc|oop}_DOMRequestError.html creates an iframe mozbrowser, attaches it to the DOM, and takes a screenshot of the iframe. Then it detaches the iframe, takes another screenshot, and expects this to fail with a DOMRequest error. This makes sense. But it no longer happens with the patch applied.

The way the failure arises in the old implementation is that nsFrameLoader sets the message manager's mCallbackData to null when the iframe is no longer rendered [1]. When you then try to call mm.sendAsyncMessage(), we throw NS_ERROR_NOT_INITIALIZED if mCallbackData isn't null [2]. This then gets converted to a DOMRequest error by BrowserElementParent [3,4].

[1] https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameLoader.cpp#2339
[2] https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameMessageManager.cpp#289
[3] https://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementParent.js#323
[4] https://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementParent.js#462


In my implementation, the entire mCallback is set to null. Having a null mCallback is a valid scenario, so we no longer throw NS_ERROR_NOT_INITIALIZED. Which means the message is just silently dropped.

I see two possible fixes:

(a) We make MessageManagerCallback methods return nsresult (or take a Paris binding-style ErrorResult&) so that nsFrameLoader can be in charge of throwing things when !mRemoteBrowserShown.

(b) We add a CanReceiveMessages() method to MessageManagerCallback that returns true for all implementation but in nsFrameLoader it returns mRemoteBrowserShown (or something like that). If !callback->CanReceiveMessages(), we raise NS_ERROR_NOT_INITIALIZED or perhaps a more appropriate value, like NS_ERROR_NOT_AVAILABLE.
I forgot:

(c) We do NS_ENSURE_TRUE(mCallback, NS_ERROR_NOT_INITIALIZED); for non-broadcasters. This *should* restore the old logic, and smaug seems to agree on IRC. On top of that, we can enforce that broadcasters don't even get down this path.
Yeah, I like (c).
We can leave the broadcaster enforcing thingie to a followup, if needed.
Attached patch v4 (obsolete) (deleted) — — Splinter Review 
Implemented (c) as suggested by smaug. The previously failing tests now pass locally. \o/ Let's see how the rest is going to hold up: https://tbpl.mozilla.org/?tree=Try&rev=cfbb2b8a8728

Also rebased on top of bug 793479.
Attachment #663629 - Attachment is obsolete: true
(In reply to Philipp von Weitershausen [:philikon] from comment #38)
> Created attachment 663858 [details] [diff] [review]
> v4

(Of course this also addressed smaug's review comments from comment 32.)
(In reply to Philipp von Weitershausen [:philikon] from comment #38)
> Implemented (c) as suggested by smaug. The previously failing tests now pass
> locally. \o/ Let's see how the rest is going to hold up:
> https://tbpl.mozilla.org/?tree=Try&rev=cfbb2b8a8728

That content/base/test/test_mixed_content_blocker.html failure on Windows opt is a bit worrying. Pushed off another try build on top of m-c tip while I'm building on Windows myself to potentially debug (FML): https://tbpl.mozilla.org/?tree=Try&rev=415b09b120d7
(In reply to Philipp von Weitershausen [:philikon] from comment #40)
> (In reply to Philipp von Weitershausen [:philikon] from comment #38)
> > Implemented (c) as suggested by smaug. The previously failing tests now pass
> > locally. \o/ Let's see how the rest is going to hold up:
> > https://tbpl.mozilla.org/?tree=Try&rev=cfbb2b8a8728
> 
> That content/base/test/test_mixed_content_blocker.html failure on Windows
> opt is a bit worrying. Pushed off another try build on top of m-c tip while
> I'm building on Windows myself to potentially debug (FML):
> https://tbpl.mozilla.org/?tree=Try&rev=415b09b120d7

The Win opt run is green on this run (WinXP opt hasn't started yet). Locally I cannot reproduce a crash or timeout. With and without the patch, on both opt and debug builds, I *do* see a stack being printed if I touch the browser UI after the test run (http://philikon.pastebin.mozilla.org/1842975) but that seems unrelated.

Going to land this thing.
https://hg.mozilla.org/integration/mozilla-inbound/rev/be95f234ff6e
Summary: Add a JS helper to check capabilities of apps through messagemanager → Add a scriptable API to assert app permissions of apps through messagemanager
Target Milestone: --- → mozilla18
Backed out for various M1 failure:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb8a3fd355ba
Target Milestone: mozilla18 → ---
Summary: Add a scriptable API to assert app permissions of apps through messagemanager → Add a scriptable API to assert app permissions through messagemanager
Attached patch v5 (deleted) — — Splinter Review 
Going to have to disable the tests on Windows and Android for the same reason as bug 762049 comment 16. Bug 763081 is on file to re-enable them at a later time.

I figured out the missing leak log failure. When we purposely kill a tab process, we need to leave something behind in the leak log to let the test harness know that it was killed on purpose. This patch does this now.

Actual code remains unchanged. If https://tbpl.mozilla.org/?tree=Try&rev=089fccef01d4 goes green, I'm going to land this.
Attachment #663858 - Attachment is obsolete: true
(In reply to Philipp von Weitershausen [:philikon] from comment #44)
> Actual code remains unchanged. If
> https://tbpl.mozilla.org/?tree=Try&rev=089fccef01d4 goes green, I'm going to
> land this.

I pushed prematurely. New try build: https://tbpl.mozilla.org/?tree=Try&rev=5c4ca3fb9423
The three changesets in the push backed out for causing (/conflicting with the backout of):
https://tbpl.mozilla.org/php/getParsedLog.php?id=15626544&tree=Mozilla-Inbound

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc9d251fdb2e
Sorry Phil, our log parsers aren't smart enough to figure out that in

INFO TEST-PASS | /tests/content/base/test/test_child_process_shutdown_message.html | Observed fatal error event. - fatal should equal fatal

"TEST-PASS" means that a test passed.  So we have to change the strings that are used in the comparisons here.
sed -i s'/Observed fatal error event/Observed expected event/'g *
Depends on: 795298
Really sorry about that, my brain is attuned to reading TEST-UNEXPECTED-FAIL all the time, so completely missed that it was a PASS.

I've filed bug 795298 to make the TBPL parser regex exclude TEST-PASS even if another condition matches (in this case the string "fatal error").
(In reply to Ed Morley [:edmorley UTC+1] from comment #50)
> Really sorry about that, my brain is attuned to reading TEST-UNEXPECTED-FAIL
> all the time, so completely missed that it was a PASS.
> 

No worries at all!  Everyone appreciates the work you do :).  This is entirely tbpl's fault.

> I've filed bug 795298 to make the TBPL parser regex exclude TEST-PASS even
> if another condition matches (in this case the string "fatal error").

Great.  In parallel I'll see if I can find someone to take over my log parser, which distinguished different build steps and was pretty carefully written to be fast (~1GB/s on my fast machine).

(In reply to Chris Jones [:cjones] [:warhammer] from comment #49)
> sed -i s'/Observed fatal error event/Observed expected event/'g *

To be clear, I applied this change locally and will reland asap (if I'm still alive).
Thanks, Chris & Ed!
https://hg.mozilla.org/mozilla-central/rev/e393ae7353c0
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
(In reply to Chris Jones [:cjones] [:warhammer] from comment #51)
> Great.  In parallel I'll see if I can find someone to take over my log
> parser, which distinguished different build steps and was pretty carefully
> written to be fast (~1GB/s on my fast machine).

That's the first I've heard about this - where can I find it? (Sounds great; particularly differientiating the build steps, which is on my list of things to tackle soon!) :-)
Depends on: 796930
Depends on: 820353
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: