E10s, content process event handlers

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(2 attachments, 8 obsolete attachments)

I should have a patch (without CPOW) ready for the first review pretty soon, 
hopefully later today.
Posted patch WIP (obsolete) — Splinter Review
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.
Posted patch up-to-date (obsolete) — Splinter Review
Attachment #423802 - Attachment is obsolete: true
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.
Attachment #424119 - Attachment is obsolete: true
Attachment #424627 - Flags: review?(benjamin)
I think remote-test.js is missing from the patch.
+++ e10s/dom/ipc/remote-test.js	2010-01-29 23:45:42.568737185 +0200

I can sure see it there: :)
Seems like nsIDOMWindowRootScope.idl
has extra #include "nsIDOMEventTarget.idl". That prevents opt compilation.
Posted patch Some more tests in test.xul (obsolete) — Splinter Review
Attachment #424627 - Attachment is obsolete: true
Attachment #424782 - Flags: review?(benjamin)
Attachment #424627 - Flags: review?(benjamin)
Blocks: 543812
Attachment #424782 - Flags: review?(jst)
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 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.
Attachment #424782 - Flags: review?(benjamin) → review+
Posted patch v7 (obsolete) — Splinter Review
This should fix the crash jdm was seeing.
unlinking messagemanager didn't disconnect child managers.
Attachment #424782 - Attachment is obsolete: true
Attachment #426203 - Flags: review?(jst)
Attachment #424782 - Flags: review?(jst)
Posted patch v8 (obsolete) — Splinter Review
applies to current e10s branch
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...
(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.
Posted patch v9 (obsolete) — Splinter Review
Added the QI impls and a comment to ::Push
(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.
Posted patch better documentation in .idl (obsolete) — Splinter Review
Added also few tests and made the message properties enumerable.
And don't push context to stack if there are no message listeners.
Attachment #426203 - Attachment is obsolete: true
Attachment #426257 - Attachment is obsolete: true
Attachment #426834 - Attachment is obsolete: true
Attachment #427302 - Attachment is obsolete: true
Attachment #427627 - Flags: review?(jst)
Attachment #426203 - Flags: review?(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.
Attachment #427627 - Flags: review?(jst) → review-
(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.
Posted patch v12Splinter Review
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.
Attachment #427747 - Flags: review?(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 on attachment 427747 [details] [diff] [review]
v12

r=jst, but I'd really like to see a new name per the previous comment.
Attachment #427747 - Flags: review?(jst) → review+
Ok, I'll change it to TabChildGlobal.
http://hg.mozilla.org/projects/electrolysis/rev/416375517820

I'll mark this fixed, if everything stays green.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Blocks: 549682
You need to log in before you can comment on or make changes to this bug.