582569 - Fire an event in child frame scripts when the TabChild is closing

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

In order to have some sane cleanup of resources in a child/content frame scripts, we need some form of event to tell the scripts that the TabChild is shutting down.

Since TabChildGlobal is a DOMEventTarget, we could fire an event, like "close", and the script could addEventListener("close", ...) to clean up any resources.

Smaug thinks we could safely fire the async event from TabChild::ActorDestroy

Comment 1

[Benjamin Stover (:stechz)]

Attachment: WIP (that doesn't work!)

My first stab at this gives me:
"###!!! ASSERTION: This is unsafe! Fix the caller!:"

And crashes.  I assume I don't want the docshell window.

Comment 2

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

Comment on attachment 464270 [details] [diff] [review]
WIP (that doesn't work!)

> void
> nsInProcessTabChildGlobal::Disconnect()
> {
>   nsCOMPtr<nsIDOMWindow> win = do_GetInterface(mDocShell);
>+
>+    nsCOMPtr<nsIDOMDocument> document;
>+    win->GetDocument(getter_AddRefs(document));
>+    nsCOMPtr<nsIDOMDocumentEvent> docEvent(do_QueryInterface(document));
>+        printf("A\n");
>+    if (docEvent) {
>+        nsCOMPtr<nsIDOMEvent> event;
>+        docEvent->CreateEvent(NS_LITERAL_STRING("Events"), getter_AddRefs(event));
>+
>+        printf("B\n");
>+        if (event) {
>+            event->InitEvent(NS_LITERAL_STRING("MozUnloadFrameListener"), PR_TRUE, PR_FALSE);
>+            nsCOMPtr<nsIPrivateDOMEvent> privateEvent(do_QueryInterface(event));
>+            privateEvent->SetTrusted(PR_TRUE);
>+
>+            nsCOMPtr<nsIDOMEventTarget> target(do_QueryInterface(win));
>+            printf("C\n");
>+            if (target) {
>+                PRBool defaultActionEnabled;
>+                target->DispatchEvent(event, &defaultActionEnabled);
>+                printf("D\n");
>+            }
>+        }
>+    }
>+
>+

You can use nsContentUtils::DispatchTrustedEvent to dispatch events.
But you must not dispatch events at random times (which is why comment 0 says "safely fire the async event from TabChild::ActorDestroy").
You need to keep nsInProcessTabChildGlobal and its JSContext alive and dispatch the event using a script runner.
(See nsContentUtils.h)
But note, at the time the event is dispatched, the chrome message manager may already be disconnected from the content message manager
so send(A)syncMessage may not work. But since this all is just for content script cleanup, I guess that is ok.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

"MozUnloadFrameListener" -> "MozUnloadFrameScript" ?

Comment 4

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

Btw, why not just call the event "close" ? Or "unload"?

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: wip

Another WIP

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

This patch correctly fires an event in the frame scripts when the tab is closing.

I tested in the frame scripts using:

addEventListener("unload", function(e) { dump("---shutting down script\n"); }, false);

Note in the patch that I couldn't use nsContentUtils::DispatchTrustedEvent. The code was failing to get the "target" correctly from the TabChildGlobal. I assume it has to do with how I am static_casting it, which was needed to get past an ambiguity on nsISupports during compile.

If you have a suggestion for how to pass in the TabChildGlobal, I'll gladly switch to use it.

Your comment about needing to make sure the TabChildGlobal stays alive during the event was beyond me as well. It must be alive long enough for my simple event handler to be called. Honestly, I don't see where we kill the TabChildGlobal. It seems to be held by the JS context.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 2

This patch adds the event dispatch to the InProcessTabChildGlobal. I try to make sure the docshell survives longer than the event by delaying the disconnect cleanup until after the event is fired.

I tested this part with local (non-ipc) pages and it seems to work too.

I could change the TabChild code to work like the InProcessTabChildGlobal code: using a runnable method and delaying the Destroy cleanup.

Comment 8

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

Ah, sorry I forgot. nsContentUtils::DispatchTrustedEvent works currently with
nodes.

Comment 9

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

Comment on attachment 464733 [details] [diff] [review]
patch 2


> void
> nsInProcessTabChildGlobal::Disconnect()
> {
>+  // Let the frame scripts know the child is being closed. We do any other
>+  // cleanup after the event has been fired. See DelayedDisconnect
>+  nsContentUtils::AddScriptRunner(
>+     NS_NewRunnableMethod(this, &nsInProcessTabChildGlobal::DelayedDisconnect)
>+  );
>+}

You need to ensure that Disconnect isn't called in the destructor.
As far as I see, the disconnect in dtor is not needed. nsFrameLoader
always calls Disconnect before releasing mChildMessageManager.


>+nsInProcessTabChildGlobal::DelayedDisconnect()
>+{
>   nsCOMPtr<nsIDOMWindow> win = do_GetInterface(mDocShell);
>+
>+  // Fire the "unload" event
>+  nsCOMPtr<nsIDOMDocument> document;
>+  win->GetDocument(getter_AddRefs(document));
>+
>+  nsCOMPtr<nsIDOMDocumentEvent> docEvent(do_QueryInterface(document));
>+  if (docEvent) {
>+    nsCOMPtr<nsIDOMEvent> event;
>+    docEvent->CreateEvent(NS_LITERAL_STRING("Events"), getter_AddRefs(event));
You could call something like
NS_NewDOMEvent(getter_AddRefs(event), nsnull, nsnull);


>+class TabChildEvent : public nsRunnable {
>+public:
>+  TabChildEvent(TabChildGlobal* aTabChildGlobal, const nsAString& aType)
>+    : mTabChildGlobal(aTabChildGlobal), mType(aType)
>+  { }
>+
>+  NS_IMETHOD Run()
>+  {
>+    nsCOMPtr<nsIDOMWindow> window;
>+    mTabChildGlobal->GetContent(getter_AddRefs(window));
>+    nsCOMPtr<nsIDOMDocument> document;
>+    window->GetDocument(getter_AddRefs(document));
>+
>+    nsCOMPtr<nsIDOMDocumentEvent> docEvent(do_QueryInterface(document));
>+    if (docEvent) {
>+      nsCOMPtr<nsIDOMEvent> event;
>+      docEvent->CreateEvent(NS_LITERAL_STRING("Events"), getter_AddRefs(event));
NS_NewDOMEvent(getter_AddRefs(event), nsnull, nsnull);
should work here too.
No need for all the window/document stuff.

>+
>+      if (event) {
>+        event->InitEvent(mType, PR_TRUE, PR_FALSE);
>+        nsCOMPtr<nsIPrivateDOMEvent> privateEvent(do_QueryInterface(event));
>+        privateEvent->SetTrusted(PR_TRUE);
>+        privateEvent->SetTarget(mTabChildGlobal);
No need for this. Target is set when dispatching the event.


>+
>+        PRBool dummy;
>+        mTabChildGlobal->DispatchEvent(event, &dummy);
>+      }
>+    }
>+
>+    //nsCOMPtr<nsIDocument> doc = do_GetInterface(document);
>+    //nsContentUtils::DispatchTrustedEvent(doc, static_cast<nsIDOMEventTarget*>(mTabChildGlobal), mType, PR_TRUE, PR_FALSE);
>+
>+    return NS_OK;
>+  }
>+
>+  TabChildGlobal* mTabChildGlobal;
I think you should have nsRefPtr<TabChild> here.
That way both TabChild and TabChildGlobal and the related JSContext should stay alive
long enough.

>+  nsString mType;
Since the class is used only for unload, perhaps no need for this.
Just use NS_LITERAL_STRING for InitEvent.


But in general, looks pretty much what I had in mind.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 3

Addressed all the review feedback.

Comment 11

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

Comment on attachment 464788 [details] [diff] [review]
patch 3

>+class UnloadScriptEvent : public nsRunnable {
Nit,
'{'
should be in the next line.


> TabChild::RecvDestroy()
> {
>-    DestroyWidget();
>+  // Let the frame scripts know the child is being closed
>+  nsContentUtils::AddScriptRunner(
>+    new UnloadScriptEvent(this, mTabChildGlobal)
>+  );

AFAIK this happens before ActorDestroy.
So messageManager connection is still alive.
I don't think we want that, since it is something we can't guarantee in all
situations, especially in non-e10s case.

Could we move the code from ActorDestroy to happen before nsContentUtils::AddScriptRunner?

This needs some tests. Could be a separate patch.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Commenting at Mark's request.

RecvDestroy() is where we should do all normal shutdown.  The normal shutdown sequence will be
  RecvDestroy()
    [Send__delete__()]
    ActorDestroy()

We don't have a reason yet to care about abnormal shutdown; the content process will just _exit(0) in that case.  I'm all for moving ActorDestroy() code into RecvDestroy(); I didn't do that when I added the Destroy() phase because I don't know what all that code does.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

After some testing and moving code around, it doesn't seem possible to move the code to kill the MessageManager from ActorDestroy to before firing the event in RecvDestroy.

Doing that stops the event handler in the remote script from being called. I fixed the style nit, but have not moved the any other code.

Anything I should try? or can we move ahead with the patch as is?

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Chris - if I leave the code alone, with respect to ActorDestroy, do you still think we need the mDestroyed flag you mentioned in IRC?

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I would still like it, but that's orthogonal to your patch.  We can add that in another bug.

Comment 16

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

(In reply to comment #13)
> After some testing and moving code around, it doesn't seem possible to move the
> code to kill the MessageManager from ActorDestroy to before firing the event in
> RecvDestroy.
> 
> Doing that stops the event handler in the remote script from being called.
I wonder why.

But if that doesn't work, I think we need a boolean flag to prevent
any (a)sync messages during and after 'unload' so that the API works
consistently.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

This was pushed and backed out due to oranges here:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1281632477.1281633423.14661.gz

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

In an effort to see how this code could affect Firefox, only the InProcessTabChildGlobal could even do that, I am pushing to Try with the "unload" event set to _not_ bubble.

Comment 19

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

Uh, right. We're firing unload at wrong time if it affects to Firefox.
It must fire after we set mOwner to null.
So I think if you move mOwner = nsnull; to happen before firing the event,
that should do it.

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 4 (mochitest fix)

This patch is the same as the one I attempted to check-in, except the "unload" is not bubbled. That fixes the Firefox mochitest orange. We really don't want the event to bubble anyway.

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 5 (no escape)

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 465362 [details] [diff] [review]
patch 5 (no escape)

In addition to the can't bubble change, this patch unhooks the mOwner befor firing the event, further ensuring the event will not escape.

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
http://hg.mozilla.org/mozilla-central/rev/541caee57ec7

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

TXul increase on XP Firefox?  http://mzl.la/c3Yofr

Comment 25

[Nickolay_Ponomarev]

dev-doc-needed: this should be mentioned on https://developer.mozilla.org/en/The_message_manager#Content_scripts, right?

Comment 26

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #25)
> dev-doc-needed: this should be mentioned on
> https://developer.mozilla.org/en/The_message_manager#Content_scripts, right?

Yes