Sotaro Ikeda [:sotaro] Assignee
	Description • 11 years ago

+++ This bug was initially created as a clone of Bug #1000525 +++

This bug is created based on Bug 1000525 Comment 34.

During testing the following STR. LayerTransactionChild, LayerTransactionParent and ShadowLayerForwarder are created for each window.open(). But they are not destructed until the application is killed.

STR:
* Run the test app at bug 964386 attachment 8366455 [details] on master b2g device.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 1 • 11 years ago

It blocks 1.4+ bug.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 2 • 11 years ago

I confirmed that the window's TabParent::Destroy() and TabChild::Destroy() are called in the STR.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 3 • 11 years ago

(In reply to Sotaro Ikeda [:sotaro] from comment #2)
> I confirmed that the window's TabParent::Destroy() and TabChild::Destroy()
> are called in the STR.

Correction:
TabParent::Destroy() and TabChild::RecvDestroy()

	Sotaro Ikeda [:sotaro] Assignee
	Comment 4 • 11 years ago

I did misunderstanding. ShadowLayerForwarder was correctly destroyed.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 5 • 11 years ago

I understand why the leak happens.

LayerTransactionChild is reference counted IPC object. It is explicitly add-refed by CompositorChild. Therefore to release it, it need to be explicitly released. The relationship between LayerTransactionChild and ShadowLayerForwarder is very similar to TextureChild and TextureClient.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 6 • 11 years ago

By applying the patch, I confirmed that LayerTransactionChild is destroyed on master nexus-5.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 7 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=46532d199746

	Sotaro Ikeda [:sotaro] Assignee
	Comment 8 • 11 years ago

(In reply to Sotaro Ikeda [:sotaro] from comment #7)
> https://tbpl.mozilla.org/?tree=Try&rev=46532d199746

PLayerTransaction shutdown seems to have a problem. I am going to try PLayerTransaction shutdown change in Bug 1000525.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 9 • 11 years ago

Apply LayerTransaction shutdown fix in Bug 1000525.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 10 • 11 years ago

Fix nits.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 11 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=fa4a6ff4c1d3

	Sotaro Ikeda [:sotaro] Assignee
	Comment 13 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=322be398e075

	Sotaro Ikeda [:sotaro] Assignee
	Comment 14 • 11 years ago

Add more IPC Open checks.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 15 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=2ccba4f2839c

	Sotaro Ikeda [:sotaro] Assignee
	Comment 16 • 11 years ago

Add more IPC Open check.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 17 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=d85f0d1255fa

	Sotaro Ikeda [:sotaro] Assignee
	Comment 18 • 11 years ago

This bug no longer blocks bug 1000525. This bug seems difficult than I thought at first. It seem better to handle separately from bug 1000525.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 19 • 11 years ago

This bug is present since b2g v1.2. It seems to be added as part of new gfx layers. And Memory leak of LayerTransactionChild and LayerTransactionParent is small. We seem not have much necessity to fix this bug in b2g v1.4.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 20 • 11 years ago

Set "1.4?" based on Comment 19.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 23 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=5639af9ecadb

	Sotaro Ikeda [:sotaro] Assignee
	Comment 25 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=22888ff08911

	Sotaro Ikeda [:sotaro] Assignee
	Comment 26 • 11 years ago

(In reply to Sotaro Ikeda [:sotaro] from comment #25)
> https://tbpl.mozilla.org/?tree=Try&rev=22888ff08911

There are some test failures. It happens when LayerTransactionChild is destroyed via CompositorChild::Destroy(). CompositorChild does not reference count LayerTransactionChild. It directly uses raw pointer. During CompositorChild calling LayerTransactionChild::Destroy(), LayerTransactionChild is destroyed by the following sequence.

nsBaseWidget::DestroyCompositor()
->CompositorChild::Destroy()
->LayerTransactionChild::Destroy()
->PLayerTransactionChild::Send__delete__()
->"destroy TextureChilds"
   This removes all reference to ShadowLayerForwarder.
->ShadowLayerForwarder::~ShadowLayerForwarder()
  ->remove reference to PLayerTransactionChild
->PLayerTransactionChild's ref count becomes 0

	Sotaro Ikeda [:sotaro] Assignee
	Comment 27 • 11 years ago

Fix how to call LayerTransactionChild::Destroy() from ShadowLayerForwarder::~ShadowLayerForwarder().

	Sotaro Ikeda [:sotaro] Assignee
	Comment 28 • 11 years ago

I also found another problem during destroying compositor. Before PTextureParent::Send__delete__() arrive to TexuteChild, TextureChild is already deleted.

TextureChild/TextureParent is destroyed like the following sequence.

TextureChild::SendRemoveTexture()
->TextureParent::RecvRemoveTextureSync()
->PTextureParent::Send__delete__()

During the destroying compositor, TextureChild::SendRemoveTexture()s are called. Then the parent sides send PTextureParent::Send__delete__(). But before the message arrival to the TextureChilds, they are already deleted. Then the following errors happen.

> ###!!! [Child][DispatchAsyncMessage] Error: Route error: message sent to unknown actor ID

	Sotaro Ikeda [:sotaro] Assignee
	Comment 29 • 11 years ago

(In reply to Sotaro Ikeda [:sotaro] from comment #27)
> Created attachment 8416911 [details] [diff] [review]
> patch v10 - Destroy LayerTransactionChild(r=bjacob)
> 
> Fix how to call LayerTransactionChild::Destroy() from
> ShadowLayerForwarder::~ShadowLayerForwarder().

For fixing, Task is used. But it hits the pending task check during shutdown.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 30 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=25aa9d5af03d

	Sotaro Ikeda [:sotaro] Assignee
	Comment 31 • 11 years ago

> nsBaseWidget::DestroyCompositor()
> ->CompositorChild::Destroy()
> ->LayerTransactionChild::Destroy()
> ->PLayerTransactionChild::Send__delete__()
> ->"destroy TextureChilds"
>    This removes all reference to ShadowLayerForwarder.
> ->ShadowLayerForwarder::~ShadowLayerForwarder()
>   ->remove reference to PLayerTransactionChild
> ->PLayerTransactionChild's ref count becomes 0

The following corrects call sequence in Comment 26. 

CompositorChild::Destroy()
  ->LayerTransactionChild::Destroy()
    ->PLayerTransactionChild::Send__delete__()
      ->PLayerTransactionChild::DestroySubtree();
        ->PLayerTransactionChild::ActorDestroy()
      ->PLayerTransactionChild::DeallocSubtree();
        ->ShadowLayerForwarder::~ShadowLayerForwarder()
          ->LayerTransactionChild::Destroy()
            ->if (IPCOpen == true)
              ->PLayerTransactionChild::Send__delete__()
                -> ###!!! ABORT: actor has been |delete|d: !!!!!!!!!!!!!!!!!!!
      ->PCompositorChild::RemoveManagee()
        ->CompositorChild::DeallocPLayerTransactionChild()
          ->LayerTransactionChild::ReleaseIPDLReference()
            ->LayerTransactionChild::mIPCOpen = false;
                        // Set IPCOpen flag to false here !!!!!!!!!!!!!!!!!!
            ->LayerTransactionChild::Release();

Second LayerTransactionChild::Destroy() is called withing the first LayerTransactionChild::Destroy() call. When the second LayerTransactionChild::Destroy() is called, The "IPCOpen flag" is still true.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 32 • 11 years ago

Remove Task usage for calling LayerTransactionChild::Destroy().

	Sotaro Ikeda [:sotaro] Assignee
	Comment 33 • 11 years ago

Add mDestroyed flag to LayerTransactionChild.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 34 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=f512dc914d65

	Sotaro Ikeda [:sotaro] Assignee
	Comment 35 • 11 years ago

(In reply to Sotaro Ikeda [:sotaro] from comment #34)
> https://tbpl.mozilla.org/?tree=Try&rev=f512dc914d65

tryserver results seems good. attachment 8416958 [details] [diff] [review] still has a problem of Comment 28. But it seems not affect to the test results.

	Nicolas Silva [:nical]
	Comment 36 • 11 years ago

Comment on attachment 8416958 [details] [diff] [review]
patch v12 - Destroy LayerTransactionChild(r=bjacob)

Review of attachment 8416958 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

	Nicolas Silva [:nical]
	Comment 37 • 11 years ago

(In reply to Sotaro Ikeda [:sotaro] from comment #28)
> I also found another problem during destroying compositor. Before
> PTextureParent::Send__delete__() arrive to TexuteChild, TextureChild is
> already deleted.
> 
> TextureChild/TextureParent is destroyed like the following sequence.
> 
> TextureChild::SendRemoveTexture()
> ->TextureParent::RecvRemoveTextureSync()
> ->PTextureParent::Send__delete__()
> 
> During the destroying compositor, TextureChild::SendRemoveTexture()s are
> called. Then the parent sides send PTextureParent::Send__delete__(). But
> before the message arrival to the TextureChilds, they are already deleted.
> Then the following errors happen.
> 
> > ###!!! [Child][DispatchAsyncMessage] Error: Route error: message sent to unknown actor ID

This looks like bug 966284.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 38 • 11 years ago

> > 
> > During the destroying compositor, TextureChild::SendRemoveTexture()s are
> > called. Then the parent sides send PTextureParent::Send__delete__(). But
> > before the message arrival to the TextureChilds, they are already deleted.
> > Then the following errors happen.
> > 
> > > ###!!! [Child][DispatchAsyncMessage] Error: Route error: message sent to unknown actor ID
> 
> This looks like bug 966284.

I think that it is different. The above error caused by shutdown sequence difference between PCompositor/PLayerTransaction and PTexture. PCompositor and PLayerTransaction actually close the protocols from child side. But PTexture actually close from the parent side, though TextureChild triggers the protocol close from the child side by calling PTextureChild::SendRemoveTexture().

bug 966284 is about the problem of TextureParent::RecvRemoveTextureSync(). ipdl protocol seems not to expect to delete the protocol during receiving sync message.

	Nicolas Silva [:nical]
	Comment 39 • 11 years ago

Bug 966284 is about making sure that a manager protocol doesn't "interrupt" it's managees when it gets destroyed. I write "interrupt" as in force the managees to be destroyed synchronously while it's still in the middle of an asynchronous conversation or a shutdown handshake.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 40 • 11 years ago

Un-bitrot.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 41 • 11 years ago

Fix nits.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 42 • 11 years ago

Update a comment.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 43 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=6bc9dda2ee36

	Sotaro Ikeda [:sotaro] Assignee
	Comment 44 • 11 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/b196df92475e

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 45 • 11 years ago

Sotaro, this still blocks bug 998504?

	Sotaro Ikeda [:sotaro] Assignee
	Comment 46 • 11 years ago

(In reply to Milan Sreckovic [:milan] from comment #45)
> Sotaro, this still blocks bug 998504?

This bug does not block bug 998504.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 47 • 11 years ago

(In reply to Sotaro Ikeda [:sotaro] from comment #46)
> (In reply to Milan Sreckovic [:milan] from comment #45)
> > Sotaro, this still blocks bug 998504?
> 
> This bug does not block bug 998504.

It is commented in Comment 19.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 48 • 11 years ago

(In reply to Sotaro Ikeda [:sotaro] from comment #46)
> (In reply to Milan Sreckovic [:milan] from comment #45)
> > Sotaro, this still blocks bug 998504?
> 
> This bug does not block bug 998504.

Thanks, just double checking; I removed the dependency.

	Andrew Overholt [:overholt]
	Comment 49 • 11 years ago

So 1.4? -> backlog?

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 50 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/b196df92475e

	Sotaro Ikeda [:sotaro] Assignee
	Comment 51 • 10 years ago

This problem could cause file descriptors leak with bugs like Bug 988135, Bug 1006957. It seems better to be uplifted to b2g v1.4. But this bug's patch have a dependency to Bug 933082. It is a relatively big change :-(

	Sotaro Ikeda [:sotaro] Assignee
	Comment 52 • 10 years ago

Nominate to b2g v1.4 based on Comment 51.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 53 • 10 years ago

(In reply to Sotaro Ikeda [:sotaro] from comment #51)
> But this bug's
> patch have a dependency to Bug 933082. It is a relatively big change :-(

The above part is my misunderstanding. It is already in b2g v1.4:-)

	Sotaro Ikeda [:sotaro] Assignee
	Comment 54 • 10 years ago

Patch for b2g v1.4. Carry r=nical,bjacob.

	Ryan VanderMeulen [:RyanVM]
	Comment 55 • 10 years ago

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/b997f98c96fe