Closed Bug 564382 Opened 12 years ago Closed 12 years ago

e10s: Remove CPOWs from PContentProcess tree

Categories

(Core :: IPC, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jdm, Assigned: jdm)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 10 obsolete files)

The word from those is the know is that they've got to go.
Blocks: 559200
Depends on: 562414
Attached patch WIP (obsolete) — Splinter Review
Right now I'm not just cutting CPOWs, but replacing them with Handles instead.  benjamn, is this the right way?  I'd also especially appreciate your input on the //XXX line in there, because I'm having trouble figuring out what is supposed to be used instead.
Assignee: nobody → josh
Attachment #444052 - Flags: feedback?(mozilla+ben)
Comment on attachment 444052 [details] [diff] [review]
WIP

(In reply to comment #1)
> Right now I'm not just cutting CPOWs, but replacing them with Handles instead. 
> benjamn, is this the right way?

It's certainly not backwards compatible in any way, but no promises were made to that effect.

I don't see the need for more than one Handle.  Can you think of any?

> I'd also especially appreciate your input on
> the //XXX line in there, because I'm having trouble figuring out what is
> supposed to be used instead.

Hmm.  This issue argues for bsmedberg's original design of letting handles have an .object property whose properties the client can modify.  Then the obj you're passing could just become that property.  I'm tempted to switch to that, since any further expansion of the API exposed by handle objects (.createHandle(), .isValid, .invalidate()) risks conflicting with client-defined object properties, anyway.
Attachment #444052 - Flags: feedback?(mozilla+ben) → feedback+
(In reply to comment #2)
> (From update of attachment 444052 [details] [diff] [review])
> (In reply to comment #1)
> > Right now I'm not just cutting CPOWs, but replacing them with Handles instead. 
> > benjamn, is this the right way?
> 
> It's certainly not backwards compatible in any way, but no promises were made
> to that effect.
> 
> I don't see the need for more than one Handle.  Can you think of any?

So instead of sendSyncMessageToParent taking an optional array of CPOWs, it just takes one optional handle instead?
In addition to coping with the renaming of js/src/ipc to js/ipc, this patch partially addresses the XXXjdm discussed above.  That is, if the JSObject you're trying to pass isn't a Handle, it just comes out as null on the other side.  In the future, we can be more forgiving (creating a new Handle and defining an .object property thereon).
What about throwing an exception in that case?
Comment on attachment 444718 [details] [diff] [review]
Update to accomodate bug 565078, and use Handle::FromJSObject for passing Handles in messages.

>-    // GetOrCreateWrapper is null safe!
>-    aObjects.AppendElement(mContextWrapper->GetOrCreateWrapper(obj));
>+    aObjects.AppendElement(HandleChild::FromJSObject(ctx, obj));

This is the only substantive change with this patch.  Basically, I'm proposing to delegate the conversion implementation to HandleChild, so you don't have to worry about it here.
Attachment #444718 - Flags: feedback?(josh)
Comment on attachment 444718 [details] [diff] [review]
Update to accomodate bug 565078, and use Handle::FromJSObject for passing Handles in messages.

I'm a fan of sensible delegation.
Attachment #444718 - Flags: feedback?(josh) → feedback+
Attached patch Patch (obsolete) — Splinter Review
Remove all traces of CPOWs from the PIFrameEmbedding tree and associated content elements.
Attachment #444052 - Attachment is obsolete: true
Attachment #444718 - Attachment is obsolete: true
Attachment #444783 - Flags: review?(mozilla+ben)
Attachment #444783 - Flags: review?(Olli.Pettay)
You should update iid when modifying interfaces.
Comment on attachment 444783 [details] [diff] [review]
Patch

This fixes bug 554913.

https://wiki.mozilla.org/Content_Process_Event_Handlers needs to be updated after this.
And http://mxr.mozilla.org/projects-central/source/electrolysis/content/base/public/nsIFrameMessageManager.idl?mark=54-54#47 needs to be updated when landing this.

You should be able to remove ReportChildAlreadyBlocked() from RecvsendSyncMessageToParent.

I'm not familiar with PHandle, but from messageManager point of view looks ok.
Attachment #444783 - Flags: review?(Olli.Pettay) → review+
(Btw, is there a bug open to remove rpc createWindow() ?)
Comment on attachment 444783 [details] [diff] [review]
Patch

Thanks for going to the trouble of replicating the makeRoot/mRootHandle logic from ContextWrapper{Parent,Child}.h, but that's not going to be needed long-term.  I'm planning to expose nsIFrameMessageManager.createHandle() and a similar method on TabChild::mCx::globalObject, rather than exposing global handle objects.

If you'd prefer, you could try your hand at adding those methods yourself, if you want to learn about nsIVariants and defining functions using the JSAPI.
Attachment #444783 - Flags: review?(mozilla+ben) → review+
(In reply to comment #12)
> If you'd prefer, you could try your hand at adding those methods yourself, if
> you want to learn about nsIVariants and defining functions using the JSAPI.

If not, feel free just to remove the makeRoot/mRootHandle business :)
Blocks: 554913
Attached patch Patch (obsolete) — Splinter Review
All requested changes made.  I'm really interested in doing the createHandle work, as I'd like to learn more about all that, but that work can happen in another patch if I end up getting to it.
Attachment #444783 - Attachment is obsolete: true
Could you still update test.xul a bit. It has some tests for messageManager+CPOW.
The test works so that you click some link in the page, and
you should get link url + PASS in the statusbar.
If CPOWs are removed those test would break.
Attached patch Patch (obsolete) — Splinter Review
Good call.  I've removed all traces of the CPOW tests for now, as I'm still not quite certain how they translate over to the handles.
Attachment #446178 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
Too much deletion from test.xul.
Attachment #446188 - Attachment is obsolete: true
(In reply to comment #11)
> (Btw, is there a bug open to remove rpc createWindow() ?)

Bug 567058.
Attached patch patch v.1 (obsolete) — Splinter Review
This removes js/ipc and related code.  It does not add back handle support.
Attachment #451407 - Flags: review?(Olli.Pettay)
Attachment #451407 - Flags: review?(Olli.Pettay) → review-
Comment on attachment 451407 [details] [diff] [review]
patch v.1

>+++ b/content/base/public/nsIFrameMessageManager.idl
>@@ -47,17 +47,17 @@ interface nsIFrameMessageListener : nsIS
>   /**
>    * This is for JS only.
>    * receiveMessage is called with one parameter, which has the following
>    * properties:
>    *   {
>    *     name:    %message name%,
>    *     sync:    %true or false%.
>    *     json:    %json object or null%,
>-   *     objects: %array of cpow or null, always null if sync is false%
>+   *     objects: %array of handles or null, always null if sync is false%
Since you're removing 'objects', remove this comment too. 
And also the "@note objects property isn't implemented yet."

We can add those back if we start to use handles.

> bool
> ContentProcessChild::RecvPTestShellConstructor(PTestShellChild* actor)
> {
>-    actor->SendPContextWrapperConstructor()->SendPObjectWrapperConstructor(true);
>-    return true;
>+    return false;
> }
I don't know why this is needed.


>-    PContextWrapper();
>-
>-    rpc sendSyncMessageToParent(nsString aMessage, nsString aJSON, PObjectWrapper[] aObjects)
>+    rpc sendSyncMessageToParent(nsString aMessage, nsString aJSON)
>       returns (nsString[] retval);

The whole point of this bug is to make sendSyncMessageToParent 'sync'.

> rpc protocol PTestShell
> {
>   manager PContentProcess;
> 
>   manages PTestShellCommand;
>-  manages PContextWrapper;
> 
> child:
>   __delete__();
> 
>   ExecuteCommand(nsString aCommand);
> 
>   PTestShellCommand(nsString aCommand);
> 
>-parent:
>-  PContextWrapper();
>-
> };

I don't know about this change either. But I guess it is ok.

>--- a/js/ipc/CPOWTypes.h
>+++ /dev/null
Uh, you're removing CPOWs.
Is that what we want? This bug is not about that.
Could you just remove CPOW usage from sendSencMessage/TabChild/TabParent 
and file a new bug for CPOW removal if that is what we want to do.
(I thought we might want to use CPOWs with jetpacks or something, but I guess I'm
wrong here.)
my misunderstanding... i was under the impression after our conversation that cpows were to be removed.  Please ignore the noise.  (and thanks for the review!)
Attached patch Patch without handles (obsolete) — Splinter Review
All traces of handles removed from the patch.  Make sure I haven't done anything stupid, please?
Attachment #451407 - Attachment is obsolete: true
Attachment #451865 - Flags: review?(Olli.Pettay)
Comment on attachment 451865 [details] [diff] [review]
Patch without handles

      gfxMatrix matrix, Shmem buf);
> 
>     // @param matrix the transformation matrix the context we're going to draw into should have.
>     PDocumentRendererNativeID(PRInt32 x, PRInt32 y, PRInt32 w, PRInt32 h, nsString bgcolor, PRUint32 flags, bool flush,
>-                              gfxMatrix matrix, PRUint32 nativeID);
>+                              gfxMatrix matrix, PRUint32 nativeID);                              
> };
Don't add extra spaces.

> TabParent::ReceiveMessage(const nsString& aMessage,
>                           PRBool aSync,
>                           const nsString& aJSON,
>-                          const nsTArray<PObjectWrapperParent*>* aObjects,
>                           nsTArray<nsString>* aJSONRetVal)
> {
>   nsCOMPtr<nsIFrameLoaderOwner> frameLoaderOwner =
>     do_QueryInterface(mFrameElement);
>   if (frameLoaderOwner) {
>     nsRefPtr<nsFrameLoader> frameLoader = frameLoaderOwner->GetFrameLoader();
>     if (frameLoader && frameLoader->GetFrameMessageManager()) {
>       nsFrameMessageManager* manager = frameLoader->GetFrameMessageManager();
>       JSContext* ctx = manager->GetJSContext();
>       JSAutoRequest ar(ctx);
>       jsval* dest;
>-      PRUint32 len = aObjects ? aObjects->Length() : 0;
>+      PRUint32 len = 0;
Could you add a comment here that once we have handles, the array length
will be > 0


Looks ok to me.
Attachment #451865 - Flags: review?(Olli.Pettay) → review+
No longer depends on: 562414
Attached patch Patch without handles (obsolete) — Splinter Review
Nits addressed.  Let's land this sucker.
Attachment #446190 - Attachment is obsolete: true
Attachment #451865 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [land in e10s]
Depends on: 572827
Attached patch Patch without handles (obsolete) — Splinter Review
Rebased; should hopefully apply cleanly.
Attachment #451951 - Attachment is obsolete: true
Now rebased without the handles patch interfering.
Attachment #452074 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [land in e10s]
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.