513354 - Destroy the baseWindow on tab destruction

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Frederic Plourde (:fred23)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2
Build Identifier: 

Upon TabChild destruction, the base window (baseWin) doesn't get destroyed, but should be.

Reproducible: Always

Comment 1

[Frederic Plourde (:fred23)]

Attachment: patch v.1

Tiny issue.
I might be missing something here, but I really think it's that simple.

Comment 2

[Benjamin Smedberg]

Comment on attachment 397351 [details] [diff] [review]
patch v.1

As discussed on IRC, it's not that simple, but it's a good start!

Comment 3

[Frederic Plourde (:fred23)]

Attachment: Walk-through in all nsWindow creations from E10s start to finish

I've been revisiting that bug and I'm kinda wondering what was the problem in the first place! As I understand its title, we wanna make sure that the TabChild's baseWindow gets destroyed on Tab destruction (on Window close, for example), that is : ->Destroy() gets called and OnDestroy event handler gets called and everything's cleaned up ok.

Well, I've debugged E10s from start to finish with the parent and child concurrent processes, breaking at each nsWindow::Create() and nsWindow::destroy()...as well as OnDestroy() event handler, and this attachment cleary shows that every single window that was created gets destroyed.

Thus, it's not surprising that my ->Destroy() call in patch v 1.0 above does no good, and just returns with NS_OK because "if (nsnull == mWnd)", meaning WM_DESTROY has already fired.

bsmedberg: maybe I'm mistaken, but I really thought this work wasn't already done in the "user closes firefox" case. By recalling our discussions we had long time ago about this, maybe this bug concerned destroying baseWindow in the "user close tab" case only ? ... which, atm, is unrelevant because there's not way of "closing a tab"...


Note: For each call, I included the call stack (to give hints on the kind of window it is) and there is some notes indicating what's happening UI-wise.

Comment 4

[Frederic Plourde (:fred23)]

Attachment: first draft

Here's a first draft... With you initial comments, I'll make a first real patch, updated to tip.

question : I've added two buttons in the chrome (test-ipc.xul). "Open Tab" and "Close Tab" which, in turn, opens or closes our single tab. I find it convenient for now, but.. would you keep it in the patch ?

Comment 5

[Benjamin Smedberg]

Comment on attachment 409123 [details] [diff] [review]
first draft

>diff --git a/content/base/src/nsFrameLoader.cpp b/content/base/src/nsFrameLoader.cpp
>--- a/content/base/src/nsFrameLoader.cpp
>+++ b/content/base/src/nsFrameLoader.cpp
>@@ -773,6 +773,13 @@ nsFrameLoader::Destroy()
>   }
>   mDestroyCalled = PR_TRUE;
> 
>+#ifdef MOZ_IPC
>+  if (mChildProcess) {
>+     ContentProcessParent::GetSingleton()->DestroyTab(mChildProcess);
>+     mChildProcess = nsnull;

I think you should early-return out of this function: the rest of the function applies to in-process frameloaders only.

>diff --git a/dom/ipc/ContentProcessChild.cpp b/dom/ipc/ContentProcessChild.cpp
>--- a/dom/ipc/ContentProcessChild.cpp
>+++ b/dom/ipc/ContentProcessChild.cpp
>@@ -127,7 +127,7 @@ ContentProcessChild::DeallocPNecko(PNeck
> void
> ContentProcessChild::Quit()
> {
>-    NS_ASSERTION(mQuit, "Exiting uncleanly!");
>+    NS_ASSERTION(mQuit, "Exiting uncleanly!");
>     mIFrames.Clear();
>     mTestShells.Clear();
> }

What happened here? Windows-style line ending or something? Please make sure you only use linux-style line endings in the code?

>diff --git a/dom/ipc/ContentProcessParent.cpp b/dom/ipc/ContentProcessParent.cpp
>--- a/dom/ipc/ContentProcessParent.cpp
>+++ b/dom/ipc/ContentProcessParent.cpp
>@@ -86,13 +86,18 @@ ContentProcessParent::GetSingleton()
> TabParent*
> ContentProcessParent::CreateTab(const MagicWindowHandle& hwnd)
> {
>-  return static_cast<TabParent*>(SendPIFrameEmbeddingConstructor(hwnd));
>+    return static_cast<TabParent*>(SendPIFrameEmbeddingConstructor(hwnd));
>+}

Please don't reindent code that's not directly related to your patch.

>+void
>+ContentProcessParent::DestroyTab(PIFrameEmbeddingParent *frame) {
>+    SendPIFrameEmbeddingDestructor(frame);
> }

Opening brace of method goes in column 0

>diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp

>@@ -62,7 +61,7 @@ TabChild::TabChild(const MagicWindowHand
> 
>     nsCOMPtr<nsIWebBrowser> webBrowser(do_CreateInstance(NS_WEBBROWSER_CONTRACTID));
> 
>-    nsCOMPtr<nsIBaseWindow> baseWindow = do_QueryInterface(webBrowser);
>+    baseWindow = do_QueryInterface(webBrowser);

You need to merge forward, I think: this code was refactored slightly in bug 523224.

>diff --git a/dom/ipc/TabChild.h b/dom/ipc/TabChild.h

> private:
>     nsCOMPtr<nsIWebNavigation> mWebNav;
>+    nsCOMPtr<nsIBaseWindow> baseWindow;

This change is unnecessary: you can get from mWebNav to nsIBaseWindow any time you want using QueryInterface (and member variables must be named like mFoo anyway).

>diff --git a/dom/ipc/test.xul b/dom/ipc/test.xul

I think it would probably be better, for now, just to open a new test-ipc.xul window, rather than trying to do dynamic tabs in the existing window.

Comment 6

[Frederic Plourde (:fred23)]

Attachment: patch v.2

I asked 'bz' about whether or not we might have use cases for frameloaders' lifetime extending beyond a single widget Create/Destroy cycle, and he said we might. So I preferred not to call ->senddestroyWidget from nsFrameLoader::Hide() but rather from nsFrameLoader::Destroy();

Besides, everything in Comment #5 was done in v.2

Comment 7

[Benjamin Smedberg]

Comment on attachment 411475 [details] [diff] [review]
patch v.2

I don't think there's any reason to have a separate destroyWidget message if we're always going to call DestroyTab immediately after... I think I wanted the equivalent of nsFrameLoader::Hide, which makes the widget invisible without destroying it.

Also, why bother with the DestroyTab method when you could just call SendPIFrameEmbeddingDestructor directly?

Comment 8

[Frederic Plourde (:fred23)]

Attachment: patch v.3

patch v.3 according to what we discussed. I got rid of the senddestroyWidget(), but felt like keeping destroying it within the tabChild's destructor. I protected the call with "bool mWidgetCreated"

Comment 9

[Frederic Plourde (:fred23)]

Attachment: patch v.4

pathch v.3 was the wrong one.

here's the good one, though I retouched it a little... and I've noticed we're not doing anything useful with the return value of TabChild::destroyWidget()... just returning true in any case.

Comment 10

[Frederic Plourde (:fred23)]

Confirmed. Patch applies to tip. Checkin-needed.

Comment 11

[Frederic Plourde (:fred23)]

Attachment: patch to land (with commit info)

patch with commit info.

Comment 12

[Frederic Plourde (:fred23)]

Pushed http://hg.mozilla.org/projects/electrolysis/rev/58ef47386035

Comment 13

[Boris Zbarsky [:bzbarsky]]

So I'm confused.  I thought I had explained that all widgetry needs to die on hide/show... is there a reason we decided to not do that?

Comment 14

[Frederic Plourde (:fred23)]

Not closing this bug yet...

I just noticed an intermittent warning : "WARNING: No docshells for remote frames!: file d:/WORK/collabora/electrolysis/codebase/content/base/src/nsFrameLoader.cpp, line 351".

... not sure it's related to this patch, but could be. I'm going to gather some info (stack trace, etc...) and we'll see.

Comment 15

[Doug Turner (:dougt)]

fred should we file a follow up - separate issue now deserves a separate bug.

Comment 16

[Frederic Plourde (:fred23)]

Not sure we're still having this warning in mobile. I'll check again and file another bug if needed.

Comment 17

[Doug Turner (:dougt)]

marking fixed.