542242 - E10s, content process event handlers

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

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

I should have a patch (without CPOW) ready for the first review pretty soon, 
hopefully later today.

Comment 1

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

Attachment: WIP

This works reasonable well. Needs testing though.

chrome frameloaders and window object have .messageManager which can be
used to send async messages and to add message listeners.

content root scope implements similar messageManager interface, though it is
possible to send also sync messages. content root scope implements also
nsIDOMEventTarget, so that is can get events from content and there is 
also nsIDOMWindowRootScope, which has just .content and dump(in DOMString msg)

Sending CPOWs from content to chrome is not supported.

uploading here so that I have a backup.

Comment 2

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

Attachment: up-to-date

Comment 3

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

Attachment: Some changes to make XPCNativeWrappers work in the listener context

Benjamin, could you perhaps look at the APIs here.
The scope for content process listeners implements nsIDOMEventTarget,
nsIDOMWindowRootScope and nsIContentFrameMessageManager (which extends nsIFrameMessageManager).

The test.xul+remote-test.js has a bit silly test to check that different kinds
of message handling work when (for example) clicking links.

Comment 4

[Josh Matthews [:jdm]]

I think remote-test.js is missing from the patch.

Comment 5

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

+++ e10s/dom/ipc/remote-test.js	2010-01-29 23:45:42.568737185 +0200

I can sure see it there: :)

Comment 6

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

Seems like nsIDOMWindowRootScope.idl
has extra #include "nsIDOMEventTarget.idl". That prevents opt compilation.

Comment 7

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

Attachment: Some more tests in test.xul

Comment 8

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

Comment on attachment 424782 [details] [diff] [review]
Some more tests in test.xul

Jst, could you look at the content/ and dom/ and script context creation parts?

I'll improve the script loading in the followup bug.

Comment 9

[Benjamin Smedberg]

Comment on attachment 424782 [details] [diff] [review]
Some more tests in test.xul

I did not review the guts of the DOM stuff very carefully, since I don't really know those details, but I did review the API surface and the IPDL bits, as well as the test changes, and they all appear reasonable.

Comment 10

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

Attachment: v7

This should fix the crash jdm was seeing.
unlinking messagemanager didn't disconnect child managers.

Comment 11

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

Attachment: v8

applies to current e10s branch

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 426257 [details] [diff] [review]
v8

- In nsScriptSecurityManager::GetPrincipalFromContext():

+    nsIScriptContextPrincipal* scp =
+        GetScriptContextPrincipalFromJSContext(cx);
+
+    if (!scp)
     {
         return NS_ERROR_FAILURE;

Seems like we'll always hit this, since neither of the two classes that implement nsIScriptContextPrincipal claim they do in their QI implementations, and GetScriptContextPrincipalFromJSContext() QI's.

- In nsCxPusher::Push(nsPIDOMEventTarget *...):

   nsIScriptContext* scx =
     aCurrentTarget->GetContextForEventHandlers(&rv);
   NS_ENSURE_SUCCESS(rv, PR_FALSE);
 
   if (!scx) {
+    JSContext* cx = aCurrentTarget->GetJSContextForEventHandlers();
+    if (cx) {
+      DoPush(cx);
+    }
     // Nothing to do here, I guess.  Have to return true so that event firing
     // will still work correctly even if there is no associated JSContext

The comment should be updated now...

     return PR_TRUE;
   }

That's all for now, next up is to continue in TabChild.cpp. And I still don't have a clear picture of exactly what the root context thing is really for, but as I read through more of this I'm sure it'll become clearer...

Comment 13

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

(In reply to comment #12)
> (From update of attachment 426257 [details] [diff] [review])
> - In nsScriptSecurityManager::GetPrincipalFromContext():
> 
> +    nsIScriptContextPrincipal* scp =
> +        GetScriptContextPrincipalFromJSContext(cx);
> +

Oops. How did this work at all. I'll test.

Comment 14

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

Attachment: v9

Added the QI impls and a comment to ::Push

Comment 15

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

(In reply to comment #12)
> And I still don't
> have a clear picture of exactly what the root context thing is really for, but
> as I read through more of this I'm sure it'll become clearer...

We don't want to forward all the events to the chrome process.
If we can handle the events in content process, we don't need to send anything
to the chrome process (and maybe block it). And if something is needed on 
chrome side, using async and/or sync messaging from content to chrome process
is possible.

So the root context is for the content process event listeners. Something which
is above the top level DOMWindow, something which stays there even if new
pages are loaded. 

Chrome asks content process to load some script in the root context and the
idea
is that the script would add event listeners to the root context. Then
those event listeners can do whatever they do, for example send messages
to chrome process.
(Currently for example ctrl+click is handled in the Firefox UI code, and
 I expect that code to move partially to content process root context.)

There is also additional support for messages coming from chrome process to
content process. That is always asynchronous. If nothing else, that is great
for testing and debugging.

Comment 16

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

Attachment: better documentation in .idl

Added also few tests and made the message properties enumerable.
And don't push context to stack if there are no message listeners.

Comment 17

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

Attachment: merge windowrootscope and contentframemessagemanager

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 427627 [details] [diff] [review]
merge windowrootscope and contentframemessagemanager

- In nsGlobalChromeWindow::GetMessageManager():

+  return CallQueryInterface(mMessageManager.get(), aManager);

Do we really need to QI here every time, can't we just cast and AddRef() here?

- In dom/ipc/PIFrameEmbedding.ipdl:

+
+    sync sendSyncMessage(nsString aMessage, nsString aJSON) returns (nsString[] retval);
+    sendAsyncMessage(nsString aMessage, nsString aJSON);
[...]
+    loadRemoteScript(nsString aURL);
+
+    sendAsyncMessage2(nsString aMessage, nsString aJSON);

This naming is somewhat unfortunate, is there no better way to do this in IPDL?

- In TabChild::InitRootContext():

Could we rename this to InitFrameMessageManager() and merge RootContextScope into nsFrameMessageManager?

- In nsFrameMessageManager::Dump():

+  fputs(NS_ConvertUTF16toUTF8(aStr).get(), stdout);
+  fflush(stdout);

The IDL says this uses stderr, either way is fine with me, but be consistent :)

Other than that this looks good. I'd like to look over an updated patch once more (the RootContextScope/nsFrameMessageManager merge in particular, assuming we can do that), so r- until then.

Comment 19

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

(In reply to comment #18)
> This naming is somewhat unfortunate, is there no better way to do this in IPDL?

IPDL doesn't seem to allow same name for parent->child message and for
child->parent.
But perhaps I could rename those to sendAsyncMessageToParent and sendAsyncMessageToChild. That would be good enough, I think.

> 
> - In TabChild::InitRootContext():
> 
> Could we rename this to InitFrameMessageManager() and merge RootContextScope
> into nsFrameMessageManager?
nsFrameMessageManager is really just about sending and receiving messages.
RootContextScope adds support to event handling etc. so I would not want to
merge them. RootContextScope is used only in content process, nsFrameMessageManager is used also in chrome process.
I know the nsIDOMWindowRootScope and nsIContentFrameMessageManager merge
made the interface a bit uglier, but still the implementation of 
nsFrameMessageManager is focused on message handling.
RootContextScope is the object which *extends* nsFrameMessageManager so that
the root js scope has all the necessary features.

> The IDL says this uses stderr, either way is fine with me, but be consistent :)
Oops, I should have written stdout in the idl.

Comment 20

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

Attachment: v12

This is without merging RootContextScope and FrameMessageManager, since the
whole point of RootContextScope is to add additional features to FrameMessageManager.
And RootContextScope is good enough name for the object, which represents the
scope in root context, IMO.

Comment 21

[Johnny Stenback (:jst)]

I really don't like the name RootContextScope because there's IMO already too much confusion about what "scope" and "context" mean in mozilla code, in particular around code that deals with JS. Given that this is the global object for JS execution, I don't want this to be confused with or assumed to be a JSContext, which is what "context" is mostly used in the code around this stuff, and while this is technically a JS scope object, I think we'd be better off not adding more confusion relating to scopes either.

How about we name this TabChildRoot, TabChildGlobal, MessageManagerGlobal, or something along those lines?

Comment 22

[Johnny Stenback (:jst)]

Comment on attachment 427747 [details] [diff] [review]
v12

r=jst, but I'd really like to see a new name per the previous comment.

Comment 23

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

Ok, I'll change it to TabChildGlobal.

Comment 24

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

http://hg.mozilla.org/projects/electrolysis/rev/416375517820

I'll mark this fixed, if everything stays green.

Comment 25

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

Some documentation https://wiki.mozilla.org/Content_Process_Event_Handlers