Closed Bug 582569 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: IPC, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0

People

(Reporter: mfinkle, Assigned: mfinkle)

References

Details

(Keywords: dev-doc-needed)

Attachments

(3 files, 4 obsolete files)

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
Blocks: 582836
Attached patch WIP (that doesn't work!) (obsolete) — Splinter Review
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.
Attachment #464270 - Flags: feedback?(Olli.Pettay)
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.
Attachment #464270 - Flags: feedback?(Olli.Pettay) → feedback-
"MozUnloadFrameListener" -> "MozUnloadFrameScript" ?
Btw, why not just call the event "close" ? Or "unload"?
Attached patch wip (obsolete) — Splinter Review
Another WIP
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → mark.finkle
Attachment #464270 - Attachment is obsolete: true
Attachment #464530 - Attachment is obsolete: true
Attachment #464691 - Flags: review?(Olli.Pettay)
Attached patch patch 2 (obsolete) — Splinter Review
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.
Attachment #464691 - Attachment is obsolete: true
Attachment #464733 - Flags: review?(Olli.Pettay)
Attachment #464691 - Flags: review?(Olli.Pettay)
Ah, sorry I forgot. nsContentUtils::DispatchTrustedEvent works currently with
nodes.
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.
Attachment #464733 - Flags: review?(Olli.Pettay) → review-
Attached patch patch 3Splinter Review
Addressed all the review feedback.
Attachment #464733 - Attachment is obsolete: true
Attachment #464788 - Flags: review?(Olli.Pettay)
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.
Attachment #464788 - Flags: review?(Olli.Pettay) → review+
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.
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?
Chris - if I leave the code alone, with respect to ActorDestroy, do you still think we need the mDestroyed flag you mentioned in IRC?
I would still like it, but that's orthogonal to your patch.  We can add that in another bug.
Blocks: 552828
(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.
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.
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.
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.
Attachment #465362 - Flags: review?(Olli.Pettay)
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.
Attachment #465362 - Flags: review?(Olli.Pettay) → review+
Blocks: 550936
pushed:
http://hg.mozilla.org/mozilla-central/rev/541caee57ec7
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
dev-doc-needed: this should be mentioned on https://developer.mozilla.org/en/The_message_manager#Content_scripts, right?
Keywords: dev-doc-needed
Target Milestone: --- → mozilla2.0
(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
You need to log in before you can comment on or make changes to this bug.